Re: [PATCH net v2 2/2] net/sched: taprio: do not replay missed entries in advance_sched()

[Vinicius Costa Gomes]

Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx> writes:

> On 10/13/26 00:53, Vinicius Costa Gomes wrote:
>
> Hi Vinicius,
>
> Thanks for the review, and for the Ack on patch 1.
>
>>>  struct taprio_sched {
>>> @@ -902,8 +909,19 @@ static bool should_change_schedules(const struct sched_gate_list *admin,
>>>  	 * plus the amount that can be extended would fall after the
>>>  	 * next schedule base_time, we can extend the current schedule
>>>  	 * for that amount.
>>> +	 *
>>> +	 * cycle_time_extension is an unbounded signed value from netlink, so
>>> +	 * clamp the sum instead of letting it wrap. A wrap would make this
>>> +	 * test non-monotonic in end_time, and advance_sched() relies on it
>>> +	 * being monotonic to check it once after catching up instead of once
>>> +	 * per skipped entry.
>>>  	 */
>>> -	extension_time = ktime_add_ns(end_time, oper->cycle_time_extension);
>>> +	if (oper->cycle_time_extension > 0 &&
>>> +	    end_time > KTIME_MAX - oper->cycle_time_extension)
>>> +		extension_time = KTIME_MAX;
>>> +	else
>>> +		extension_time = ktime_add_ns(end_time,
>>> +					      oper->cycle_time_extension);
>>>
>> 
>> This one I could argue that it could be in a separate patch. As the
>> wrapping could cause issues even before these changes, no?
>> 
>
> Yes. With an extension larger than KTIME_MAX minus the end time, the sum
> becomes negative and should_change_schedules() ignores the extension.
> That is independent of the stall. I can move the clamp to a separate 
> patch with a Fixes tag for c25031e99344 ("taprio: Add support
> for cycle-time-extension"), before the advance_sched() change which
> relies on it.
>
> get_interval_end_time() has the same addition on the txtime-assist path.
> I can cover that path in the same  patch as well. Would you like me to
> do it?

Good catch. Yeah, fixing that one as well would be good.

>>> +	struct sched_entry *entry, *found = NULL;
>>> +	ktime_t cycle_start = sched->start_time;
>>> +	s64 offset, entry_end = 0, elapsed = 0;
>>> +
>>> +	if (ktime_after(now, cycle_start)) {
>>> +		s64 periods = div64_s64(ktime_sub(now, cycle_start),
>>> +					sched->period);
>>> +
>>> +		cycle_start = ktime_add_ns(cycle_start,
>>> +					   periods * sched->period);
>>> +	}
>>> +	offset = ktime_sub(now, cycle_start);
>>> +
>>> +	/* The offset is less than one period, so an entry which ends after
>>> +	 * @now is found at the latest at the entry which reaches the end of
>>> +	 * the cycle, or at the last entry. A schedule has at least one entry,
>>> +	 * so the loop always sets found.
>>> +	 */
>>> +	list_for_each_entry(entry, &sched->entries, list) {
>>> +		entry_end = min_t(s64, elapsed + entry->interval,
>>> +				  sched->cycle_time);
>>> +		if (offset < entry_end ||
>>> +		    list_is_last(&entry->list, &sched->entries)) {
>>> +			found = entry;
>>> +			break;
>>> +		}
>>> +		elapsed = entry_end;
>>> +	}
>>> +
>>> +	*start = ktime_add_ns(cycle_start, elapsed);
>>> +	*end = ktime_add_ns(cycle_start, entry_end);
>>> +	sched->cycle_end_time = ktime_add_ns(cycle_start,
>>> sched->cycle_time);
>> 
>> Having this side effect here was unexpected. I think this would make
>> more sense outside this function.
>> 
>
> Agreed. I can make taprio_entry_at() return the cycle end through an
> extra output argument and have advance_sched() store it explicitly in
> oper->cycle_end_time. The helper could then take the schedule as const.
>
> I can wait for the rest of your review of this patch before sending v3
>

I am going to try and review this by end of this week.


Cheers,
-- 
Vinicius