[Cake] Re: [PATCH net] net/sched: Mark qdisc for deletion if graft cannot delete

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Fri, Mar 13, 2026 at 11:56 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Thu, Mar 12, 2026 at 7:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 12 Mar 2026 16:36:48 -0400 Jamal Hadi Salim wrote:
> > > > > Two of the several (I think 4!) patches we had took a similar path. I
> > > > > am trying to remember at least one variant was bad for performance and
> > > > > the other was unstable. Let's see if we can revive it and take a
> > > > > closer look. BTW - none were pretty, it was maybe half the lines of
> > > > > code but touched many things.
> > > >
> > > > FWIW / of course, we have to apply similar change to all(?) callers of
> > > > __tcf_qdisc_find in cls_api. So LOC-wise it may end up also pretty long.
> > > > And it's not going to help the already spaghetti-looking locking. But
> > > > even if it's more LoC I quite like the idea of containing the poopy
> > > > code to where problems originate which is the lockless filter handling.
> > > > Fingers crossed..
> > >
> > > Something like attached.
> > > Unfortunately after running it for a few hours it reproduced.
> > > The action code path (entered by virtue of filter code path execution)
> > > releases the rtnl when attempting to load an action module. A parallel
> > > qdisc operation waiting for the lock then grabs it and we hit the same
> > > issue...
> > >
> > > So now we have to be more invasive and start coordinating the action
> > > code etc, which is not appealing. Thoughts?
> >
> > I see. Doesn't seem entirely crazy to let tcf_proto_lookup_ops()
> > return -EAGAIN without actually loading the module, and have it's
> > call path (of which there are only 2?) do the module loading once
> > all the locks are released. The call paths handle the EAGAIN and
> > retry already they just assume tcf_proto_lookup_ops() has loaded
> > the module so they don't have to.
>
> I might not be entirely following what you are saying. The core issue
> is not module loading perse. We can avoid module loading in
> tcf_proto_lookup_ops while the rtnl lock is released and the qdisc
> refcnt is greater than 1 (which is what the patch i sent attempted).
> The new quark is when we have actions associated with the filter.
>
> I will send you a much lengthier description of the issue in an hour
> or two  to get us on the same page.

Note, i am going to describe the scenario we saw with the action quark
where "filter add" enters before "qdisc del"; the one described in the
commit is the reverse order. We see the issue in both scenarios.

Sorry, this is a lot of text ;->

Let me explain the "expected" behavior
=================================================================

time x:   On cpu1: "filter add" enters increments refcnt to 2 after
          finding the root qdisc (see __tcf_qdisc_find)
time x+1: On cpu1: "filter add" in the action code releases the rtnl_lock
          before calling request_module.
time x+1: On cpu2: "qdisc del" enters for root qdisc, holding rtnl
          (example refcnt = 1), see tc_get_qdisc().
time x+2: On cpu2: "qdisc del" calls graft which calls qdisc_put() which
          decrements refcnt to 1 and cant delete qdisc because refcnt is not 0;
          "qdisc del" exits
          ***We are in an inconsistent state at this point - IOW, the qdisc
          needs to be deleted but cant because of the refcnt !=0.
time x+3: On cpu1: "filter add" acquires the rtnl_lock again in the action
          code (see time x+1).
time x+4: On cpu1: "filter add" eventually completes; see tcf_block_release()
          invocation in tc_new_tfilter() which eventually calls qdisc_put()
          which manages to decrement refcnt to 0. Qdisc is deleted..

To reproduce the issue requires 3 threads - which is what the repro does.
Note: sequence time x to x+2 is the same as "expected" flow above.
===============================================================

time x:   On cpu1: "filter add" enters increments refcnt to 2 after
          finding the root qdisc (see __tcf_qdisc_find)
time x+1: On cpu1: "filter add" in the action code releases the rtnl_lock
          before calling request_module. Note: the action code path will be
          momentarily rescheduled because of request_module call.
time x+1: On cpu2: "qdisc del" enters for root qdisc, holding rtnl
          (example refcnt = 1), see tc_get_qdisc().
time x+2: On cpu2: "qdisc del" calls graft which calls qdisc_put() which
          decrements refcnt to 1 and cant delete qdisc because refcnt is not 0;
          "qdisc del" exits
          ***We are in an inconsistent state at this point- IOW, the qdisc
          needs to be deleted but cant because of the refcnt !=0.
time x+3: On cpu3: "qdisc add" comes in and finds the deleted qdisc still
          on the qdisc hash table since it hasnt been deleted (see inconsitent
          state in time x+2). And then shit hits the fan (as described in the
          commit, qdisc_tree_reduce_backlog() etc).

In this specific example, the issue is that the classifier code path
can't release the rtnl_lock while the qdisc's refcnt is bigger than 1.

Does this make more sense?
The reason we went with the "mark for delete" approach is at time x+3
the "qdisc add" wont be able to find this qdisc. This is the common
observed pattern - for example described in the commit message where
we get have a slightly different flow with "qdisc del" before "filter
add".

cheers,
jamal