Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vinicius Costa Gomes <vinicius.gomes@intel.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org
Cc:     Jamal Hadi Salim <jhs@mojatatu.com>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Voon Weifeng <weifeng.voon@intel.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Kurt Kanzenbach <kurt@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 net 2/2] net/sched: taprio: make qdisc_leaf() see the
 per-netdev-queue pfifo child qdiscs
In-Reply-To: <20220915100802.2308279-3-vladimir.oltean@nxp.com>
References: <20220915100802.2308279-1-vladimir.oltean@nxp.com>
 <20220915100802.2308279-3-vladimir.oltean@nxp.com>
Date:   Thu, 15 Sep 2022 14:54:14 -0700
Message-ID: <87wna4fgmx.fsf@intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> taprio can only operate as root qdisc, and to that end, there exists the
> following check in taprio_init(), just as in mqprio:
>
> 	if (sch->parent != TC_H_ROOT)
> 		return -EOPNOTSUPP;
>
> And indeed, when we try to attach taprio to an mqprio child, it fails as
> expected:
>
> $ tc qdisc add dev swp0 root handle 1: mqprio num_tc 8 \
> 	map 0 1 2 3 4 5 6 7 \
> 	queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 hw 0
> $ tc qdisc replace dev swp0 parent 1:2 taprio num_tc 8 \
> 	map 0 1 2 3 4 5 6 7 \
> 	queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
> 	base-time 0 sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \
> 	flags 0x0 clockid CLOCK_TAI
> Error: sch_taprio: Can only be attached as root qdisc.
>
> (extack message added by me)
>
> But when we try to attach a taprio child to a taprio root qdisc,
> surprisingly it doesn't fail:
>
> $ tc qdisc replace dev swp0 root handle 1: taprio num_tc 8 \
> 	map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
> 	base-time 0 sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \
> 	flags 0x0 clockid CLOCK_TAI
> $ tc qdisc replace dev swp0 parent 1:2 taprio num_tc 8 \
> 	map 0 1 2 3 4 5 6 7 \
> 	queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \
> 	base-time 0 sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \
> 	flags 0x0 clockid CLOCK_TAI
>
> This is because tc_modify_qdisc() behaves differently when mqprio is
> root, vs when taprio is root.
>
> In the mqprio case, it finds the parent qdisc through
> p = qdisc_lookup(dev, TC_H_MAJ(clid)), and then the child qdisc through
> q = qdisc_leaf(p, clid). This leaf qdisc q has handle 0, so it is
> ignored according to the comment right below ("It may be default qdisc,
> ignore it"). As a result, tc_modify_qdisc() goes through the
> qdisc_create() code path, and this gives taprio_init() a chance to check
> for sch_parent != TC_H_ROOT and error out.
>
> Whereas in the taprio case, the returned q = qdisc_leaf(p, clid) is
> different. It is not the default qdisc created for each netdev queue
> (both taprio and mqprio call qdisc_create_dflt() and keep them in
> a private q->qdiscs[], or priv->qdiscs[], respectively). Instead, taprio
> makes qdisc_leaf() return the _root_ qdisc, aka itself.
>
> When taprio does that, tc_modify_qdisc() goes through the qdisc_change()
> code path, because the qdisc layer never finds out about the child qdisc
> of the root. And through the ->change() ops, taprio has no reason to
> check whether its parent is root or not, just through ->init(), which is
> not called.
>
> The problem is the taprio_leaf() implementation. Even though code wise,
> it does the exact same thing as mqprio_leaf() which it is copied from,
> it works with different input data. This is because mqprio does not
> attach itself (the root) to each device TX queue, but one of the default
> qdiscs from its private array.
>
> In fact, since commit 13511704f8d7 ("net: taprio offload: enforce qdisc
> to netdev queue mapping"), taprio does this too, but just for the full
> offload case. So if we tried to attach a taprio child to a fully
> offloaded taprio root qdisc, it would properly fail too; just not to a
> software root taprio.
>
> To fix the problem, stop looking at the Qdisc that's attached to the TX
> queue, and instead, always return the default qdiscs that we've
> allocated (and to which we privately enqueue and dequeue, in software
> scheduling mode).
>
> Since Qdisc_class_ops :: leaf  is only called from tc_modify_qdisc(),
> the risk of unforeseen side effects introduced by this change is
> minimal.
>
> Fixes: 5a781ccbd19e ("tc: Add support for configuring the taprio scheduler")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

I spent a long time trying to think of cases that this would break,
could not think of anything:


Reviewed-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
-- 
Vinicius