From owner-cvs-src@FreeBSD.ORG Thu Sep 22 10:18:49 2005 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id DE91C16A41F; Thu, 22 Sep 2005 10:18:49 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from cyrus.watson.org (cyrus.watson.org [204.156.12.53]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8605E43D45; Thu, 22 Sep 2005 10:18:49 +0000 (GMT) (envelope-from rwatson@FreeBSD.org) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by cyrus.watson.org (Postfix) with ESMTP id 1D55C46BA6; Thu, 22 Sep 2005 06:18:49 -0400 (EDT) Date: Thu, 22 Sep 2005 11:18:48 +0100 (BST) From: Robert Watson X-X-Sender: robert@fledge.watson.org To: John Baldwin In-Reply-To: <200509211545.39246.jhb@FreeBSD.org> Message-ID: <20050922111533.G34322@fledge.watson.org> References: <200509190310.j8J3ALgt095979@repoman.freebsd.org> <200509211455.59154.jhb@FreeBSD.org> <20050921190250.GX36166@cell.sick.ru> <200509211545.39246.jhb@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@FreeBSD.org, Gleb Smirnoff , Ruslan Ermilov , "M. Warner Losh" Subject: Re: cleanup of interface shutdown/detach Was: cvs commit: src/sys/dev/an X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Sep 2005 10:18:50 -0000 On Wed, 21 Sep 2005, John Baldwin wrote: >> This check confuses people, is incorrect and useless. It confuses >> people, because they think that the !IFF_DRV_RUNNING condition is >> checked. It is incorrect because upper layer must not touch/look >> at if_drv_flags. It is useless because the flag is checked without >> driver mutex being acquired, and thus does not protect from anything. >> >> Yesterday I have fixed panic in em(4) that was "protected" by this >> check. The correct way is to check the flag in interface start >> method, with driver mutex held. > > It can sometimes be ok to check a flag twice to optimize the common case: > > if (!(foo & IF_FOO)) > return; > FOO_LOCK(foo); > if (!(foo & IF_FOO)) { > FOO_UNLOCK(foo); > return; > } > ... > FOO_UNLOCK(foo); > > This can be useful if IF_FOO is often false and if you don't lose > anything by reading a stale value for the check (for example, if you > poll it every so often then if you lose the race you just lose it until > the next poll). In the case of IFF_DRV_RUNNING it's probably not a big deal, because we want to optimize for the driver running case rather than the driver not running case. However, for IFF_DRV_OACTIVE, we actually want to optimize for the case where the output routine is already active and we're rapidly enqueueing packets for the driver to send on its next interrupt, I think. So adding additional lock acquires in that case may be a problem. Something we discussed at BSDCan was eliminating the ifq lock by having the driver softc (or softc send) lock protect the queue. This potentially increases contention on enqueue because driver locks are held a fair amount on send. However, it reduces the total locking operations by 4 for a simple packet send. We'd need to carefully work out the contention costs though. If we do successfully start breaking out network device drivers into two locks: send and receive paths, then the content is likely to be much less of a problem because the send enqueue would no longer contend with the receive processing. Robert N M Watson