Date: Fri, 03 Oct 2014 08:58:25 -0400 From: John Baldwin <jhb@freebsd.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Adrian Chadd <adrian@freebsd.org>, freebsd-current <freebsd-current@freebsd.org> Subject: Re: [PATCH] Fix OACTIVE for an(4) Message-ID: <16070615.NdPaubu6kG@ralph.baldwin.cx> In-Reply-To: <20141003081328.GZ73266@FreeBSD.org> References: <2113392.UOaBFTpimf@ralph.baldwin.cx> <201410021116.27583.jhb@freebsd.org> <20141003081328.GZ73266@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?16070615.NdPaubu6kG>