Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 17 Jul 2020 11:56:09 -0700
From:      John-Mark Gurney <jmg@funkthat.com>
To:        Marko Zec <zec@fer.hr>
Cc:        freebsd-net@FreeBSD.org
Subject:   Re: IF_DRV_PREPEND unlocked?
Message-ID:  <20200717185609.GX4213@funkthat.com>
In-Reply-To: <20200717120311.59377e0d@x23>
References:  <20200715232624.GR4213@funkthat.com> <20200716072622.5fa35ba2@x23> <20200716074917.04445daa@x23> <20200716185629.GT4213@funkthat.com> <20200717120311.59377e0d@x23>

next in thread | previous in thread | raw e-mail | index | archive | help
Marko Zec wrote this message on Fri, Jul 17, 2020 at 12:03 +0200:
> On Thu, 16 Jul 2020 11:56:29 -0700
> John-Mark Gurney <jmg@funkthat.com> wrote:
> 
> > Marko Zec wrote this message on Thu, Jul 16, 2020 at 07:49 +0200:
> > > On Thu, 16 Jul 2020 07:26:22 +0200
> > > Marko Zec <zec@fer.hr> wrote:
> > >   
> > > > On Wed, 15 Jul 2020 16:26:25 -0700
> > > > John-Mark Gurney <jmg@funkthat.com> wrote:
> > > >   
> > > > > I happen to be looking at the implementation of
> > > > > IFQ_DRV_PREPEND, and unlike IFQ_DRV_DEQUEUE, it doesn't obtain
> > > > > the lock when manipulating the ifq.  I took a brief look at a
> > > > > few drivers, and it looks like some of them expect that
> > > > > _PREPEND lock the Q like _DEQUEUE does.
> > > > > 
> > > > > This is likely not an issue often, since it's an error path that
> > > > > likely rarely happens, but we should fix it.
> > > > > 
> > > > > Should we just add the IFQ_LOCK/_UNLOCK to the macro?
> > > > > 
> > > > > Comments or thoughts?    
> > > > 
> > > > I also have a hard time understanding the semantics of IFQ_DRV_*
> > > > macros: per altq(9) they appear to be variants of their IFQ_
> > > > counterparts which are intended to be protected by some other
> > > > mechanism rather than IFQ_LOCK, but in some instances this isn't
> > > > the case, as they do grab the IFQ_LOCK.  
> > > 
> > > Hmm it's not that unclear after looking better: ifq_drv_* parts of
> > > struct ifaltq should be protected by some external mechanisms when
> > > using IFQ_DRV_ macros, and ifq_ parts are still protected by
> > > ifq_mtx. So it doesn't look like IFQ_LOCK/_UNLOCK should be added to
> > > IFQ_DRV_PREPEND()...  
> > 
> > Ahh, I was miss reading the macros on what was protected.  Yes, it
> > does look like the ifq_drv_* fields are supposed to be protected by
> > another lock...
> 
> We were both too fast to conclude that all IFQ_DRV_* are OK, but now
> I'm not sure any more:
> 
> #define IFQ_DRV_IS_EMPTY(ifq) \
>     (((ifq)->ifq_drv_len == 0) && ((ifq)->ifq_len == 0))
> 
> So, if per altq(9) the contract is that with IFQ_DRV_* the ifq_drv_*
> fields should be protected by some caller-provided mechanism, while the
> other ifq_* fields will be implictly protected by ifq_mtx, how can
> accessing ifw_len without holding ifq_mtx in the above example be safe?

Reading is safe when you aren't modifying it, and only using it to
inform if you should recheck w/ a lock...

This way a driver can do:
	if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd)) {
		mtx_lock(sc->sc_mtx);
		for (;;) {
			IFQ_DRV_DEQUEUE(&ifp->if_snd, m);
			if (m == NULL)
				break;
			sendpkt(m);
		}
		mtx_unlock(sc->sc_mtx);
	}

which saves an expensive lock/unlock op when there are no packets
in the queue...

> > I just now found that altq(9) documents this interface... I had
> > assumed it as undocumented after man IFQ_DRV_DEQUEUE didn't return
> > results..
> > 
> > Looks like I REALLY do need to complete my script to make sure that
> > ALL Nm's and related pages have MLINKs.  ALTQ.9 is missing 16 links
> > to the functions it documents...
> > 
> > Just as a reminder to people who write docs, please, make sure you
> > link ALL of the function in your docs...  Otherwise your docs won't
> > be as easy to find..

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200717185609.GX4213>