Re: packet stuck in qdisc : patch proposal

From: Vincent Ray <vray@kalrayinc.com>
To: linyunsheng <linyunsheng@huawei.com>
Cc: davem <davem@davemloft.net>, 方国炬 <guoju.fgj@alibaba-inc.com>,
	kuba <kuba@kernel.org>, netdev <netdev@vger.kernel.org>,
	"Samuel Jones" <sjones@kalrayinc.com>,
	"vladimir oltean" <vladimir.oltean@nxp.com>,
	"Guoju Fang" <gjfang@linux.alibaba.com>,
	"Remy Gauguey" <rgauguey@kalrayinc.com>,
	"Eric Dumazet" <eric.dumazet@gmail.com>,
	will@kernel.org
Subject: Re: packet stuck in qdisc : patch proposal
Date: Tue, 24 May 2022 10:13:53 +0200 (CEST)	[thread overview]
Message-ID: <1758521608.15136543.1653380033771.JavaMail.zimbra@kalray.eu> (raw)
In-Reply-To: <d374b806-1816-574e-ba8b-a750a848a6b3@huawei.com>

> Hi, thanks for the testing and debugging. So the main problem is that
> test_bit() is not an atomic operation, so using smp_mb __*_atomic() is
> not really helping, right?

Yes

> In that case we might only need to change smp_mb__before_atomic() to
> smp_rmb() in qdisc_run_begin(), as we only need to order test_bit()
> after set_bit() and clear_bit(), which is a read/write ordering?

I don't think so : we need to order test_bit() with respect to an earlier enqueue().
So we need a store/load barrier => smp_mb()

> By the way,  Guoju need to ensure ordering between spin_unlock() and
> test_bit() in qdisc_run_end(), which is a write/read ordering, so
> smp_mb() is used.

Agreed

But, for qdisc_run_begin(), I think Eric is right anyway, his fix, using test_and_set_bit() seems much better.
I'm going to try it and will tell you what it gives.

Thanks,

V