[PATCH v3 net 0/4] qbv cycle time extension/truncation

[PATCH v3 net 0/4] qbv cycle time extension/truncation

[Faizal Rahim]

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/

Faizal Rahim (4):
  net/sched: taprio: fix too early schedules switching
  net/sched: taprio: fix cycle time adjustment for next entry
  net/sched: taprio: fix impacted fields value during cycle time
    adjustment
  net/sched: taprio: get corrected value of cycle_time and interval

 net/sched/sch_taprio.c | 178 +++++++++++++++++++++++++++++++----------
 1 file changed, 135 insertions(+), 43 deletions(-)

-- 
2.25.1

[PATCH v3 net 1/4] net/sched: taprio: fix too early schedules switching

[Faizal Rahim]

Switching the schedule prematurely leads to a situation where the last
entry from oper schedule is still running, during this period, calls
to taprio_skb_exceeds_queue_max_sdu() in the enqueue path, such as
taprio_enqueue_segmented(), will inspect q->oper_sched. At this point,
q->oper_sched refers to the new admin schedule instead of the ongoing
oper schedule.

Fixes: a878fd46fe43 ("net/sched: keep the max_frm_len information inside struct sched_gate_list")
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 net/sched/sch_taprio.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 31a8252bd09c..bbcaf05d40ba 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -41,6 +41,7 @@ static struct static_key_false taprio_have_working_mqprio;
 #define TXTIME_ASSIST_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST)
 #define FULL_OFFLOAD_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_FULL_OFFLOAD)
 #define TAPRIO_FLAGS_INVALID U32_MAX
+#define CYCLE_TIME_CORRECTION_UNSPEC S64_MIN
 
 struct sched_entry {
 	/* Durations between this GCL entry and the GCL entry where the
@@ -75,6 +76,7 @@ struct sched_gate_list {
 	ktime_t cycle_end_time;
 	s64 cycle_time;
 	s64 cycle_time_extension;
+	s64 cycle_time_correction;
 	s64 base_time;
 };
 
@@ -213,6 +215,11 @@ static void switch_schedules(struct taprio_sched *q,
 	*admin = NULL;
 }
 
+static bool sched_switch_pending(const struct sched_gate_list *oper)
+{
+	return oper->cycle_time_correction != CYCLE_TIME_CORRECTION_UNSPEC;
+}
+
 /* Get how much time has been already elapsed in the current cycle. */
 static s32 get_cycle_time_elapsed(struct sched_gate_list *sched, ktime_t time)
 {
@@ -940,7 +947,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 	admin = rcu_dereference_protected(q->admin_sched,
 					  lockdep_is_held(&q->current_entry_lock));
 
-	if (!oper)
+	if (!oper || sched_switch_pending(oper))
 		switch_schedules(q, &admin, &oper);
 
 	/* This can happen in two cases: 1. this is the very first run
@@ -981,7 +988,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 		 * schedule runs.
 		 */
 		end_time = sched_base_time(admin);
-		switch_schedules(q, &admin, &oper);
+		oper->cycle_time_correction = 0;
 	}
 
 	next->end_time = end_time;
@@ -1174,6 +1181,7 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
 	}
 
 	taprio_calculate_gate_durations(q, new);
+	new->cycle_time_correction = CYCLE_TIME_CORRECTION_UNSPEC;
 
 	return 0;
 }
-- 
2.25.1

[PATCH v3 net 2/4] net/sched: taprio: fix cycle time adjustment for next entry

[Faizal Rahim]

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

Fix cyle time modification logic for the next entry that includes the
following cases:
a) positive correction - cycle time extension
b) negative correction - cycle time truncation
c) zero correction - new admin base time aligns exactly with the old
cycle

Fixes: a3d43c0d56f1 ("taprio: Add support adding an admin schedule")
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 net/sched/sch_taprio.c | 100 +++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 38 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index bbcaf05d40ba..e70dc69c311f 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -893,38 +893,54 @@ static bool should_restart_cycle(const struct sched_gate_list *oper,
 	return false;
 }
 
