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

[Vlad Buslov <vladbu@nvidia.com>]

On Wed 07 Jun 2023 at 18:08, Peilin Ye <yepeilin.cs@gmail.com> wrote:
> On Wed, Jun 07, 2023 at 11:18:32AM +0300, Vlad Buslov wrote:
>> > I also thought about adding the new DELETED-REJECT-NEW-FILTERS flag to
>> > ::state2, but not sure if it's okay to extend it for our purpose.
>>
>> As you described above qdisc->flags is already used to interact with cls
>> api (including changing it dynamically), so I don't see why not.
>
> Sorry, I don't follow, I meant qdisc->state2:
>
>   enum qdisc_state2_t {
>           /* Only for !TCQ_F_NOLOCK qdisc. Never access it directly.
>            * Use qdisc_run_begin/end() or qdisc_is_running() instead.
>            */
>           __QDISC_STATE2_RUNNING,
>   };

Sorry, I misunderstood what you were suggesting. Got it now.

>
> NVM, I think using qdisc->flags after making it atomic sounds better.

Agree.

>
> On Wed, Jun 07, 2023 at 11:18:32AM +0300, Vlad Buslov wrote:
>> > 	err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
>> > 			      flags, extack);
>> > 	if (err == 0) {
>> > 		tfilter_notify(net, skb, n, tp, block, q, parent, fh,
>> > 			       RTM_NEWTFILTER, false, rtnl_held, extack);
>> > 		tfilter_put(tp, fh);
>> > 		/* q pointer is NULL for shared blocks */
>> > 		if (q)
>> > 			q->flags &= ~TCQ_F_CAN_BYPASS;
>> > 	}               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >
>> > TCQ_F_CAN_BYPASS is cleared after e.g. adding a filter to the Qdisc, and it
>> > isn't atomic [1].
>> 
>> Yeah, I see we have already got such behavior in 3f05e6886a59
>> ("net_sched: unset TCQ_F_CAN_BYPASS when adding filters").
>> 
>> > We also have this:
>> >
>> >   ->dequeue()
>> >     htb_dequeue()
>> >       htb_dequeue_tree()
>> >         qdisc_warn_nonwc():
>> >
>> >   void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc)
>> >   {
>> >           if (!(qdisc->flags & TCQ_F_WARN_NONWC)) {
>> >                   pr_warn("%s: %s qdisc %X: is non-work-conserving?\n",
>> >                           txt, qdisc->ops->id, qdisc->handle >> 16);
>> >                   qdisc->flags |= TCQ_F_WARN_NONWC;
>> >           }       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >   }
>> >   EXPORT_SYMBOL(qdisc_warn_nonwc);
>> >
>> > Also non-atomic; isn't it possible for the above 2 underlined statements to
>> > race with each other?  If true, I think we need to change Qdisc::flags to
>> > use atomic bitops, just like what we're doing for Qdisc::state and
>> > ::state2.  It feels like a separate TODO, however.
>> 
>> 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. However, Jakub had reservations with
such approach so don't know where we stand now regarding this.

>
>   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.