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/[email protected]/ > 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; > }
