From owner-freebsd-arch@FreeBSD.ORG Fri Sep 30 14:06:07 2005 Return-Path: X-Original-To: freebsd-arch@freebsd.org Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 8FDCE16A427; Fri, 30 Sep 2005 14:06:07 +0000 (GMT) (envelope-from jhb@FreeBSD.org) Received: from mv.twc.weather.com (mv.twc.weather.com [65.212.71.225]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2397943D48; Fri, 30 Sep 2005 14:06:05 +0000 (GMT) (envelope-from jhb@FreeBSD.org) Received: from [10.50.41.233] (Not Verified[10.50.41.233]) by mv.twc.weather.com with NetIQ MailMarshal (v6, 0, 3, 8) id ; Fri, 30 Sep 2005 10:22:03 -0400 From: John Baldwin To: freebsd-arch@freebsd.org Date: Fri, 30 Sep 2005 10:07:09 -0400 User-Agent: KMail/1.8 References: <20050930124000.GA45345@cell.sick.ru> In-Reply-To: <20050930124000.GA45345@cell.sick.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-6" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200509301007.10494.jhb@FreeBSD.org> Cc: Gleb Smirnoff , net@freebsd.org Subject: Re: [REVIEW/TEST] polling(4) changes X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Sep 2005 14:06:07 -0000 On Friday 30 September 2005 08:40 am, Gleb Smirnoff wrote: > [please, follow-up on net@ only] > > Colleagues, > > here are some patches for review. > > Problems addressed: > > 1) When Giant was removed from polling a problem was introduced. The id= le > poll feature was broken. The idle poll thread can enter polling handler on > one interface and put to sleep for a long time, until CPU resources found. > During this time no traffic is received on interface. Well, this is what > idle thread is supposed to do. Why didn't this happen with Giant? Because > idle poll entered poll handler holding Giant, and other threads (in > particular netisr poll) contested on Giant and propagated their priority = to > idle poll. Well, this is a hack, but idle poll significantly improves > polling performance on an idle box, that's why I won't axe it but try to > fix it. > > To address the problem we need to use the same technique as before, but > use poll_mtx instead of Giant. However, this will resurrect LORs, that we= re > fixed with Giant removal. The alternative lock path happens, when driver > decides to deregister from polling itself. The LOR is fixed by further > changes. See 3). > > 2) Drivers indicate their ability to do polling(4) with IFCAP_POLLING > flag int if_capabilites field. Setting the flag in if_capenable should > register interface with polling and disable interrupts. However, the > if_flags is also abused with IFF_POLLING flag. The aim is to remove > IFF_POLLING flag. > > 3) The polling is switched on and off not functionally. That is, when y= ou > say 'sysctl kern.polling.enable=3D1' or 'ifconfig fxp0 -polling', the pol= ling > is switched on/off not immediately but on next tick or next interrupt. Th= is > non-functional approach leads to a lot of ambiguouties in code, makes it > harder to understand and maintain. It also exposes race conditions. > > The attached patch removes: > - IFF_POLLING flag. > - Use of if_flags, if_drv_flags, if_capenable from kern_poll.c. > All current accesses to these fields are not locked and polling > shouldn't look there. > - poll in trap feature. Sorry, we can't acquire mutexes in trap(). Anyo= ne > used it, anyway? > - POLL_DEREGISTER command. No hacks. Everything is done functionally via > ioctl(). > > The new world order for driver is the following: > > 1) Declare IFCAP_POLLING in if_capabilities on attach. Do not touch > if_capenable. 2) in ioctl method, in SIOCSIFCAP case the driver should: > - call ether_poll_[de]register > - if no error, set the IFCAP_POLLING flag in if_capenable > - obtain driver lock > - [dis/en]able interrupts > - drop driver lock > 3) In poll method, check IFF_DRV_RUNNING flag after obtaining driver lo= ck > 4) In interrupt handler check IFCAP_POLLING flag in if_capenable. If > present, then return. This is important to protect from spurious > interrupts. 5) In device detach method, call ether_poll_deregister() befo= re > obtaining driver lock. =46rom my limited experience with locking various NIC drivers, I like this= =20 change. I think it is much better to tweak the polling state in the ioctl(= )=20 handler rather than in the poll handler. =2D-=20 John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" =3D http://www.FreeBSD.org