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

[Krystian Kaniewski <krystianmkaniewski@gmail.com>]

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

-- 
Krystian Kaniewski