Date: Sat, 18 Jul 2020 10:52:43 -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: <20200718175243.GZ4213@funkthat.com> In-Reply-To: <20200717235438.1fee733b@x23> References: <20200715232624.GR4213@funkthat.com> <20200716072622.5fa35ba2@x23> <20200716074917.04445daa@x23> <20200716185629.GT4213@funkthat.com> <20200717120311.59377e0d@x23> <20200717185609.GX4213@funkthat.com> <20200717235438.1fee733b@x23>
next in thread | previous in thread | raw e-mail | index | archive | help
Marko Zec wrote this message on Fri, Jul 17, 2020 at 23:54 +0200:
> On Fri, 17 Jul 2020 11:56:09 -0700
> John-Mark Gurney <jmg@funkthat.com> wrote:
> > Marko Zec wrote this message on Fri, Jul 17, 2020 at 12:03 +0200:
> ...
> > > #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...
>
> The above snippet is fine even if IFQ_DRV_IS_EMPTY() returns 0 during a
> race with another thread: per altq(9) the subsequent (then properly
> locked) IFQ_DRV_DEQUEUE() might find out that the queue is actually
> empty, and bail out if (m == NULL).
>
> But what if IFQ_DRV_IS_EMPTY() returns 1 due to a race with another
> thread which has just executed IFQ_ENQUEUE() (invisible to the first
> thread due to lack of synchronization), and therefore leaves the mbuf in
> the queue, instead of dequeuing and processing it?
This is where the _OACTIVE flag comes into play.. IFQ_HANDOFF_ADJ
checks the flag, and calls if_start if it's not set.. (hmm, do we
need a barrier here to ensure that enqueue completes before we check
the _OACTIVE flag, as this check of the flag is unlocked?) As long
the driver has not set the _OACTIVE flag, the if_start routine will
be called for the enqueued packet...
It's only a problem if the driver does an unlocked check for the queue
and the driver has set the _OACTIVE flag, AND the driver will not do
a future check of the queue, like it's suppose to when it finishes
a packet TX and clears the _OACTIVE flag...
--
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?20200718175243.GZ4213>