-static bool should_change_schedules(const struct sched_gate_list *admin,
-				    const struct sched_gate_list *oper,
-				    ktime_t end_time)
-{
-	ktime_t next_base_time, extension_time;
-
-	if (!admin)
-		return false;
-
-	next_base_time = sched_base_time(admin);
-
-	/* This is the simple case, the end_time would fall after
-	 * the next schedule base_time.
-	 */
-	if (ktime_compare(next_base_time, end_time) <= 0)
-		return true;
-
-	/* This is the cycle_time_extension case, if the end_time
-	 * plus the amount that can be extended would fall after the
-	 * next schedule base_time, we can extend the current schedule
-	 * for that amount.
-	 */
-	extension_time = ktime_add_ns(end_time, oper->cycle_time_extension);
+static bool should_extend_cycle(const struct sched_gate_list *oper,
+				ktime_t new_base_time,
+				ktime_t next_entry_end_time,
+				const struct sched_entry *next_entry)
+{
+	ktime_t next_cycle_end_time = ktime_add_ns(oper->cycle_end_time,
+						   oper->cycle_time);
+	bool extension_supported = oper->cycle_time_extension > 0;
+	s64 extension_limit = oper->cycle_time_extension;
+	s64 extension_duration = ktime_sub(new_base_time, next_entry_end_time);
+
+	return extension_supported &&
+	       list_is_last(&next_entry->list, &oper->entries) &&
+	       ktime_before(new_base_time, next_cycle_end_time) &&
+	       extension_duration < extension_limit;
+}
+
+static s64 get_cycle_time_correction(const struct sched_gate_list *oper,
+				     ktime_t new_base_time,
+				     ktime_t next_entry_end_time,
+				     const struct sched_entry *next_entry)
+{
+	s64 correction = CYCLE_TIME_CORRECTION_UNSPEC;
+
+	if (ktime_compare(new_base_time, next_entry_end_time) <= 0) {
+		/* Negative correction - The new admin base time starts earlier
+		 * than the next entry's end time.
+		 * Zero correction - The new admin base time aligns exactly
+		 * with the old cycle.
+		 */
+		correction = ktime_sub(new_base_time, next_entry_end_time);
 
-	/* FIXME: the IEEE 802.1Q-2018 Specification isn't clear about
-	 * how precisely the extension should be made. So after
-	 * conformance testing, this logic may change.
-	 */
-	if (ktime_compare(next_base_time, extension_time) <= 0)
-		return true;
+		/* Below is to hande potential issue where the negative correction
+		 * exceed the entry's interval. This typically shouldn't happen.
+		 * Setting to 0 enables schedule changes without altering cycle time.
+		 */
+		if (abs(correction) > next_entry->interval)
+			correction = 0;
+	} else if (ktime_after(new_base_time, next_entry_end_time) &&
+		   should_extend_cycle(oper, new_base_time,
+				       next_entry_end_time, next_entry)) {
+		/* Positive correction - The new admin base time starts after the
+		 * last entry end time and within the next cycle time of old oper.
+		 */
+		correction = ktime_sub(new_base_time, next_entry_end_time);
+	}
 
-	return false;
+	return correction;
 }
 
 static enum hrtimer_restart advance_sched(struct hrtimer *timer)
@@ -975,6 +991,22 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 	end_time = ktime_add_ns(entry->end_time, next->interval);
 	end_time = min_t(ktime_t, end_time, oper->cycle_end_time);
 
+	if (admin) {
+		ktime_t new_base_time = sched_base_time(admin);
+
+		oper->cycle_time_correction =
+			get_cycle_time_correction(oper, new_base_time,
+						  end_time, next);
+
+		if (sched_switch_pending(oper)) {
+			/* The next entry is the last entry we will run from
+			 * oper, subsequent ones will take from the new admin
+			 */
+			oper->cycle_end_time = new_base_time;
+			end_time = new_base_time;
+		}
+	}
+
 	for (tc = 0; tc < num_tc; tc++) {
 		if (next->gate_duration[tc] == oper->cycle_time)
 			next->gate_close_time[tc] = KTIME_MAX;
@@ -983,14 +1015,6 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 								 next->gate_duration[tc]);
 	}
 
-	if (should_change_schedules(admin, oper, end_time)) {
-		/* Set things so the next time this runs, the new
-		 * schedule runs.
-		 */
-		end_time = sched_base_time(admin);
-		oper->cycle_time_correction = 0;
-	}
-
 	next->end_time = end_time;
 	taprio_set_budgets(q, oper, next);
 
-- 
2.25.1

[PATCH v3 net 3/4] net/sched: taprio: fix impacted fields value during cycle time adjustment

[Faizal Rahim]

During the cycle time adjustment period, there's a single entry left
from the oper schedule to be executed. As a result, updates are
needed for the affected fields' logic, which did not previously
consider dynamic scheduling.

