Re: [PATCH v5 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting

From: Peilin Ye <yepeilin.cs@gmail.com>
To: Vlad Buslov <vladbu@nvidia.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Pedro Tammela <pctammela@mojatatu.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Peilin Ye <peilin.ye@bytedance.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	John Fastabend <john.fastabend@gmail.com>,
	Hillf Danton <hdanton@sina.com>,
	netdev@vger.kernel.org, Cong Wang <cong.wang@bytedance.com>
Subject: Re: [PATCH v5 net 6/6] net/sched: qdisc_destroy() old ingress and clsact Qdiscs before grafting
Date: Sat, 10 Jun 2023 20:25:06 -0700	[thread overview]
Message-ID: <ZIU+krXpJ34WpiwU@C02FL77VMD6R.googleapis.com> (raw)
In-Reply-To: <87ttviwfoy.fsf@nvidia.com>

On Thu, Jun 08, 2023 at 10:48:12AM +0300, Vlad Buslov wrote:
> >> It looks like even though 3f05e6886a59 ("net_sched: unset
> >> TCQ_F_CAN_BYPASS when adding filters") was introduced after cls api
> >> unlock by now we have these in exactly the same list of supported
> >> kernels (5.4 LTS and newer). Considering this, the conversion to the
> >> atomic bitops can be done as a standalone fix for cited commit and after
> >> it will have been accepted and backported the qdisc fix can just assume
> >> that qdisc->flags is an atomic bitops field in all target kernels and
> >> use it as-is. WDYT?
> >
> > Sounds great, how about:
> >
> >   1. I'll post the non-replay version of the fix (after updating the commit
> >      message), and we apply that first, as suggested by Jamal
>
> From my side there are no objections to any of the proposed approaches
> since we have never had any users with legitimate use-case where they
> need to replace/delete a qdisc concurrently with a filter update, so
> returning -EBUSY (or -EAGAIN) to the user in such case would work as
> either temporary or the final fix.

I see, yeah.

> However, Jakub had reservations with such approach so don't know where we
> stand now regarding this.

Either way I'd say applying that non-replay version first is better than
leaving the bug unfixed.  It's been many days since the root cause of the
issue has been figured out.  I'll post it, and start making qdisc->flags
atomic.

> >   2. Make qdisc->flags atomic
> >
> >   3. Make the fix better by replaying and using the (now atomic)
> >      IS-DESTROYING flag with test_and_set_bit() and friends
> >
> > ?
>
> Again, no objections from my side. Ping me if you need help with any of
> these.

Sure, thanks, Vlad!
Peilin Ye