cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h

[Nate Lawson]

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