Re: kqueue for usb_dev

On 02/27/14 08:13, Kohji Okuno wrote:
> Hi John-Mark,
>
> Thank you for you comment.
>
> From: John-Mark Gurney <jmg@funkthat.com>
>> Kohji Okuno wrote this message on Thu, Feb 27, 2014 at 14:26 +0900:
>>> I tried add kqueue I/F to usb_dev.c. I attached my patch.
>>> What do you think about my patch?
>>
>> A few comments...
>>
>> 1) You should just drop the use of flag_iskevent and just
>> unconditionally call KNOTE... since you have the lock already held,
>> the cost is minimal (and w/ modern branch prediction, may be cheaper)...
>
> Should we set the use of flag_iskevent, when usb_filter_read() and
> usb_filter_write() return `0'?
>
>
>> 2) Why do you try to start read/write transfers in the _filter?  You
>> should just check to see if data is available and not do work..  This
>> is also important since kqueue calls the filter just before delivering
>> the knote to userland to verify that there is still data, and it will
>> call your _event function for each knote on the fd...  The work should
>> be started through other mechanisms, like read/write syscall or
>> interrupt or timeout/callout...  If it's required to get results from
>> USB_IF_POLL, then it's fine..
>
> I copied from usb_poll().
> Should we try to start read/write transfers in usb_kqfilter()?
> Or should not we try to start read/write transfers in poll and kqueue?
>
>> 3) I don't see any calls to knlist_destroy... These calls are needed
>> to clean up the knlist...
>
> I understood.
>
>> Obviously the #if 1's will need to go...
>>
>> Also, I don't think your change is against HEAD..  The line numbers
>> in my version of usb_dev.c are different...
>
> I'm sorry.

Hi,

I've found two bugs:

1)

+#if 1
+       knlist_init_mtx(&f_tx->selinfo.si_note, f_rx->priv_mtx);
+#endif

Should be:

+#if 1
+       knlist_init_mtx(&f_tx->selinfo.si_note, f_tx->priv_mtx);
+#endif

2)

Event filters need to lock the FIFO's mutex.

BTW: I'm working on getting the code into -HEAD. I'll run some test 
before it goes in.

--HPS
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"