From owner-freebsd-current@FreeBSD.ORG Thu Feb 27 08:01:24 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 80F037A1 for ; Thu, 27 Feb 2014 08:01:24 +0000 (UTC) Received: from mta05.bitpro.no (mta05.bitpro.no [92.42.64.202]) by mx1.freebsd.org (Postfix) with ESMTP id 0C7821609 for ; Thu, 27 Feb 2014 08:01:23 +0000 (UTC) Received: from mail.lockless.no (mail.lockless.no [46.29.221.38]) by mta05.bitpro.no (Postfix) with ESMTPS id 26F5117FFBB; Thu, 27 Feb 2014 09:01:22 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by mail.lockless.no (Postfix) with ESMTP id 61318181AB8; Thu, 27 Feb 2014 09:02:20 +0100 (CET) X-Virus-Scanned: by amavisd-new-2.6.4 (20090625) (Debian) at lockless.no Received: from mail.lockless.no ([127.0.0.1]) by localhost (mail.lockless.no [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GlX-DfPnBrxh; Thu, 27 Feb 2014 09:02:19 +0100 (CET) Received: from laptop015.home.selasky.org (cm-176.74.213.204.customer.telag.net [176.74.213.204]) by mail.lockless.no (Postfix) with ESMTPSA id 04977181ABE; Thu, 27 Feb 2014 09:02:18 +0100 (CET) Message-ID: <530EF104.1020706@bitfrost.no> Date: Thu, 27 Feb 2014 09:02:12 +0100 From: Hans Petter Selasky Organization: Bitfrost A/S User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: Kohji Okuno Subject: Re: kqueue for usb_dev References: <20140227060232.GA47921@funkthat.com> <20140227.161317.453723361596662298.okuno.kohji@jp.panasonic.com> <530EEA2A.50604@bitfrost.no> <20140227.164258.18190762432316083.okuno.kohji@jp.panasonic.com> In-Reply-To: <20140227.164258.18190762432316083.okuno.kohji@jp.panasonic.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: jmg@funkthat.com, freebsd-current@freebsd.org X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Feb 2014 08:01:24 -0000 On 02/27/14 08:42, Kohji Okuno wrote: > From: Hans Petter Selasky >> On 02/27/14 08:13, Kohji Okuno wrote: >>> Hi John-Mark, >>> >>> Thank you for you comment. >>> >>> From: John-Mark Gurney >>>> 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