Fixes: a306a90c8ffe ("net/sched: taprio: calculate tc gate durations")
Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 net/sched/sch_taprio.c | 44 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index e70dc69c311f..a3c71be21af2 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -285,7 +285,8 @@ static void taprio_update_queue_max_sdu(struct taprio_sched *q,
 		/* TC gate never closes => keep the queueMaxSDU
 		 * selected by the user
 		 */
-		if (sched->max_open_gate_duration[tc] == sched->cycle_time) {
+		if (sched->max_open_gate_duration[tc] == sched->cycle_time &&
+		    !sched_switch_pending(sched)) {
 			max_sdu_dynamic = U32_MAX;
 		} else {
 			u32 max_frm_len;
@@ -681,7 +682,8 @@ static void taprio_set_budgets(struct taprio_sched *q,
 
 	for (tc = 0; tc < num_tc; tc++) {
 		/* Traffic classes which never close have infinite budget */
-		if (entry->gate_duration[tc] == sched->cycle_time)
+		if (entry->gate_duration[tc] == sched->cycle_time &&
+		    !sched_switch_pending(sched))
 			budget = INT_MAX;
 		else
 			budget = div64_u64((u64)entry->gate_duration[tc] * PSEC_PER_NSEC,
@@ -893,6 +895,29 @@ static bool should_restart_cycle(const struct sched_gate_list *oper,
 	return false;
 }
 
+/* Open gate duration were calculated at the beginning with consideration of
+ * multiple entries. If sched_switch_pending() is active, there's only a single
+ * remaining entry left from oper to run. Update open gate duration based
+ * on this last entry.
+ */
+static void update_open_gate_duration(struct sched_entry *entry,
+				      struct sched_gate_list *oper,
+				      int num_tc,
+				      u64 open_gate_duration)
+{
+	int tc;
+
+	for (tc = 0; tc < num_tc; tc++) {
+		if (entry->gate_mask & BIT(tc)) {
+			entry->gate_duration[tc] = open_gate_duration;
+			oper->max_open_gate_duration[tc] = open_gate_duration;
+		} else {
+			entry->gate_duration[tc] = 0;
+			oper->max_open_gate_duration[tc] = 0;
+		}
+	}
+}
+
 static bool should_extend_cycle(const struct sched_gate_list *oper,
 				ktime_t new_base_time,
 				ktime_t next_entry_end_time,
@@ -1002,13 +1027,26 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 			/* The next entry is the last entry we will run from
 			 * oper, subsequent ones will take from the new admin
 			 */
+			u64 new_gate_duration =
+				next->interval + oper->cycle_time_correction;
+			struct qdisc_size_table *stab;
+
 			oper->cycle_end_time = new_base_time;
 			end_time = new_base_time;
+
+			update_open_gate_duration(next, oper, num_tc,
+						  new_gate_duration);
+			rcu_read_lock();
+			stab = rcu_dereference(q->root->stab);
+			taprio_update_queue_max_sdu(q, oper, stab);
+			rcu_read_unlock();
 		}
 	}
 
 	for (tc = 0; tc < num_tc; tc++) {
-		if (next->gate_duration[tc] == oper->cycle_time)
+		if (sched_switch_pending(oper) && (next->gate_mask & BIT(tc)))
+			next->gate_close_time[tc] = end_time;
+		else 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,
-- 
2.25.1

[PATCH v3 net 4/4] net/sched: taprio: get corrected value of cycle_time and interval

[Faizal Rahim]

Added a new field, correction_active to determine the entry's correction
state. This field is required due to specific flow like
find_entry_to_transmit() -> get_interval_end_time() which retrieves
the interval for each entry. During positive cycle time correction,
it's known that the last entry interval requires correction.
However, for negative correction, the affected entry is unknown, which
is why this new field is necessary.

Note that in some cases where the original values are required,
such as in dump_schedule() and setup_first_end_time(), direct calls
to cycle_time and interval are retained without using the new functions.

Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
---
 net/sched/sch_taprio.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index a3c71be21af2..d11ddb1f554c 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -61,6 +61,7 @@ struct sched_entry {
 	u32 gate_mask;
 	u32 interval;
 	u8 command;
+	bool correction_active;
 };
 
 struct sched_gate_list {
@@ -220,6 +221,14 @@ static bool sched_switch_pending(const struct sched_gate_list *oper)
 	return oper->cycle_time_correction != CYCLE_TIME_CORRECTION_UNSPEC;
 }
 
+static s64 get_cycle_time(const struct sched_gate_list *oper)
+{
+	if (sched_switch_pending(oper))
+		return oper->cycle_time + oper->cycle_time_correction;
+	else
+		return oper->cycle_time;
+}
+
 /* Get how much time has been already elapsed in the current cycle. */
 static s32 get_cycle_time_elapsed(struct sched_gate_list *sched, ktime_t time)
 {
@@ -227,11 +236,20 @@ static s32 get_cycle_time_elapsed(struct sched_gate_list *sched, ktime_t time)
 	s32 time_elapsed;
 
 	time_since_sched_start = ktime_sub(time, sched->base_time);
-	div_s64_rem(time_since_sched_start, sched->cycle_time, &time_elapsed);
+	div_s64_rem(time_since_sched_start, get_cycle_time(sched), &time_elapsed);
 
 	return time_elapsed;
 }
 
+static u32 get_interval(const struct sched_entry *entry,
+			const struct sched_gate_list *oper)
+{
+	if (entry->correction_active)
+		return entry->interval + oper->cycle_time_correction;
+	else
+		return entry->interval;
+}
+
 static ktime_t get_interval_end_time(struct sched_gate_list *sched,
 				     struct sched_gate_list *admin,
 				     struct sched_entry *entry,
@@ -240,8 +258,9 @@ static ktime_t get_interval_end_time(struct sched_gate_list *sched,
 	s32 cycle_elapsed = get_cycle_time_elapsed(sched, intv_start);
 	ktime_t intv_end, cycle_ext_end, cycle_end;
 
-	cycle_end = ktime_add_ns(intv_start, sched->cycle_time - cycle_elapsed);
-	intv_end = ktime_add_ns(intv_start, entry->interval);
+	cycle_end = ktime_add_ns(intv_start,
+				 get_cycle_time(sched) - cycle_elapsed);
+	intv_end = ktime_add_ns(intv_start, get_interval(entry, sched));
 	cycle_ext_end = ktime_add(cycle_end, sched->cycle_time_extension);
 
 	if (ktime_before(intv_end, cycle_end))
@@ -348,7 +367,7 @@ static struct sched_entry *find_entry_to_transmit(struct sk_buff *skb,
 	if (!sched)
 		return NULL;
 
-	cycle = sched->cycle_time;
+	cycle = get_cycle_time(sched);
 	cycle_elapsed = get_cycle_time_elapsed(sched, time);
 	curr_intv_end = ktime_sub_ns(time, cycle_elapsed);
 	cycle_end = ktime_add_ns(curr_intv_end, cycle);
@@ -362,7 +381,7 @@ static struct sched_entry *find_entry_to_transmit(struct sk_buff *skb,
 			break;
 
 		if (!(entry->gate_mask & BIT(tc)) ||
-		    packet_transmit_time > entry->interval)
+		    packet_transmit_time > get_interval(entry, sched))
 			continue;
 
 		txtime = entry->next_txtime;
@@ -540,7 +559,8 @@ static long get_packet_txtime(struct sk_buff *skb, struct Qdisc *sch)
 		 * interval starts.
 		 */
 		if (ktime_after(transmit_end_time, interval_end))
-			entry->next_txtime = ktime_add(interval_start, sched->cycle_time);
+			entry->next_txtime =
+				ktime_add(interval_start, get_cycle_time(sched));
 	} while (sched_changed || ktime_after(transmit_end_time, interval_end));
 
 	entry->next_txtime = transmit_end_time;
@@ -1033,6 +1053,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
 
 			oper->cycle_end_time = new_base_time;
 			end_time = new_base_time;
+			next->correction_active = true;
 
 			update_open_gate_duration(next, oper, num_tc,
 						  new_gate_duration);
@@ -1133,6 +1154,7 @@ static int fill_sched_entry(struct taprio_sched *q, struct nlattr **tb,
 	}
 
 	entry->interval = interval;
+	entry->correction_active = false;
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Vladimir Oltean]

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.

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Eric Dumazet]

On Tue, Dec 19, 2023 at 9:17 AM Faizal Rahim
<faizal.abdul.rahim@linux.intel.com> 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.
>

Hmm... Is this series fixing any of the pending syzbot bugs ?

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Abdul Rahim, Faizal]

On 20/12/2023 12:56 am, Vladimir Oltean wrote:
> 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.

Hi Vladimir,

No worries, I truly appreciate the time you took to review and reply.

What prompted this in general is related to my project requirement to 
enable software QBV cycle time extension, so there's a validation team that 
created test cases to properly validate cycle time extension. Then I 
noticed the code doesn't handle truncation properly also, since it's the 
same code area, I just fixed it together.

Each time before sending the patch for upstream review, I normally will run 
our test cases that only validates cycle time extension. For truncation, I 
modify the test cases on my own and put logs to check if the 
cycle_time_correction negative value is within the correct range. I 
probably should have mentioned sooner that I have tested this myself, sorry 
about that.

Example of the test I run for cycle time extension:
1) 2 boards connected back-to-back with i226 NIC. Board A as sender, Board 
B as receiver
2) Time is sync between 2 boards with phc2sys and ptp4l
3) Run GCL1 on Board A with cycle time extension enabled:
     tc qdisc replace dev $INTERFACE parent root handle 100 taprio \
     num_tc 4 \
     map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
     queues 1@0 1@1 1@2 1@3 \
     base-time 0 \
     cycle-time-extension 1000000 \
     sched-entry S 09 500000 \
     sched-entry S 0a 500000 \
     clockid CLOCK_TAI
4) capture tcp dump on Board B
5) Send packets from Board A to Board B with 200us interval via UDP Tai
6) When packets reached Board B, trigger GCL2 to Board A:
    CYCLETIME=1000000
    APPLYTIME=1000000000 # 1s
    CURRENT=$(date +%s%N)
    BASE=$(( (CURRENT + APPLYTIME + (2*CYCLETIME)) - ((CURRENT + APPLYTIME)
          % CYCLETIME) + ((CYCLETIME*3)/5) ))
     tc qdisc replace dev $INTERFACE parent root handle 100 taprio \
     num_tc 4 \
     map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
     queues 1@0 1@1 1@2 1@3 \
     base-time $BASE \
     cycle-time-extension 1000000 \
     sched-entry S oc 500000 \
     sched-entry S 08 500000 \
     clockid CLOCK_TAI
