Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 27 Feb 2014 16:42:58 +0900 (JST)
From:      Kohji Okuno <okuno.kohji@jp.panasonic.com>
To:        hps@bitfrost.no
Cc:        jmg@funkthat.com, freebsd-current@freebsd.org, okuno.kohji@jp.panasonic.com
Subject:   Re: kqueue for usb_dev
Message-ID:  <20140227.164258.18190762432316083.okuno.kohji@jp.panasonic.com>
In-Reply-To: <530EEA2A.50604@bitfrost.no>
References:  <20140227060232.GA47921@funkthat.com> <20140227.161317.453723361596662298.okuno.kohji@jp.panasonic.com> <530EEA2A.50604@bitfrost.no>

next in thread | previous in thread | raw e-mail | index | archive | help
From: Hans Petter Selasky <hps@bitfrost.no>
> 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.

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?

Thanks,
 Kohji Okuno



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140227.164258.18190762432316083.okuno.kohji>