Re: [PATCH net v2 3/3] selftests/tc-testing: taprio: add case for the software minimum interval

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests/tc-testing: taprio: add case for the software minimum interval

This adds a tdc case (id f4b2) to tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
which asserts that a pure software taprio schedule with 50000 ns entries is
rejected, since the entries are below TAPRIO_MIN_SW_INTERVAL_NS.

> 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 cd19d05925e40..3fe66839f8aed 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> @@ -154,6 +154,28 @@
>              "echo \"1\" > /sys/bus/netdevsim/del_device"
>          ]
>      },
> +    {
> +        "id": "f4b2",
> +        "name": "Add taprio Qdisc with software interval below the minimum",
> +        "category": [
> +            "qdisc",
> +            "taprio"
> +        ],
> +        "plugins": {
> +            "requires": "nsPlugin"
> +        },
> +        "setup": [
> +            "echo \"1 1 8\" > /sys/bus/netdevsim/new_device"
> +        ],
> +        "cmdUnderTest": "$TC qdisc add dev $ETH root handle 1: taprio num_tc 2 queues 1@0 1@1 sched-entry S 01 50000 sched-entry S 02 50000 clockid CLOCK_TAI",
> +        "expExitCode": "2",

[Severity: Low]
This isn't a bug, but should there also be a companion case that pins the
other half of the rule?  The kernel rule this case targets is conditional in
taprio_min_interval():

net/sched/sch_taprio.c:taprio_min_interval() {
	s64 min_interval = length_to_duration(q, ETH_ZLEN);

	/* Only pure software schedules arm the per-entry hrtimer. */
	if (!FULL_OFFLOAD_IS_ENABLED(q->flags) &&
	    !TXTIME_ASSIST_IS_ENABLED(q->flags))
		min_interval = max_t(s64, min_interval,
				     TAPRIO_MIN_SW_INTERVAL_NS);

	return min_interval;
}

The new f4b2 case only covers the branch being taken.  Reading the rest of
taprio.json, no case appears able to flip that branch: the flags-carrying
cases ba39, 9462, 8d92, d092, 8471, 0a85 and 3e1e all use 300000 ns
intervals with "flags 0x1", and e8a1/6a83 use 20000000 ns with "flags 0x2",
so they pass whether or not the 100 us floor is gated on the flags.

Would that mean a change making the floor unconditional, and thus rejecting
hardware-offloaded or txtime-assist schedules with sub-100 us gates, still
leaves the whole taprio.json suite reporting PASS, including f4b2?

A mirror case using "flags 0x1" with the same 50000 ns entries and
expExitCode 0 would cover the exemption.  On netdevsim this looks feasible:
netdevsim has no get_link_ksettings, so taprio_set_picos_per_byte() falls
back to SPEED_10, giving picos_per_byte 800000 and
length_to_duration(ETH_ZLEN) of 48000 ns, so 50000 ns entries and the
derived 100000 ns cycle are above the unconditional floor.

> +        "verifyCmd": "$TC qdisc show dev $ETH",
> +        "matchPattern": "qdisc taprio 1: root refcnt",
> +        "matchCount": "0",
> +        "teardown": [
> +            "echo \"1\" > /sys/bus/netdevsim/del_device"
> +        ]
> +    },

[ ... ]