7) Analyze tcp dump data on Board B using wireshark, will observe packets 
receive pattern changed.

Note that I've hidden "Best Effort (default) 7001 → 7001" data from the 
wireshark log so that it's easier to see the pattern.

      TIMESTAMP               PRIORITY             PRIORITY    NOTES 

1702896645.925014509	Critical Applications	7004 → 7004   GCL1
1702896645.925014893	Critical Applications	7004 → 7004   GCL1
1702896645.925514454	Excellent Effort	7003 → 7003   GCL1
1702896645.925514835	Excellent Effort	7003 → 7003   GCL1
1702896645.926014371	Critical Applications	7004 → 7004   GCL1
1702896645.926014755	Critical Applications	7004 → 7004   GCL1
1702896645.926514620	Excellent Effort	7003 → 7003   GCL1
1702896645.926515004	Excellent Effort	7003 → 7003   GCL1
1702896645.927014408	Critical Applications	7004 → 7004   GCL1
1702896645.927014792	Critical Applications	7004 → 7004   GCL1
1702896645.927514789	Excellent Effort	7003 → 7003   GCL1
1702896645.927515173	Excellent Effort	7003 → 7003   GCL1
1702896645.928168304	Excellent Effort	7003 → 7003   Extended
1702896645.928368780	Excellent Effort	7003 → 7003   Extended
1702896645.928569406	Excellent Effort	7003 → 7003   Extended
1702896645.929614835	Background	        7002 → 7002   GCL2
1702896645.929615219	Background	        7002 → 7002   GCL2
1702896645.930614643	Background	        7002 → 7002   GCL2
1702896645.930615027	Background	        7002 → 7002   GCL2
1702896645.931614604	Background	        7002 → 7002   GCL2
1702896645.931614991	Background	        7002 → 7002   GCL2

