[PATCH net v2 0/3] net/sched: taprio: fix software schedule livelocks

[PATCH net v2 0/3] net/sched: taprio: fix software schedule livelocks

[Junjie Cao]

advance_sched() livelocks the owning CPU in two independent ways:
schedules with entry intervals below the cost of servicing one hrtimer
expiry pass validation, because virtual devices inflate the link speed
behind the frame-length minimum, and a valid schedule that falls
behind - delayed timer, starved CPU, stepped clock - replays its whole
backlog one entry per expiry from hrtimer context.

Neither fix covers the other case. With only bounded catch-up, a 700ns
single-entry schedule on veth is still admitted and sustains ~1M timer
irqs/s on a release build. With only the interval floor, a stepped
clock still replays the backlog. Clamping the next expiry into the
future at runtime, as tested on one of the reproducer buckets in 2025
[1], keeps the stall detector quiet but leaves the sub-microsecond
schedule admitted, the CPU servicing an expiry every few microseconds
for the lifetime of the qdisc, and the gates drifting off the
configured timeline with every clamped expiry.

Patch 2 extends the patch generated by syzkaller's patching workflow
[2] to exempt txtime-assist, which never arms the per-entry hrtimer.
syzbot tested the series against the reproducers of all three known
buckets on net.git; tags on patch 2.

Changes in v2:
- advance_sched() takes now from the timer's clock base rather than
  re-deriving it through taprio_get_time() (Hillf Danton).

v1: https://lore.kernel.org/all/20260818071706.251035-1-junjie.cao@intel.com/

[1] https://lore.kernel.org/all/20250729010657.3326-1-hdanton@sina.com/
[2] https://lore.kernel.org/all/afe041f6-ef7d-4434-b2d0-096be49b5bcb@mail.kernel.org/

Junjie Cao (2):
  net/sched: taprio: catch up in bounded time when the schedule falls
    behind
  selftests/tc-testing: taprio: add case for the software minimum
    interval

Uladzislau Zhauniarovich (1):
  net/sched: taprio: enforce a minimum interval for software schedules

 net/sched/sch_taprio.c                        | 80 +++++++++++++++++--
 .../tc-testing/tc-tests/qdiscs/taprio.json    | 22 +++++
 2 files changed, 97 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH net v2 1/3] net/sched: taprio: catch up in bounded time when the schedule falls behind

[Junjie Cao]

advance_sched() advances exactly one entry per hrtimer expiry. When the
operational schedule falls behind - the timer was delayed, the CPU was
starved, or the reference clock stepped forward - every elapsed entry
is replayed back to back from hrtimer context with current_entry_lock
held, and each replay rearms the timer with an expiry in the past. Once
the backlog is large enough the CPU never leaves timer processing and
RCU stalls follow. syzbot triggers this with schedules whose intervals
are shorter than the cost of servicing one expiry, so the backlog only
ever grows.

Skip complete cycles arithmetically and walk at most one cycle of
entries to land on the entry covering the current time. Gate close
times and budgets are still only computed for the entry landed on. An
admin schedule crossed by the jump is picked up by the existing
should_change_schedules() check on the recomputed end time. The walk is
capped at twice the entry count as a safeguard against degenerate
intervals; leftover backlog is then handled by the next expiry as
today.

Fixes: 5a781ccbd19e ("tc: Add support for configuring the taprio scheduler")
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 net/sched/sch_taprio.c | 56 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 299234a5f0fe..0d566c934b2f 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -915,6 +915,51 @@ static bool should_change_schedules(const struct sched_gate_list *admin,
 	return false;
 }
 
