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 <[email protected]>
Reviewed-by: Pedro Tammela <[email protected]>
---
v2->v3: none
v1->v2: patch is new
.../tc-testing/tc-tests/qdiscs/taprio.json | 48 +++++++++++++++++++
1 file changed, 48 insertions(+)
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"
+ ]
}
]
--
2.34.1
Vladimir Oltean <[email protected]> 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,
--
Vinicius
Hi Vinicius,
On Wed, Aug 02, 2023 at 04:29:55PM -0700, Vinicius Costa Gomes wrote:
> Vladimir Oltean <[email protected]> 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 <[email protected]>
> Reviewed-by: Pedro Tammela <[email protected]>
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 <[email protected]> writes:
> Hi Vinicius,
>
> On Wed, Aug 02, 2023 at 04:29:55PM -0700, Vinicius Costa Gomes wrote:
>> Vladimir Oltean <[email protected]> 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,
--
Vinicius
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 <[email protected]>
> > Reviewed-by: Pedro Tammela <[email protected]>
> 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/[email protected]/