From owner-freebsd-current@FreeBSD.ORG Thu Feb 27 07:43:08 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 61C77E5F for ; Thu, 27 Feb 2014 07:43:08 +0000 (UTC) Received: from smtp.mei.co.jp (smtp.mei.co.jp [133.183.100.20]) by mx1.freebsd.org (Postfix) with ESMTP id D8FF0143B for ; Thu, 27 Feb 2014 07:43:07 +0000 (UTC) Received: from mail-gw.jp.panasonic.com ([157.8.1.157]) by smtp.mei.co.jp (8.12.11.20060614/3.7W/kc-maile14) with ESMTP id s1R7gx39025571; Thu, 27 Feb 2014 16:42:59 +0900 (JST) Received: from epochmail.jp.panasonic.com ([157.8.1.130]) by mail.jp.panasonic.com (8.11.6p2/3.7W/kc-maili16) with ESMTP id s1R7gxM17005; Thu, 27 Feb 2014 16:42:59 +0900 Received: by epochmail.jp.panasonic.com (8.12.11.20060308/3.7W/lomi17) id s1R7gxjT019219; Thu, 27 Feb 2014 16:42:59 +0900 Received: from localhost by lomi17.jp.panasonic.com (8.12.11.20060308/3.7W) with ESMTP id s1R7gxPD019184; Thu, 27 Feb 2014 16:42:59 +0900 Date: Thu, 27 Feb 2014 16:42:58 +0900 (JST) Message-Id: <20140227.164258.18190762432316083.okuno.kohji@jp.panasonic.com> To: hps@bitfrost.no Subject: Re: kqueue for usb_dev From: Kohji Okuno In-Reply-To: <530EEA2A.50604@bitfrost.no> References: <20140227060232.GA47921@funkthat.com> <20140227.161317.453723361596662298.okuno.kohji@jp.panasonic.com> <530EEA2A.50604@bitfrost.no> Organization: Panasonic Corporation X-Mailer: Mew version 6.5 on Emacs 24.3 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: jmg@funkthat.com, freebsd-current@freebsd.org, okuno.kohji@jp.panasonic.com 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 07:43:08 -0000 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? Thanks, Kohji Okuno