The extended packets only will happen if cycle_time and interval fields
are updated using cycle_time_correction. Without that patch, the extended 
packets are not received.


As for the negative truncation case, I just make the interval quite long, 
and experimented with GCL2 base-time value so that it hits the "next entry" 
in advance_sched(). Then I checked my logs in get_cycle_time_correction() 
to see the truncation case and its values.

Based on your feedback of the test required, I think that my existing 
truncation test is not enough, but the extension test case part should be 
good right ?

Do let me know then, I'm more than willing to do more test for the 
truncation case as per your suggestion, well basically, anything to help 
speed up the patches series review process :)


Appreciate your suggestion and help a lot, thank you.

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Abdul Rahim, Faizal]

On 20/12/2023 1:02 am, Eric Dumazet wrote:
> On Tue, Dec 19, 2023 at 9:17 AM Faizal Rahim
> <faizal.abdul.rahim@linux.intel.com> 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.
>>
> 
> Hmm... Is this series fixing any of the pending syzbot bugs ?

Not really I think ? I found some bugs in this area when I tried to 
enable/fix software QBV cycle time extension for my project.

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Paolo Abeni]

On Tue, 2023-12-19 at 18:56 +0200, Vladimir Oltean wrote:
> 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.

I agree with Vladimir, this looks quite a bit too complex for a net fix
at this late point of the cycle. Given the period of the year, I think
it could be too late even for net-next - for this cycle.

It would be great if you could add some self-tests.

@Faizal: I understand your setup is quite complex, but it would be
great if you could come-up with something similar that could fit 
tools/testing/selftests/net

Thanks!

Paolo

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Abdul Rahim, Faizal]

On 21/12/2023 4:52 pm, Paolo Abeni wrote:
> On Tue, 2023-12-19 at 18:56 +0200, Vladimir Oltean wrote:
>> 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.
> 
> I agree with Vladimir, this looks quite a bit too complex for a net fix
> at this late point of the cycle. Given the period of the year, I think
> it could be too late even for net-next - for this cycle.
> 

Would it be better to just submit into net-next and target for the next 
cycle ? I'm okay with that.

> It would be great if you could add some self-tests.
> 
> @Faizal: I understand your setup is quite complex, but it would be
> great if you could come-up with something similar that could fit
> tools/testing/selftests/net
> 
> Thanks!
> 
> Paolo
> 

