Date: Fri, 25 Apr 2003 18:54:57 -0600 From: Scott Long <scott_long@btc.adaptec.com> To: Nate Lawson <nate@root.org> Cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h Message-ID: <3EA9D8E1.2090307@btc.adaptec.com> In-Reply-To: <Pine.BSF.4.21.0304251032540.65457-100000@root.org> References: <Pine.BSF.4.21.0304251032540.65457-100000@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Nate Lawson wrote: > On Fri, 25 Apr 2003, Sam Leffler wrote: > >>>For developers, note that the locking in the code path only protects the >>>various fxp routines (fxp_start, fxp_intr, fxp_tick, ...) and is not >>>intended to serialize access to ANY external structures. This is how it >>>should be. Please do not copy the exact approach taken here for a little >>>while until ifnet locking is finished as there may need to be some changes >>>made to this model. >> >>This doesn't make much sense to me. I've locked numerous chunks of code and >>used a totally different approach: synchronize access to data structures, >>not code paths. Perhaps you and Jeffrey Hsu need to have a private >>discussion... > > > I wrote the comment at 2 am so let me rephrase this: > > 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. The approach that I took with locking the aac(4) driver was based on the assumption that mutex operations are somewhat expensive, both in terms of the overhead of manipulating the lock and in the cost of sleeping on a contested lock. Since going from the main entrypoint (aac_disk_strategy()) to delivering the i/o to the hardware is fairly quick, I decided that there was little benefit in doing fined grained locking on ieach of the softc, queues, and registers. The few places in the path where resource shortages could happen were refactored also. 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. > > I have posted this work in progress over the course of the past two weeks > and it has been reviewed in various states by gallatin@ and mux@. My > approach is extremely similar to one gallatin@ sent me. I would > appreciate any specific input from hsu@ or others to improve this code. 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. Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3EA9D8E1.2090307>