Re: [PATCH] Fix OACTIVE for an(4)

On Friday, October 03, 2014 12:13:28 PM Gleb Smirnoff wrote:
> On Thu, Oct 02, 2014 at 11:16:27AM -0400, John Baldwin wrote:
> J> > I haven't looked at the rest of the driver; is everything else around
> J> > OACTIVE locked correctly and consistently?
> J>
> J> As well as OACTIVE is for any other driver.
> 
> Let me jump into this topic and discuss the if_drv_flags :)
> 
> It seems to me that this in general was a wrong concept, both
> IFF_DRV_OACTIVE and IFF_DRV_RUNNING. The internal state of
> the driver can be known only to the driver itself and should
> be stored in the softc, covered with internal lock.
> 
> There is simply no way to racelessly tell the state from the
> outside, without obtaining driver lock.
> 
> In my ifnet plans I am considering to remove if_drv_flags.
> Can anyone convince me that this is a wrong idea and they
> should be kept?

They used to be in if_flags.  Robert moved them to if_drv_flags precisely so 
that drivers could control their synchronization instead of doing a crazy 
locking dance with the ifnet layer.  They are still exported to userland as 
IFF_RUNNING and IFF_OACTIVE in if_flags, and they may still be somewhat useful 
for reporting that state to userland (and races in reading if_drv_flags for 
that reporting are harmless).

That said, I think long term if we make the stack aware of multiple input 
queues we will certainly use something that does not have the same raciness as 
OACTIVE.

Note, btw, you could fix OACTIVE in various drivers by simply grabbing the 
IF_LOCK on ifp->if_snd while setting or clearing OACTIVE to close the current 
OACTIVE race (you don't need it for all writes to if_drv_flags, you just want 
if_handoff() to read the current value of OACTIVE, so only locking writes to 
OACTIVE would be sufficient).

-- 
John Baldwin