Ohh my bad, I thought selftest is just to develop and run the test case 
locally, but it seems that it also refers to integrating it into the 
existing selftest framework ?
Got it. I'll explore that and cover extension/truncation cases.

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Vladimir Oltean]

(sorry, I started writing this email yesterday, I noticed the
conversation continued with Paolo)

On Wed, Dec 20, 2023 at 11:25:09AM +0800, Abdul Rahim, Faizal wrote:
> Hi Vladimir,
> 
> No worries, I truly appreciate the time you took to review and reply.
> 
> What prompted this in general is related to my project requirement to enable
> software QBV cycle time extension, so there's a validation team that created
> test cases to properly validate cycle time extension. Then I noticed the
> code doesn't handle truncation properly also, since it's the same code area,
> I just fixed it together.

We tend to do patch triage between 'net' and 'net-next' based on the
balance between the urgency/impact of the fix and its complexity.

While it's undoubtable that there are issues with taprio's handling of
dynamic schedules, you've mentioned yourself that you only hit those
issues as part of some new development work - they weren't noticed by
end users. And fixing them is not quite trivial, there are also FIXMEs
in taprio which suggest so. I'm worried that the fixes may also impact
the code from stable trees in unforeseen ways.

So I would recommend moving the development of these fixes to 'net-next',
if possible.

> Each time before sending the patch for upstream review, I normally will run
> our test cases that only validates cycle time extension. For truncation, I
> modify the test cases on my own and put logs to check if the
> cycle_time_correction negative value is within the correct range. I probably
> should have mentioned sooner that I have tested this myself, sorry about
> that.
> 
> Example of the test I run for cycle time extension:
> 1) 2 boards connected back-to-back with i226 NIC. Board A as sender, Board B
> as receiver
> 2) Time is sync between 2 boards with phc2sys and ptp4l
> 3) Run GCL1 on Board A with cycle time extension enabled:
>     tc qdisc replace dev $INTERFACE parent root handle 100 taprio \
>     num_tc 4 \
>     map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
>     queues 1@0 1@1 1@2 1@3 \
>     base-time 0 \
>     cycle-time-extension 1000000 \
>     sched-entry S 09 500000 \
>     sched-entry S 0a 500000 \
>     clockid CLOCK_TAI

Why do you need PTP sync? Cannot this test run between 2 veth ports?

> 4) capture tcp dump on Board B
> 5) Send packets from Board A to Board B with 200us interval via UDP Tai

What is udp_tai? This program?
https://gist.github.com/jeez/bd3afeff081ba64a695008dd8215866f

> 6) When packets reached Board B, trigger GCL2 to Board A:
>    CYCLETIME=1000000
>    APPLYTIME=1000000000 # 1s
>    CURRENT=$(date +%s%N)
>    BASE=$(( (CURRENT + APPLYTIME + (2*CYCLETIME)) - ((CURRENT + APPLYTIME)
>          % CYCLETIME) + ((CYCLETIME*3)/5) ))
>     tc qdisc replace dev $INTERFACE parent root handle 100 taprio \
>     num_tc 4 \
>     map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
>     queues 1@0 1@1 1@2 1@3 \
>     base-time $BASE \
>     cycle-time-extension 1000000 \
>     sched-entry S oc 500000 \
>     sched-entry S 08 500000 \
>     clockid CLOCK_TAI
> 7) Analyze tcp dump data on Board B using wireshark, will observe packets
> receive pattern changed.
> 
> Note that I've hidden "Best Effort (default) 7001 → 7001" data from the
> wireshark log so that it's easier to see the pattern.
> 
>      TIMESTAMP               PRIORITY             PRIORITY    NOTES
> 
> 1702896645.925014509	Critical Applications	7004 → 7004   GCL1
> 1702896645.925014893	Critical Applications	7004 → 7004   GCL1
> 1702896645.925514454	Excellent Effort	7003 → 7003   GCL1
> 1702896645.925514835	Excellent Effort	7003 → 7003   GCL1
> 1702896645.926014371	Critical Applications	7004 → 7004   GCL1
> 1702896645.926014755	Critical Applications	7004 → 7004   GCL1
> 1702896645.926514620	Excellent Effort	7003 → 7003   GCL1
> 1702896645.926515004	Excellent Effort	7003 → 7003   GCL1
> 1702896645.927014408	Critical Applications	7004 → 7004   GCL1
> 1702896645.927014792	Critical Applications	7004 → 7004   GCL1
> 1702896645.927514789	Excellent Effort	7003 → 7003   GCL1
> 1702896645.927515173	Excellent Effort	7003 → 7003   GCL1
> 1702896645.928168304	Excellent Effort	7003 → 7003   Extended
> 1702896645.928368780	Excellent Effort	7003 → 7003   Extended
> 1702896645.928569406	Excellent Effort	7003 → 7003   Extended
> 1702896645.929614835	Background	        7002 → 7002   GCL2
> 1702896645.929615219	Background	        7002 → 7002   GCL2
> 1702896645.930614643	Background	        7002 → 7002   GCL2
> 1702896645.930615027	Background	        7002 → 7002   GCL2
> 1702896645.931614604	Background	        7002 → 7002   GCL2
> 1702896645.931614991	Background	        7002 → 7002   GCL2
> 
> The extended packets only will happen if cycle_time and interval fields
> are updated using cycle_time_correction. Without that patch, the extended
> packets are not received.
> 
> 
> As for the negative truncation case, I just make the interval quite long,
> and experimented with GCL2 base-time value so that it hits the "next entry"
> in advance_sched(). Then I checked my logs in get_cycle_time_correction() to
> see the truncation case and its values.
> 
> Based on your feedback of the test required, I think that my existing
> truncation test is not enough, but the extension test case part should be
> good right ?
> 
> Do let me know then, I'm more than willing to do more test for the
> truncation case as per your suggestion, well basically, anything to help
> speed up the patches series review process :)
> 
> 
> Appreciate your suggestion and help a lot, thank you.

