• Vladimir Oltean's avatar
    net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs · 1461d212
    Vladimir Oltean authored
    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 13511704 ("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: 5a781ccb ("tc: Add support for configuring the taprio scheduler")
    Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
    Reviewed-by: default avatarVinicius Costa Gomes <vinicius.gomes@intel.com>
    Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
    1461d212
sch_taprio.c 52 KB