Re: [PATCH net-next v9 3/7] net_sched: sch_fq: clear past skb->tstamp if offloading pacing

netdev-bot+sashiko@ wrote:

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] The two new clearing sites use different predicates, and the
  dequeue-side one folds in a schedule the offloading device never sees.
- [Low] Incomplete coverage of the new "clear an expired EDT before
  handing the skb to a pacing-offload device" logic on FQ's internal…

--

net_sched: sch_fq: clear past skb->tstamp if offloading pacing

When pacing offload is active, sch_fq now clears skb->tstamp for packets
whose EDT has already passed (or is within a 400 nsec slack window) so
the device does not have to compare the timestamp against the current
time.  The clearing is done in two places: in fq_classify() for fastpath
packets, and at the out: label of fq_dequeue() for regular flows.

A couple of questions about the coverage of the new clearing below.

quoted

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index dfc7f98b600f8..a282812c192ee 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c

@@ -159,6 +159,9 @@ struct fq_sched_data {
 	u64		stat_allocation_errors;
 };
 
+/* EDT timestamps to clear beyond now. */
+static const int fq_offload_slack_ns = 400;
+
 /* return the i-th 2-bit value ("crumb") */
 static u8 fq_prio2band(const u8 *prio2band, unsigned int prio)
 {

@@ -421,6 +424,10 @@ static struct fq_flow *fq_classify(struct Qdisc *sch, struct sk_buff *skb,
 			smp_store_release(&sk->sk_pacing_status,
 					  SK_PACING_FQ);
 
+		if (offload_horizon &&
+		    fq_skb_cb(skb)->time_to_send <= now + fq_offload_slack_ns)
+			skb_set_delivery_time(skb, 0, SKB_CLOCK_REALTIME);
+
 		return &q->internal;
 	}
 

[ ... ]

quoted

@@ -745,7 +753,7 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 	if (skb) {
 		q->internal.qlen--;
 		fq_dequeue_skb(sch, &q->internal, skb);
-		goto out;
+		return skb;
 	}
 
 	now = ktime_get_ns();

[Severity: Low]
For packets on q->internal the clearing decision is now taken only once,
at enqueue time in fq_classify(), using the enqueue-time now.  With the
goto out changed to return skb, internal-queue packets no longer reach the
new block at the out: label, and they are returned before now is even
sampled.

fq_fastpath_check() admits a packet whose txtime is up to offload_horizon
in the future:

	if (fq_skb_cb(skb)->time_to_send > now + offload_horizon)
		return false;

and the internal queue is not single-slot:

		if (q->internal.qlen >= 8)
			return false;

So a packet can be admitted with a future txtime, sit behind up to seven
others (or behind a stopped tx queue), and by the time fq_dequeue()
returns it the txtime has expired, yet skb->tstamp is still set and the
device performs exactly the future-time comparison the change is meant to
avoid.  Is that gap intentional, or should the internal-queue path also
re-test at dequeue?

Testing fast path only in fq_enqueue is intentional.
 

Relatedly, the second exit to the internal queue in fq_classify(), the
allocation failure path:

	f = kmem_cache_zalloc(fq_flow_cachep, GFP_ATOMIC | __GFP_NOWARN);
	if (unlikely(!f)) {
		q->stat_allocation_errors++;
		return &q->internal;
	}

does no clearing at all, so even an already-expired EDT survives to the
device there.  Should that path get the same treatment?

The predicate itself is conservative (nothing more than
fq_offload_slack_ns in the future is ever cleared), so this looks like a
missed optimization rather than a wrong transmit time, but the commit
message reads as though all packets whose txtime has passed get
skb->tstamp cleared.

quoted

@@ -782,8 +790,8 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 
 	skb = fq_peek(f);
 	if (skb) {
-		u64 time_next_packet = max_t(u64, fq_skb_cb(skb)->time_to_send,
-					     f->time_next_packet);
+		time_next_packet = max_t(u64, fq_skb_cb(skb)->time_to_send,
+					 f->time_next_packet);
 
 		if (now + offload_horizon < time_next_packet) {
 			head->first = f->next;

[ ... ]

quoted

@@ -862,6 +870,10 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 	}
 
 out:
+	if (offload_horizon &&
+	    time_next_packet && time_next_packet <= now + fq_offload_slack_ns)
+		skb_set_delivery_time(skb, 0, SKB_CLOCK_REALTIME);
+
 	return skb;
 }

[Severity: Medium]
Should this test the packet's own txtime rather than time_next_packet?

EDT calculations in FQ must use time_next_packet over skb->tstamp.

The fq_classify() site uses fq_skb_cb(skb)->time_to_send, while here
time_next_packet is:

		time_next_packet = max_t(u64, fq_skb_cb(skb)->time_to_send,
					 f->time_next_packet);

f->time_next_packet is FQ's own rate-pacing schedule.  Grepping
net/sched/sch_fq.c, skb->tstamp is only ever written by
fq_mono_to_skb_tstamp() on the fq_enqueue() horizon-cap path, so
f->time_next_packet is never propagated into skb->tstamp and the device
never sees it.

The effect shows up when FQ enforces its own rate, e.g.:

  tc qdisc replace dev X root fq maxrate 1gbit offload_horizon <= \
      dev->max_pacing_offload_horizon

For an EDT packet the branch that consults sk_pacing_rate is skipped:

	if (!skb->tstamp) {
		if (skb->sk)
			rate = min(READ_ONCE(skb->sk->sk_pacing_rate), rate);

so rate stays at q->flow_max_rate and the previous dequeue set
f->time_next_packet = now + len.  The flow is then throttled until
f->time_next_packet - offload_horizon, so on release:

	now + 400 < f->time_next_packet <= now + offload_horizon

and the predicate is false even though fq_skb_cb(skb)->time_to_send is
already well in the past.  In that configuration the clearing never
happens, while the fq_classify() peer site would have cleared the same
packet.  Note fq_fastpath_check() rejects the fastpath when
q->flow_max_rate != ~0UL, so maxrate flows always take this path.

Since time_next_packet is always >= time_to_send, nothing with a genuinely
future EDT gets cleared, so this looks like a silently ineffective
optimization rather than a correctness problem.  Was using

The interplay of EDT and FQ maxrate is complex. A "silently
ineffective optimization" is acceptable.

fq_skb_cb(skb)->time_to_send here considered, to keep the two sites
consistent?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910171131.2532487-1-willemdebruijn.kernel%40gmail.com