Re: [PATCH net v2 2/3] net/sched: taprio: enforce a minimum interval for software schedules

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: taprio: enforce a minimum interval for software schedules

This adds taprio_min_interval() which raises the accepted minimum
per-entry interval to 100us for schedules that are neither fully
offloaded nor txtime-assisted, on the grounds that each entry expiry
costs an hrtimer callback. The same minimum is applied to the
cycle_time sanity check.

Link: https://lore.kernel.org/all/afe041f6-ef7d-4434-b2d0-096be49b5bcb@mail.kernel.org/ (local)

[Severity: High]
Does this narrow the range of schedules that userspace can install on
existing setups?

Before this patch, fill_sched_entry() only required
length_to_duration(q, ETH_ZLEN). At 1 Gb/s picos_per_byte is 8000, so the
bound was 60 * 8 = 480ns; at 100 Mb/s it was about 4.8us. With
taprio_min_interval() the bound for software mode becomes a flat 100000ns,
and parse_taprio_schedule() additionally requires:

	if (new->cycle_time < (s64)new->num_entries * taprio_min_interval(q)) {
		NL_SET_ERR_MSG(extack, "'cycle_time' is too small");
		return -EINVAL;
	}

So a 250us cycle split into 4 x 62.5us windows, or a 1ms cycle containing a
20us guard window, is now rejected with -EINVAL by tc qdisc add/replace on a
real NIC in software mode. Any 8-entry schedule with a cycle below 800us is
rejected as well.

The commit message says the hrtimer expiry costs "on the order of 10us on a
debug configuration and about a microsecond on a release build", yet the
constant is 100us for all configurations. Is a 100x margin over the
release-build cost intended as a hard limit on PREEMPT_RT and production
kernels, where sub-100us gating on isolated CPUs is the reason software
taprio exists?

Since the change carries a Fixes: tag it will be picked up for stable, so
working 802.1Qbv configurations would stop loading after a stable update.
Would it make sense to make the floor tunable (module parameter, sysctl or
netlink attribute) and document it, rather than a compile-time constant?

Was flooring the rearm interval at runtime in advance_sched() considered as
an alternative, so that the schedule is still accepted but the timer is not
rearmed in the past? As a mitigation for the reported stalls the constant is
also only a constant factor: a CAP_NET_ADMIN user in a netns can still
create many veth or bonding devices, each running a 100us cycle.

[Severity: Low]
This isn't a bug, but the comment and the extack string further down in
fill_sched_entry() no longer describe what is enforced:

	/* The interval should allow at least the minimum ethernet
	 * frame to go out.
	 */
	if (interval < min_duration) {
		NL_SET_ERR_MSG(extack, "Invalid interval for schedule entry");
		return -EINVAL;
	}

For any link at 1 Gb/s or faster the ETH_ZLEN term is a few hundred
nanoseconds, so the frame-length rule the comment names never wins and the
rule actually applied is the 100us hrtimer-cost floor documented only at the
taprio_min_interval() definition. Could the comment be updated, and could
the extack message mention the minimum so a user whose 5us interval is
rejected after an upgrade can tell why?