Date: Tue, 19 Dec 2023 18:56:50 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
Cc: Vinicius Costa Gomes <vinicius.gomes@...el.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

On Tue, Dec 19, 2023 at 03:14:49AM -0500, Faizal Rahim wrote:
> According to IEEE Std. 802.1Q-2018 section Q.5 CycleTimeExtension,
> the Cycle Time Extension variable allows this extension of the last old
> cycle to be done in a defined way. If the last complete old cycle would
> normally end less than OperCycleTimeExtension nanoseconds before the new
> base time, then the last complete cycle before AdminBaseTime is reached
> is extended so that it ends at AdminBaseTime.
> 
> Changes in v3:
> - Removed the last 3 patches related to fixing cycle time adjustment
> for the "current entry". This is to simplify this patch series submission 
> which only covers cycle time adjustment for the "next entry".
> - Negative correction calculation in get_cycle_time_correction() is
>   guarded so that it doesn't exceed interval
> - Some rename (macro, function)
> - Transport commit message comments to the code comments 
> - Removed unnecessary null check
> - Reword commit message 
> 
> Changes in v2:
> - Added 's64 cycle_time_correction' in 'sched_gate_list struct'.
> - Removed sched_changed created in v1 since the new cycle_time_correction
>   field can also serve to indicate the need for a schedule change.
> - Added 'bool correction_active' in 'struct sched_entry' to represent
>   the correction state from the entry's perspective and return corrected
>   interval value when active.
> - Fix cycle time correction logics for the next entry in advance_sched()
> - Fix and implement proper cycle time correction logics for current
>   entry in taprio_start_sched()
> 
> v2 at:
> https://lore.kernel.org/lkml/20231107112023.676016-1-faizal.abdul.rahim@linux.intel.com/
> v1 at:
> https://lore.kernel.org/lkml/20230530082541.495-1-muhammad.husaini.zulkifli@intel.com/

I'm sorry that I stopped responding on your v2. I realized the discussion
reached a point where I couldn't figure out who is right without some
testing. I wanted to write a selftest to highlight the expected correct
behavior of the datapath during various schedule changes, and whether we
could ever end up with a negative interval after the correction. However,
writing that got quite complicated and that ended there.

How are you testing the behavior, and who reported the issues / what prompted
the changes? Honestly I'm not very confident in the changes we're
pushing down the linux-stable pipe. They don't look all that obvious, so
I still think that having selftests would help. If you don't have a
testing rig already assembled, and you don't want to start one, I might
want to give it a second try and cook something up myself.

Something really simple like:
- start schedule 1 with base-time A and cycle-time-extension B
- start schedule 2 with base-time C
- send one packet with isochron during the last cycle of schedule 1

By varying the parameters, we could check if the schedule is correctly
extended or truncated. We could configure the 2 schedules in such a way
that "extending" would mean that isochron's gate (from schedule 1) is
open (and thus, the packet will pass) and "truncating" would mean that
the packet is scheduled according to schedule 2 (where isochron's gate
will be always closed, so the packet will never pass).

We could then alter the cycle-time-extension relative to the base-times,
to force a truncation of 1, 2, 3 entries or more, and see that the
behavior is always correct.

Powered by blists - more mailing lists