Re: kqueue for usb_dev

On 02/27/14 08:42, Kohji Okuno wrote:
> From: Hans Petter Selasky <hps_at_bitfrost.no>
>> On 02/27/14 08:13, Kohji Okuno wrote:
>>> Hi John-Mark,
>>>
>>> Thank you for you comment.
>>>
>>> From: John-Mark Gurney <jmg_at_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.
>
> Hi,
>
> Thank you for you comment.
> 1) You are right.
>
> 2) I think that priv_mtx is hold in caller function.
>     Would you refer to kqueue_scan() in kern/kern_event.c?


Hi,

You are right!

I will add an assert there instead.

--HPS