Date: Thu, 27 Feb 2014 08:32:58 +0100 From: Hans Petter Selasky <hps@bitfrost.no> To: Kohji Okuno <okuno.kohji@jp.panasonic.com>, jmg@funkthat.com Cc: freebsd-current@freebsd.org Subject: Re: kqueue for usb_dev Message-ID: <530EEA2A.50604@bitfrost.no> In-Reply-To: <20140227.161317.453723361596662298.okuno.kohji@jp.panasonic.com> References: <20140227.142615.924807465819500067.okuno.kohji@jp.panasonic.com> <20140227060232.GA47921@funkthat.com> <20140227.161317.453723361596662298.okuno.kohji@jp.panasonic.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?530EEA2A.50604>