| Message ID | 20230801182421.1997560-10-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve the taprio qdisc's relationship with its children | expand |
Vladimir Oltean <vladimir.oltean@nxp.com> writes: > Check that the "Can only be attached as root qdisc" error message from > taprio is effective by attempting to attach it to a class of another > taprio qdisc. That operation should fail. > > In the bug that was squashed by change "net/sched: taprio: try again to > report q->qdiscs[] to qdisc_leaf()", grafting a child taprio to a root > software taprio would be misinterpreted as a change() to the root > taprio. Catch this by looking at whether the base-time of the root > taprio has changed to follow the base-time of the child taprio, > something which should have absolutely never happened assuming correct > semantics. > This test is somehow flaky (all others are fine), 1 in ~4 times, it fails. Taking a look at the test I couldn't quickly find out the reason for the flakyness. Here's the verbose output of one of the failures: vcgomes@otc-cfl-clr-30 ~/src/net-next/tools/testing/selftests/tc-testing $ sudo ./tdc.py -e 39b4 -v -- ns/SubPlugin.__init__ ns/SubPlugin.adjust_command adjust_command: return command [/sbin/ip link add v0p0 type veth peer name v0p1] _exec_cmd: command "/sbin/ip link add v0p0 type veth peer name v0p1" ns/SubPlugin.adjust_command adjust_command: return command [/sbin/ip link set v0p0 up] _exec_cmd: command "/sbin/ip link set v0p0 up" ns/SubPlugin.adjust_command adjust_command: return command [/sbin/ip netns add tcut] _exec_cmd: command "/sbin/ip netns add tcut" ns/SubPlugin.adjust_command adjust_command: return command [/sbin/ip link set v0p1 netns tcut] _exec_cmd: command "/sbin/ip link set v0p1 netns tcut" ns/SubPlugin.adjust_command adjust_command: return command [/sbin/ip -n tcut link set v0p1 up] _exec_cmd: command "/sbin/ip -n tcut link set v0p1 up" ==================== =====> Test 39b4: Reject grafting taprio as child qdisc of software taprio -----> prepare stage ns/SubPlugin.adjust_command adjust_command: stage is setup; inserting netns stuff in command [echo "1 1 8" > /sys/bus/netdevsim/new_device] list [['echo', '"1', '1', '8"', '>', '/sys/bus/netdevsim/new_device']] adjust_command: return command [/sbin/ip netns exec tcut echo "1 1 8" > /sys/bus/netdevsim/new_device] command "/sbin/ip netns exec tcut echo "1 1 8" > /sys/bus/netdevsim/new_device" ns/SubPlugin.adjust_command adjust_command: stage is setup; inserting netns stuff in command [/sbin/tc qdisc replace dev eth0 handle 8001: parent root stab overhead 24 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 ff 20000000 clockid CLOCK_TAI] list [['/sbin/tc', 'qdisc', 'replace', 'dev', 'eth0', 'handle', '8001:', 'parent', 'root', 'stab', 'overhead', '24', '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', 'ff', '20000000', 'clockid', 'CLOCK_TAI']] adjust_command: return command [/sbin/ip netns exec tcut /sbin/tc qdisc replace dev eth0 handle 8001: parent root stab overhead 24 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 ff 20000000 clockid CLOCK_TAI] command "/sbin/ip netns exec tcut /sbin/tc qdisc replace dev eth0 handle 8001: parent root stab overhead 24 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 ff 20000000 clockid CLOCK_TAI" -----> execute stage ns/SubPlugin.adjust_command adjust_command: stage is execute; inserting netns stuff in command [/sbin/tc qdisc replace dev eth0 parent 8001:7 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 200 sched-entry S ff 20000000 clockid CLOCK_TAI] list [['/sbin/tc', 'qdisc', 'replace', 'dev', 'eth0', 'parent', '8001:7', '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', '200', 'sched-entry', 'S', 'ff', '20000000', 'clockid', 'CLOCK_TAI']] adjust_command: return command [/sbin/ip netns exec tcut /sbin/tc qdisc replace dev eth0 parent 8001:7 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 200 sched-entry S ff 20000000 clockid CLOCK_TAI] command "/sbin/ip netns exec tcut /sbin/tc qdisc replace dev eth0 parent 8001:7 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 200 sched-entry S ff 20000000 clockid CLOCK_TAI" -----> verify stage ns/SubPlugin.adjust_command adjust_command: stage is verify; inserting netns stuff in command [/sbin/tc -j qdisc show dev eth0 root | jq '.[].options.base_time'] list [['/sbin/tc', '-j', 'qdisc', 'show', 'dev', 'eth0', 'root', '|', 'jq', "'.[].options.base_time'"]] adjust_command: return command [/sbin/ip netns exec tcut /sbin/tc -j qdisc show dev eth0 root | jq '.[].options.base_time'] command "/sbin/ip netns exec tcut /sbin/tc -j qdisc show dev eth0 root | jq '.[].options.base_time'" -----> teardown stage ns/SubPlugin.adjust_command adjust_command: stage is teardown; inserting netns stuff in command [/sbin/tc qdisc del dev eth0 root] list [['/sbin/tc', 'qdisc', 'del', 'dev', 'eth0', 'root']] adjust_command: return command [/sbin/ip netns exec tcut /sbin/tc qdisc del dev eth0 root] command "/sbin/ip netns exec tcut /sbin/tc qdisc del dev eth0 root" ns/SubPlugin.adjust_command adjust_command: stage is teardown; inserting netns stuff in command [echo "1" > /sys/bus/netdevsim/del_device] list [['echo', '"1"', '>', '/sys/bus/netdevsim/del_device']] adjust_command: return command [/sbin/ip netns exec tcut echo "1" > /sys/bus/netdevsim/del_device] command "/sbin/ip netns exec tcut echo "1" > /sys/bus/netdevsim/del_device" ns/SubPlugin.post_suite ns/SubPlugin.adjust_command adjust_command: return command [/sbin/ip netns delete tcut] _exec_cmd: command "/sbin/ip netns delete tcut" All test results: 1..1 not ok 1 39b4 - Reject grafting taprio as child qdisc of software taprio Could not match regex pattern. Verify command output: parse error: Objects must consist of key:value pairs at line 1, column 334 Cheers,
Hi Vinicius, On Wed, Aug 02, 2023 at 04:29:55PM -0700, Vinicius Costa Gomes wrote: > Vladimir Oltean <vladimir.oltean@nxp.com> writes: > This test is somehow flaky (all others are fine), 1 in ~4 times, it fails. > > Taking a look at the test I couldn't quickly find out the reason for the > flakyness. > > Here's the verbose output of one of the failures: > > vcgomes@otc-cfl-clr-30 ~/src/net-next/tools/testing/selftests/tc-testing $ sudo ./tdc.py -e 39b4 -v > All test results: > > 1..1 > not ok 1 39b4 - Reject grafting taprio as child qdisc of software taprio > Could not match regex pattern. Verify command output: > parse error: Objects must consist of key:value pairs at line 1, column 334 Interesting. I'm not seeing this, and I re-ran it a few times. The error message seems to come from jq, as if it's not able to parse something. Sorry, I only have caveman debugging techniques. Could you remove the pipe into jq and rerun a few times, see what it prints when it fails? diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json index de51408544e2..bb6be1f78e31 100644 --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json @@ -148,8 +148,8 @@ ], "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 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 200 sched-entry S ff 20000000 clockid CLOCK_TAI", "expExitCode": "2", - "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'", - "matchPattern": "0", + "verifyCmd": "$TC -j qdisc show dev $ETH root", + "matchPattern": "\\[{\"kind\":\"taprio\",\"handle\":\"8001:\",\"root\":true,\"refcnt\":9,\"options\":{\"tc\":0,\"map\":\\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\\],\"queues\":\\[\\],\"clockid\":\"TAI\",\"base_time\":0,\"cycle_time\":20000000,\"cycle_time_extension\":0,\"schedule\":\\[{\"index\":0,\"cmd\":\"S\",\"gatemask\":\"0xff\",\"interval\":20000000}\\],\"max-sdu\":\\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\\],\"fp\":\\[\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\"\\]}}\\]", "matchCount": "1", "teardown": [ "$TC qdisc del dev $ETH root",
On 01/08/2023 15:24, Vladimir Oltean wrote: > Check that the "Can only be attached as root qdisc" error message from > taprio is effective by attempting to attach it to a class of another > taprio qdisc. That operation should fail. > > In the bug that was squashed by change "net/sched: taprio: try again to > report q->qdiscs[] to qdisc_leaf()", grafting a child taprio to a root > software taprio would be misinterpreted as a change() to the root > taprio. Catch this by looking at whether the base-time of the root > taprio has changed to follow the base-time of the child taprio, > something which should have absolutely never happened assuming correct > semantics. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Pedro Tammela <pctammela@mojatatu.com> If I understood correctly, these tests depend on CONFIG_PTP_1588_CLOCK_MOCK. If that is the case, you should add it to the tdc config file (tools/testing/selftests/tc-testing/config). cheers, Victor
Vladimir Oltean <vladimir.oltean@nxp.com> writes: > Hi Vinicius, > > On Wed, Aug 02, 2023 at 04:29:55PM -0700, Vinicius Costa Gomes wrote: >> Vladimir Oltean <vladimir.oltean@nxp.com> writes: >> This test is somehow flaky (all others are fine), 1 in ~4 times, it fails. >> >> Taking a look at the test I couldn't quickly find out the reason for the >> flakyness. >> >> Here's the verbose output of one of the failures: >> >> vcgomes@otc-cfl-clr-30 ~/src/net-next/tools/testing/selftests/tc-testing $ sudo ./tdc.py -e 39b4 -v >> All test results: >> >> 1..1 >> not ok 1 39b4 - Reject grafting taprio as child qdisc of software taprio >> Could not match regex pattern. Verify command output: >> parse error: Objects must consist of key:value pairs at line 1, column 334 > > Interesting. I'm not seeing this, and I re-ran it a few times. The error > message seems to come from jq, as if it's not able to parse something. > > Sorry, I only have caveman debugging techniques. Could you remove the > pipe into jq and rerun a few times, see what it prints when it fails? > > diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json > index de51408544e2..bb6be1f78e31 100644 > --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json > +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json > @@ -148,8 +148,8 @@ > ], > "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 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 200 sched-entry S ff 20000000 clockid CLOCK_TAI", > "expExitCode": "2", > - "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'", > - "matchPattern": "0", > + "verifyCmd": "$TC -j qdisc show dev $ETH root", > + "matchPattern": "\\[{\"kind\":\"taprio\",\"handle\":\"8001:\",\"root\":true,\"refcnt\":9,\"options\":{\"tc\":0,\"map\":\\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\\],\"queues\":\\[\\],\"clockid\":\"TAI\",\"base_time\":0,\"cycle_time\":20000000,\"cycle_time_extension\":0,\"schedule\":\\[{\"index\":0,\"cmd\":\"S\",\"gatemask\":\"0xff\",\"interval\":20000000}\\],\"max-sdu\":\\[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0\\],\"fp\":\\[\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\",\"E\"\\]}}\\]", > "matchCount": "1", > "teardown": [ > "$TC qdisc del dev $ETH root", Hmmm, I think that this test discovered another bug (perhaps even two). When it fails here's the json I get (edited for clarity): [{"kind":"taprio","handle":"8001:","root":true,"refcnt":9, "options":{ "tc":0, "map":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0], "queues":[], "clockid":"TAI", "base_time":0, "cycle_time":0, "cycle_time_extension":0, { "base_time":0, "cycle_time":20000000, "cycle_time_extension":0, "schedule":[{"index":0,"cmd":"S","gatemask":"0xff","interval":20000000}] }}}] Thinking out loud: If I am reading this right, there's no "oper" schedule, only an "admin" schedule. So the first bug is probably a taprio bug when deciding if it should create an "open" vs. "admin" schedule. The second bug seems to be in the way that q_taprio in iproute2 handles the admin schedule, is just an object inside another, which seems to be invalid. Does it make sense? Cheers,
On Thu, Aug 03, 2023 at 02:21:07PM -0300, Victor Nogueira wrote: > On 01/08/2023 15:24, Vladimir Oltean wrote: > > Check that the "Can only be attached as root qdisc" error message from > > taprio is effective by attempting to attach it to a class of another > > taprio qdisc. That operation should fail. > > > > In the bug that was squashed by change "net/sched: taprio: try again to > > report q->qdiscs[] to qdisc_leaf()", grafting a child taprio to a root > > software taprio would be misinterpreted as a change() to the root > > taprio. Catch this by looking at whether the base-time of the root > > taprio has changed to follow the base-time of the child taprio, > > something which should have absolutely never happened assuming correct > > semantics. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Reviewed-by: Pedro Tammela <pctammela@mojatatu.com> > If I understood correctly, these tests depend on CONFIG_PTP_1588_CLOCK_MOCK. > If that is the case, you should add it to the tdc > config file (tools/testing/selftests/tc-testing/config). Thanks, will do.
On Thu, Aug 03, 2023 at 11:43:16AM -0700, Vinicius Costa Gomes wrote: > Hmmm, I think that this test discovered another bug (perhaps even two). > When it fails here's the json I get (edited for clarity): > > [{"kind":"taprio","handle":"8001:","root":true,"refcnt":9, > "options":{ > "tc":0, > "map":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0], > "queues":[], > "clockid":"TAI", > "base_time":0, > "cycle_time":0, > "cycle_time_extension":0, > { > "base_time":0, > "cycle_time":20000000, > "cycle_time_extension":0, > "schedule":[{"index":0,"cmd":"S","gatemask":"0xff","interval":20000000}] > }}}] > > Thinking out loud: If I am reading this right, there's no "oper" > schedule, only an "admin" schedule. So the first bug is probably a > taprio bug when deciding if it should create an "open" vs. "admin" > schedule. > > The second bug seems to be in the way that q_taprio in iproute2 > handles the admin schedule, is just an object inside another, which > seems to be invalid. > > Does it make sense? Yes, it makes sense, thanks. I've sent some iproute2 patches that fix the user space issues, and I'll soon send a v4 which takes into consideration the fact that the admin schedule may not become operational right away. https://patchwork.kernel.org/project/netdevbpf/patch/20230807160827.4087483-1-vladimir.oltean@nxp.com/
diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json index 68a7264e083d..8dbed66a9acc 100644 --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json @@ -131,5 +131,53 @@ "teardown": [ "echo \"1\" > /sys/bus/netdevsim/del_device" ] + }, + { + "id": "39b4", + "name": "Reject grafting taprio as child qdisc of software taprio", + "category": [ + "qdisc", + "taprio" + ], + "plugins": { + "requires": "nsPlugin" + }, + "setup": [ + "echo \"1 1 8\" > /sys/bus/netdevsim/new_device", + "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 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 ff 20000000 clockid CLOCK_TAI" + ], + "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 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 200 sched-entry S ff 20000000 clockid CLOCK_TAI", + "expExitCode": "2", + "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'", + "matchPattern": "0", + "matchCount": "1", + "teardown": [ + "$TC qdisc del dev $ETH root", + "echo \"1\" > /sys/bus/netdevsim/del_device" + ] + }, + { + "id": "e8a1", + "name": "Reject grafting taprio as child qdisc of offloaded taprio", + "category": [ + "qdisc", + "taprio" + ], + "plugins": { + "requires": "nsPlugin" + }, + "setup": [ + "echo \"1 1 8\" > /sys/bus/netdevsim/new_device", + "$TC qdisc replace dev $ETH handle 8001: parent root stab overhead 24 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 ff 20000000 flags 0x2" + ], + "cmdUnderTest": "$TC qdisc replace dev $ETH parent 8001:7 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 200 sched-entry S ff 20000000 flags 0x2", + "expExitCode": "2", + "verifyCmd": "$TC -j qdisc show dev $ETH root | jq '.[].options.base_time'", + "matchPattern": "0", + "matchCount": "1", + "teardown": [ + "$TC qdisc del dev $ETH root", + "echo \"1\" > /sys/bus/netdevsim/del_device" + ] } ]