Do you think you could automate a test suite which only measures software
TX timestamps and works on veth?

I prepared this very small patch set just to give you a head start
(the skeleton). You'll still have to add the logic for individual tests.
https://lore.kernel.org/netdev/20231221132521.2314811-1-vladimir.oltean@nxp.com/
I'm terribly sorry, but this is the most I can do due to my current lack
of spare time, unfortunately.

If you've never run kselftests before, you'll need some kernel options
to enable VRF support. From my notes I have this list below, but there
may be more missing options.

CONFIG_IP_MULTIPLE_TABLES=y
CONFIG_NET_L3_MASTER_DEV=y
CONFIG_NET_VRF=y

Let me know if you face any trouble or if I can help in some way.
Thanks for doing this.

Re: [PATCH v3 net 0/4] qbv cycle time extension/truncation

[Abdul Rahim, Faizal]

Hi Vladimir,

Sorry for the late reply, was on leave.

On 21/12/2023 9:35 pm, Vladimir Oltean wrote:
> (sorry, I started writing this email yesterday, I noticed the
> conversation continued with Paolo)
> 
> On Wed, Dec 20, 2023 at 11:25:09AM +0800, Abdul Rahim, Faizal wrote:
>> Hi Vladimir,
>>
>> No worries, I truly appreciate the time you took to review and reply.
>>
>> What prompted this in general is related to my project requirement to enable
>> software QBV cycle time extension, so there's a validation team that created
>> test cases to properly validate cycle time extension. Then I noticed the
>> code doesn't handle truncation properly also, since it's the same code area,
>> I just fixed it together.
> 
> We tend to do patch triage between 'net' and 'net-next' based on the
> balance between the urgency/impact of the fix and its complexity.
> 
> While it's undoubtable that there are issues with taprio's handling of
> dynamic schedules, you've mentioned yourself that you only hit those
> issues as part of some new development work - they weren't noticed by
> end users. And fixing them is not quite trivial, there are also FIXMEs
> in taprio which suggest so. I'm worried that the fixes may also impact
> the code from stable trees in unforeseen ways.
> 
> So I would recommend moving the development of these fixes to 'net-next',
> if possible.

Got it, will move it to net-next.

>> Each time before sending the patch for upstream review, I normally will run
>> our test cases that only validates cycle time extension. For truncation, I
>> modify the test cases on my own and put logs to check if the
>> cycle_time_correction negative value is within the correct range. I probably
>> should have mentioned sooner that I have tested this myself, sorry about
>> that.
>>
>> Example of the test I run for cycle time extension:
>> 1) 2 boards connected back-to-back with i226 NIC. Board A as sender, Board B
>> as receiver
>> 2) Time is sync between 2 boards with phc2sys and ptp4l
>> 3) Run GCL1 on Board A with cycle time extension enabled:
>>      tc qdisc replace dev $INTERFACE parent root handle 100 taprio \
>>      num_tc 4 \
>>      map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
>>      queues 1@0 1@1 1@2 1@3 \
>>      base-time 0 \
>>      cycle-time-extension 1000000 \
>>      sched-entry S 09 500000 \
>>      sched-entry S 0a 500000 \
>>      clockid CLOCK_TAI
> 
> Why do you need PTP sync? Cannot this test run between 2 veth ports?
PTP sync is probably not needed, but the test case already has it (I just 
reuse the test case), I assume it's to simulate a complete use case of a 
real user.
Let me explore testing using veth ports, haven't tried this before.

