From owner-cvs-src@FreeBSD.ORG Fri Apr 25 18:24:08 2003 Return-Path: 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 2184537B404 for ; Fri, 25 Apr 2003 18:24:08 -0700 (PDT) Received: from rootlabs.com (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id 0A02543FE1 for ; Fri, 25 Apr 2003 18:24:07 -0700 (PDT) (envelope-from nate@rootlabs.com) Received: (qmail 66167 invoked by uid 1000); 26 Apr 2003 01:24:08 -0000 Date: Fri, 25 Apr 2003 18:24:08 -0700 (PDT) From: Nate Lawson To: Scott Long In-Reply-To: <3EA9D8E1.2090307@btc.adaptec.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@freebsd.org cc: src-committers@freebsd.org cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 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: Sat, 26 Apr 2003 01:24:08 -0000 On Fri, 25 Apr 2003, Scott Long wrote: > Nate Lawson wrote: > > I am not locking the code path. I am locking the softc, device registers, > > and any resources shared _within_ the driver. I am NOT locking ifp > > accesses or other external objects. This work is merely the driver end > > node locking and makes as few assumptions about the outside world as > > possible. > > > > However, I did not make a huge effort to refactor the code path and as > > such, the locking is not nearly fine-grained enough to be called a > > finished product. For instance, fxp_intr() holds sc->sc_lock for the > > entire duration of the routine as it accesses various card registers and > > softc variables throughout. It may make sense to lock/unlock the softc > > multiple times and refactor fxp_intr() to allow this but on the other > > hand, this may require too many mutex operations. The only way to tell is > > by testing and profiling. > > I'm not familiar with inner workings of fxp or other network drivers, > but it sounds like you took a similar approach. My feeling is that > the greatest benefit comes from removing Giant in the first place. Once > this first step is taken, fine-tuning locking strategies can be debated > and code paths can be profiled. Agreed 100%. There may end up being a task queue or other construct. The first step was to make the existing code work with the minimal reworking of flow. Other than a "pre-lock" setup for fxp_start and fxp_init to avoid mutex recursion (fxp_intr was already split for the polling case), I did not rework the fast path. (I did some small attach reworking to fix some potential memory leaks but that was a separate effort from locking). > Thanks Nate, this looks like a great first step. I'm not clear from > your previous emails if you have actually run the driver with Giant > removed, nor if/how you handle calling into the ifnet code with your > strategy. I have run various versions of the patch for about 3 weeks and the final version with no changes for about a week, all without Giant. The reason why I did not see ifnet problems even though I processed ~400M packets was because all ifnet processing happened to be with the fxp lock held and my laptop only had one network interface. This is not an intentional part of the patch; it is not an attempt to protect ifnet with a local fxp lock! It is just a side effect and if you have multiple interfaces processing packets, you WILL have problems until the ifnet locking is done. That is why I did not enable MPSAFE. Once hsu@ gives the ok that ifnet is done and there are no other parts that need Giant, we can enable MPSAFE and test things and do lock pushdown where appropriate. -Nate