+/* The operational schedule fell behind, e.g. because the timer was delayed
+ * or the reference clock stepped forward. Advancing one entry per timer
+ * expiry would replay the whole backlog from hrtimer context, so skip
+ * complete cycles arithmetically and walk the remaining entries to land on
+ * the entry covering the current time.
+ */
+static void taprio_catch_up(struct sched_gate_list *oper,
+			    struct sched_entry **next, ktime_t *next_start,
+			    ktime_t *end_time, ktime_t now)
+{
+	int budget = 2 * oper->num_entries + 1;
+	struct sched_entry *entry = *next;
+	ktime_t start = *next_start;
+	ktime_t end = *end_time;
+	s64 behind = ktime_sub(now, end);
+
+	if (oper->cycle_time > 0 && behind >= oper->cycle_time) {
+		s64 jump = div64_s64(behind, oper->cycle_time) * oper->cycle_time;
+
+		start = ktime_add_ns(start, jump);
+		end = ktime_add_ns(end, jump);
+		oper->cycle_end_time = ktime_add_ns(oper->cycle_end_time, jump);
+	}
+
+	while (ktime_before(end, now) && --budget) {
+		if (list_is_last(&entry->list, &oper->entries) ||
+		    ktime_compare(end, oper->cycle_end_time) == 0) {
+			entry = list_first_entry(&oper->entries,
+						 struct sched_entry, list);
+			oper->cycle_end_time = ktime_add_ns(oper->cycle_end_time,
+							    oper->cycle_time);
+		} else {
+			entry = list_next_entry(entry, list);
+		}
+
+		start = end;
+		end = ktime_add_ns(end, entry->interval);
+		end = min_t(ktime_t, end, oper->cycle_end_time);
+	}
+
+	*next = entry;
+	*next_start = start;
+	*end_time = end;
+}
+
 static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 {
 	struct taprio_sched *q = container_of(timer, struct taprio_sched,
@@ -924,7 +969,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 	int num_tc = netdev_get_num_tc(dev);
 	struct sched_entry *entry, *next;
 	struct Qdisc *sch = q->root;
-	ktime_t end_time;
+	ktime_t end_time, next_start, now;
 	int tc;
 
 	spin_lock(&q->current_entry_lock);
@@ -960,14 +1005,19 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 		next = list_next_entry(entry, list);
 	}
 
-	end_time = ktime_add_ns(entry->end_time, next->interval);
+	next_start = entry->end_time;
+	end_time = ktime_add_ns(next_start, next->interval);
 	end_time = min_t(ktime_t, end_time, oper->cycle_end_time);
 
+	now = hrtimer_cb_get_time(timer);
+	if (unlikely(ktime_before(end_time, now)))
+		taprio_catch_up(oper, &next, &next_start, &end_time, now);
+
 	for (tc = 0; tc < num_tc; tc++) {
 		if (next->gate_duration[tc] == oper->cycle_time)
 			next->gate_close_time[tc] = KTIME_MAX;
 		else
-			next->gate_close_time[tc] = ktime_add_ns(entry->end_time,
+			next->gate_close_time[tc] = ktime_add_ns(next_start,
 								 next->gate_duration[tc]);
 	}
 
-- 
2.43.0

Re: [PATCH net v2 1/3] net/sched: taprio: catch up in bounded time when the schedule falls behind

[Jakub Kicinski]

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: catch up in bounded time when the schedule falls behind

This adds taprio_catch_up(), which skips whole cycles arithmetically and
then walks at most a bounded number of entries so that advance_sched()
lands on the entry covering the current time instead of replaying every
elapsed entry from hrtimer context. The walk is capped at twice the entry
count and the caller uses the resulting entry, start time and end time for
gate close times, budgets and the next timer expiry.

No Link: tags present in the commit header.

> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 299234a5f0fe6..0d566c934b2f3 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -915,6 +915,51 @@ static bool should_change_schedules(const struct sched_gate_list *admin,
>  	return false;
>  }
>  
> +/* The operational schedule fell behind, e.g. because the timer was delayed
> + * or the reference clock stepped forward. Advancing one entry per timer
> + * expiry would replay the whole backlog from hrtimer context, so skip
> + * complete cycles arithmetically and walk the remaining entries to land on
> + * the entry covering the current time.
> + */

[Severity: Low]
Does this comment promise more than the function delivers?

The wording "walk the remaining entries to land on the entry covering the
current time" reads as an unconditional postcondition, but the loop below
has a second exit condition (--budget), so taprio_catch_up() can return
with *end_time still before now and *next pointing at an entry whose
interval already elapsed.

There is also the case where behind is an exact multiple of cycle_time: the
jump makes end == now, and the strict ktime_before(end, now) test then skips
the walk entirely, so the returned entry is the one whose interval just
ended rather than the one covering the new interval.

The budget cap and the "leftover backlog is then handled by the next expiry"
behaviour appear only in the changelog. Could both be mentioned in the
comment so a reader of the source sees the same contract?

> +static void taprio_catch_up(struct sched_gate_list *oper,
> +			    struct sched_entry **next, ktime_t *next_start,
> +			    ktime_t *end_time, ktime_t now)
> +{
> +	int budget = 2 * oper->num_entries + 1;

[Severity: Low]
How long can this walk hold current_entry_lock in hardirq context?

advance_sched() takes the lock with plain spin_lock() and the advance timer
is set up with HRTIMER_MODE_ABS (no HRTIMER_MODE_SOFT), so the callback runs
in hardirq context. num_entries comes straight from the nested
TCA_TAPRIO_SCHED_ENTRY list and parse_sched_list() only rejects the empty
case, so a large gate control list turns a previously O(1) critical section
into an O(num_entries) walk over separately allocated entries.

Meanwhile taprio_change() waits on the same lock:

	/* Protects against advance_sched() */
	spin_lock_irqsave(&q->current_entry_lock, flags);

so it spins with interrupts disabled for the whole duration of the walk on
the other CPU. The total work is not worse than before the patch, but it is
now concentrated in one lock hold.

> +	struct sched_entry *entry = *next;
> +	ktime_t start = *next_start;
> +	ktime_t end = *end_time;
> +	s64 behind = ktime_sub(now, end);
> +
> +	if (oper->cycle_time > 0 && behind >= oper->cycle_time) {
> +		s64 jump = div64_s64(behind, oper->cycle_time) * oper->cycle_time;
> +
> +		start = ktime_add_ns(start, jump);
> +		end = ktime_add_ns(end, jump);
> +		oper->cycle_end_time = ktime_add_ns(oper->cycle_end_time, jump);
> +	}

[Severity: Medium]
Can this jump shift the gate timeline to a phase the schedule never
occupies?

The jump adds a multiple of cycle_time to start, end and cycle_end_time
while leaving entry untouched, which assumes the entry sequence repeats with
period cycle_time. That holds when the sum of the intervals is at least
cycle_time, because the min_t(..., cycle_end_time) clamp forces each lap to
end on a cycle boundary.

When the sum of the intervals is smaller than cycle_time, the existing
progression in advance_sched() restarts the list as soon as the tail entry
ends:

net/sched/sch_taprio.c:should_restart_cycle() {
	if (list_is_last(&entry->list, &oper->entries))
		return true;
	...
}

and the next end is derived from the previous entry's end_time:

	next_start = entry->end_time;
	end_time = ktime_add_ns(next_start, next->interval);
	end_time = min_t(ktime_t, end_time, oper->cycle_end_time);

so entries run back to back with an effective period of the interval sum
while cycle_end_time advances a full cycle_time per lap, making the clamp
inoperative. Adding N * cycle_time then shifts the timeline by
(N * cycle_time) modulo the interval sum, and the gate_close_time[] values
derived from next_start plus the budgets from taprio_set_budgets() stay
offset from base_time alignment for all later cycles.

Such a schedule is accepted by the ABI, since parse_taprio_schedule() only
checks a lower bound:

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

For example two 100 us entries with cycle-time 2000000000 passes.

> +
> +	while (ktime_before(end, now) && --budget) {

[Severity: Medium]
Can this loop exit with end still before now, leaving the timer armed in the
past?

The arithmetic branch above only removes whole multiples of cycle_time
(behind >= oper->cycle_time), so a residual in [0, cycle_time) is left over.
The walk then advances the timeline by at most twice the sum of the entry
intervals before --budget reaches zero.

For a schedule where cycle_time is larger than twice the interval sum, for
example:

	sched-entry S 01 100000
	sched-entry S 02 100000
	cycle-time 2000000000

each pass of the walk buys only the interval sum of schedule time, because
cycle_end_time runs ahead of end by (cycle_time - interval sum) per lap and
the min_t(ktime_t, end, oper->cycle_end_time) clamp never engages. The loop
then terminates on budget exhaustion with end < now, and behind stays below
cycle_time on the following expiries so the arithmetic jump never re-engages.

advance_sched() consumes the result unconditionally:

	next->end_time = end_time;
	taprio_set_budgets(q, oper, next);
	...
	hrtimer_set_expires(&q->advance_timer, end_time);

so the timer is rearmed with an expiry in the past and the callback fires
again immediately, with every gate_close_time[] also in the past so
taprio_dequeue_from_txq() transmits nothing. With the cycle_time policy cap
of INT_MAX that is up to a few thousand back-to-back expiries per
fall-behind event.

With this patch alone, before the later change that enforces a minimum
interval for software schedules, intervals down to roughly 48 ns are still
accepted on virtual devices, and then twice the interval sum is smaller than
the cost of servicing one expiry, so for gap schedules the storm the
changelog describes does not converge at all. Is the intent that this patch
depends on the minimum interval patch for the bound to hold?

> +		if (list_is_last(&entry->list, &oper->entries) ||
> +		    ktime_compare(end, oper->cycle_end_time) == 0) {
> +			entry = list_first_entry(&oper->entries,
> +						 struct sched_entry, list);
> +			oper->cycle_end_time = ktime_add_ns(oper->cycle_end_time,
> +							    oper->cycle_time);
> +		} else {
> +			entry = list_next_entry(entry, list);
> +		}
> +
> +		start = end;
> +		end = ktime_add_ns(end, entry->interval);
> +		end = min_t(ktime_t, end, oper->cycle_end_time);
> +	}
> +
> +	*next = entry;
> +	*next_start = start;
> +	*end_time = end;
> +}
> +
>  static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  {
>  	struct taprio_sched *q = container_of(timer, struct taprio_sched,

[ ... ]

> @@ -960,14 +1005,19 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  		next = list_next_entry(entry, list);
>  	}
>  
> -	end_time = ktime_add_ns(entry->end_time, next->interval);
> +	next_start = entry->end_time;
> +	end_time = ktime_add_ns(next_start, next->interval);
>  	end_time = min_t(ktime_t, end_time, oper->cycle_end_time);
>  
> +	now = hrtimer_cb_get_time(timer);
> +	if (unlikely(ktime_before(end_time, now)))
> +		taprio_catch_up(oper, &next, &next_start, &end_time, now);
> +
>  	for (tc = 0; tc < num_tc; tc++) {
>  		if (next->gate_duration[tc] == oper->cycle_time)
>  			next->gate_close_time[tc] = KTIME_MAX;
>  		else
> -			next->gate_close_time[tc] = ktime_add_ns(entry->end_time,
> +			next->gate_close_time[tc] = ktime_add_ns(next_start,
>  								 next->gate_duration[tc]);
>  	}

[Severity: Low]
This is a pre-existing situation rather than something introduced here, but
it is worth noting for the multi-entry case that the walk newly makes
reachable.

After taprio_catch_up() walks forward, next can be the same sched_entry that
is still published in q->current_entry and being dereferenced by
taprio_dequeue_from_txq() under rcu_read_lock() only. advance_sched() then
rewrites that live entry's gate_close_time[] above and resets its budgets in
taprio_set_budgets(), and the following rcu_assign_pointer(q->current_entry,
next) publishes the same pointer, so it provides no ordering.

The same thing already happens without this patch for a single-entry
schedule, where should_restart_cycle() returns true through list_is_last()
and next ends up being the same object as entry. In the newly reachable
multi-entry case next == entry, so the gate mask is unchanged and a reader
sees either the old close time with a partly consumed budget or the new
close time with a full budget. On 32-bit hosts the ktime_t store can still
be observed torn, which is also pre-existing. Is it worth a comment that
advance_sched() may legitimately update the currently published entry?

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

[Junjie Cao]

From: Uladzislau Zhauniarovich <uladzislau.zhauniarovich@gmail.com>

The interval validation only requires an entry to cover the
transmission of a minimum sized frame at link speed. Virtual devices
inflate that budget: veth advertises 10Gb/s and bonding sums the
speeds of its members, so length_to_duration(ETH_ZLEN) evaluates to a
few tens of nanoseconds and schedules with nanosecond intervals pass
validation. In software mode each entry expiry is an hrtimer callback
costing on the order of 10us on a debug configuration and about a
microsecond on a release build; intervals below that cost rearm the
timer with an expiry already in the past, storming the CPU with back
to back timer interrupts until RCU stalls.

Require 100us per entry in software mode, leaving margin above the
timer service cost. Offloaded and txtime-assist schedules never arm
the per-entry hrtimer and keep the frame-length based minimum only.

Fixes: b5b73b26b3ca ("taprio: Fix allowing too small intervals")
Reported-by: syzbot+19d01f6082ec61dd45b2@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=19d01f6082ec61dd45b2
Reported-by: syzbot+8785aaf121cfb2141e0d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8785aaf121cfb2141e0d
Reported-by: syzbot+2642f347f7309b4880dc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2642f347f7309b4880dc
Tested-by: syzbot+19d01f6082ec61dd45b2@syzkaller.appspotmail.com
Tested-by: syzbot+8785aaf121cfb2141e0d@syzkaller.appspotmail.com
Tested-by: syzbot+2642f347f7309b4880dc@syzkaller.appspotmail.com
Link: https://lore.kernel.org/all/afe041f6-ef7d-4434-b2d0-096be49b5bcb@mail.kernel.org/
Signed-off-by: Uladzislau Zhauniarovich <uladzislau.zhauniarovich@gmail.com>
[jc: exempt txtime-assist, use s64 to keep rejecting negative
 cycle_time, rework commit message]
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 net/sched/sch_taprio.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 0d566c934b2f..91a7f7f17462 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -259,6 +259,26 @@ static int length_to_duration(struct taprio_sched *q, int len)
 	return div_u64(len * atomic64_read(&q->picos_per_byte), PSEC_PER_NSEC);
 }
 
+/* Software schedules service one hrtimer expiry per entry; intervals
+ * shorter than the expiry service cost rearm the timer with an expiry
+ * already in the past and storm the CPU. 100us leaves margin above the
+ * measured cost on debug configurations.
+ */
+#define TAPRIO_MIN_SW_INTERVAL_NS	(100 * NSEC_PER_USEC)
+
+static s64 taprio_min_interval(struct taprio_sched *q)
+{
+	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;
+}
+
 static int duration_to_length(struct taprio_sched *q, u64 duration)
 {
 	return div_u64(duration * PSEC_PER_NSEC, atomic64_read(&q->picos_per_byte));
@@ -1088,7 +1108,7 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
 			    struct sched_entry *entry,
 			    struct netlink_ext_ack *extack)
 {
-	int min_duration = length_to_duration(q, ETH_ZLEN);
+	s64 min_duration = taprio_min_interval(q);
 	u32 interval = 0;
 
 	if (tb[TCA_TAPRIO_SCHED_ENTRY_CMD])
@@ -1216,7 +1236,7 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
 		new->cycle_time = cycle;
 	}
 
-	if (new->cycle_time < new->num_entries * length_to_duration(q, ETH_ZLEN)) {
+	if (new->cycle_time < (s64)new->num_entries * taprio_min_interval(q)) {
 		NL_SET_ERR_MSG(extack, "'cycle_time' is too small");
 		return -EINVAL;
 	}
-- 
2.43.0

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

[Jakub Kicinski]

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/

> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 0d566c934b2f3..91a7f7f17462b 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -259,6 +259,26 @@ static int length_to_duration(struct taprio_sched *q, int len)
>  	return div_u64(len * atomic64_read(&q->picos_per_byte), PSEC_PER_NSEC);
>  }
>  
> +/* Software schedules service one hrtimer expiry per entry; intervals
> + * shorter than the expiry service cost rearm the timer with an expiry
> + * already in the past and storm the CPU. 100us leaves margin above the
> + * measured cost on debug configurations.
> + */
> +#define TAPRIO_MIN_SW_INTERVAL_NS	(100 * NSEC_PER_USEC)
> +
> +static s64 taprio_min_interval(struct taprio_sched *q)
> +{
> +	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;
> +}
> +

[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.

>  static int duration_to_length(struct taprio_sched *q, u64 duration)
>  {
>  	return div_u64(duration * PSEC_PER_NSEC, atomic64_read(&q->picos_per_byte));
> @@ -1088,7 +1108,7 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
>  			    struct sched_entry *entry,
>  			    struct netlink_ext_ack *extack)
>  {
> -	int min_duration = length_to_duration(q, ETH_ZLEN);
> +	s64 min_duration = taprio_min_interval(q);
>  	u32 interval = 0;
>  

[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?

>  	if (tb[TCA_TAPRIO_SCHED_ENTRY_CMD])
> @@ -1216,7 +1236,7 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
>  		new->cycle_time = cycle;
>  	}
>  
> -	if (new->cycle_time < new->num_entries * length_to_duration(q, ETH_ZLEN)) {
> +	if (new->cycle_time < (s64)new->num_entries * taprio_min_interval(q)) {
>  		NL_SET_ERR_MSG(extack, "'cycle_time' is too small");
>  		return -EINVAL;
>  	}

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

[Junjie Cao]

Entries below TAPRIO_MIN_SW_INTERVAL_NS must be rejected for software
schedules.

Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 .../tc-testing/tc-tests/qdiscs/taprio.json    | 22 +++++++++++++++++++
 1 file changed, 22 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 cd19d05925e4..3fe66839f8ae 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",
+        "verifyCmd": "$TC qdisc show dev $ETH",
+        "matchPattern": "qdisc taprio 1: root refcnt",
+        "matchCount": "0",
+        "teardown": [
+            "echo \"1\" > /sys/bus/netdevsim/del_device"
+        ]
+    },
     {
         "id": "831f",
         "name": "Add taprio Qdisc with too short cycle-time",
-- 
2.43.0

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

[Jakub Kicinski]

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"
> +        ]
> +    },

[ ... ]