> 
>> 4) capture tcp dump on Board B
>> 5) Send packets from Board A to Board B with 200us interval via UDP Tai
> 
> What is udp_tai? This program?
> https://gist.github.com/jeez/bd3afeff081ba64a695008dd8215866f
> 

Yea the base app looks similar to the one that I use, but the one I use is 
modified. It's to transmit UDP packets.

>> 6) When packets reached Board B, trigger GCL2 to Board A:
>>     CYCLETIME=1000000
>>     APPLYTIME=1000000000 # 1s
>>     CURRENT=$(date +%s%N)
>>     BASE=$(( (CURRENT + APPLYTIME + (2*CYCLETIME)) - ((CURRENT + APPLYTIME)
>>           % CYCLETIME) + ((CYCLETIME*3)/5) ))
>>      tc qdisc replace dev $INTERFACE parent root handle 100 taprio \
>>      num_tc 4 \
>>      map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
>>      queues 1@0 1@1 1@2 1@3 \
>>      base-time $BASE \
>>      cycle-time-extension 1000000 \
>>      sched-entry S oc 500000 \
>>      sched-entry S 08 500000 \
>>      clockid CLOCK_TAI
>> 7) Analyze tcp dump data on Board B using wireshark, will observe packets
>> receive pattern changed.
>>
>> Note that I've hidden "Best Effort (default) 7001 → 7001" data from the
>> wireshark log so that it's easier to see the pattern.
>>
>>       TIMESTAMP               PRIORITY             PRIORITY    NOTES
>>
>> 1702896645.925014509	Critical Applications	7004 → 7004   GCL1
>> 1702896645.925014893	Critical Applications	7004 → 7004   GCL1
>> 1702896645.925514454	Excellent Effort	7003 → 7003   GCL1
>> 1702896645.925514835	Excellent Effort	7003 → 7003   GCL1
>> 1702896645.926014371	Critical Applications	7004 → 7004   GCL1
>> 1702896645.926014755	Critical Applications	7004 → 7004   GCL1
>> 1702896645.926514620	Excellent Effort	7003 → 7003   GCL1
>> 1702896645.926515004	Excellent Effort	7003 → 7003   GCL1
>> 1702896645.927014408	Critical Applications	7004 → 7004   GCL1
>> 1702896645.927014792	Critical Applications	7004 → 7004   GCL1
>> 1702896645.927514789	Excellent Effort	7003 → 7003   GCL1
>> 1702896645.927515173	Excellent Effort	7003 → 7003   GCL1
>> 1702896645.928168304	Excellent Effort	7003 → 7003   Extended
>> 1702896645.928368780	Excellent Effort	7003 → 7003   Extended
>> 1702896645.928569406	Excellent Effort	7003 → 7003   Extended
>> 1702896645.929614835	Background	        7002 → 7002   GCL2
>> 1702896645.929615219	Background	        7002 → 7002   GCL2
>> 1702896645.930614643	Background	        7002 → 7002   GCL2
>> 1702896645.930615027	Background	        7002 → 7002   GCL2
>> 1702896645.931614604	Background	        7002 → 7002   GCL2
>> 1702896645.931614991	Background	        7002 → 7002   GCL2
>>
>> The extended packets only will happen if cycle_time and interval fields
>> are updated using cycle_time_correction. Without that patch, the extended
>> packets are not received.
>>
>>
>> As for the negative truncation case, I just make the interval quite long,
>> and experimented with GCL2 base-time value so that it hits the "next entry"
>> in advance_sched(). Then I checked my logs in get_cycle_time_correction() to
>> see the truncation case and its values.
>>
>> Based on your feedback of the test required, I think that my existing
>> truncation test is not enough, but the extension test case part should be
>> good right ?
>>
>> Do let me know then, I'm more than willing to do more test for the
>> truncation case as per your suggestion, well basically, anything to help
>> speed up the patches series review process :)
>>
>>
>> Appreciate your suggestion and help a lot, thank you.
> 
> Do you think you could automate a test suite which only measures software
> TX timestamps and works on veth?
> 
> I prepared this very small patch set just to give you a head start
> (the skeleton). You'll still have to add the logic for individual tests.
> https://lore.kernel.org/netdev/20231221132521.2314811-1-vladimir.oltean@nxp.com/
> I'm terribly sorry, but this is the most I can do due to my current lack
> of spare time, unfortunately.
> 
> If you've never run kselftests before, you'll need some kernel options
> to enable VRF support. From my notes I have this list below, but there
> may be more missing options.
> 
> CONFIG_IP_MULTIPLE_TABLES=y
> CONFIG_NET_L3_MASTER_DEV=y
> CONFIG_NET_VRF=y
> 
> Let me know if you face any trouble or if I can help in some way.
> Thanks for doing this.

Thank you so much for helping with this self test skeleton ! I'll explore 
and continue from where you've left. Appreciate it.