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

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

On Tue, Mar 10, 2026 at 9:47 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat,  7 Mar 2026 16:20:58 -0500 Jamal Hadi Salim wrote:
> > Note: We tried a couple of different approaches that had smaller code
> > footprint but were a bit fugly. The first approach was to use recursion
> > on the qdisc hash table to iterate the descendants of the qdisc; however,
> > the challenge here is if the graph depth is "high" - we may overflow the
> > stack. The second approach was to use a breadth first search to achieve
> > the same goal; the challenge here was it was a quadratic algorithm.
>
> Lots of complexity when realistically only ingress/clsact support
> the unlocked operations. Can we not just take rtnl before the
> references and not bother all the real qdiscs with this @#%$ ?
> (diff just to illustrate the point not even compiled)
>

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.

cheers,
jamal

> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 4829c27446e3..21b461f3323d 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -2255,6 +2255,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>         int err;
>         int tp_created;
>         bool rtnl_held = false;
> +       bool rtnl_take = false
>         u32 flags;
>
>  replay:
> @@ -2290,11 +2291,17 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>                 }
>         }
>
> +       /* Realistically only INGRESS supports unlocked ops */
> +       if (parent != TC_H_INGRESS) {
> +               rtnl_held = true;
> +               rtnl_lock();
> +       }
> +
>         /* Find head of filter chain. */
>
>         err = __tcf_qdisc_find(net, &q, &parent, t->tcm_ifindex, false, extack);
>         if (err)
> -               return err;
> +               goto errout;
>
>         if (tcf_proto_check_kind(tca[TCA_KIND], name)) {
>                 NL_SET_ERR_MSG(extack, "Specified TC filter name too long");
> @@ -2306,11 +2313,12 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>          * block is shared (no qdisc found), qdisc is not unlocked, classifier
>          * type is not specified, classifier is not unlocked.
>          */
> -       if (rtnl_held ||
> +       if (rtnl_take ||
>             (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
>             !tcf_proto_is_unlocked(name)) {
> +               if (!rtnl_held)
> +                       rtnl_lock();
>                 rtnl_held = true;
> -               rtnl_lock();
>         }
>
>         err = __tcf_qdisc_cl_find(q, parent, &cl, t->tcm_ifindex, extack);
> @@ -2451,17 +2459,16 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>         }
>         tcf_block_release(q, block, rtnl_held);
>
> -       if (rtnl_held)
> -               rtnl_unlock();
> -
>         if (err == -EAGAIN) {
>                 /* Take rtnl lock in case EAGAIN is caused by concurrent flush
>                  * of target chain.
>                  */
> -               rtnl_held = true;
> +               rtnl_take = true;
>                 /* Replay the request. */
>                 goto replay;
>         }
> +       if (rtnl_held)
> +               rtnl_unlock();
>         return err;
>
>  errout_locked: