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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.
> 
> > 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;
> >  	}
> >  
> 
> [ ... ]
> 
> > @@ -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.
> 
> > @@ -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;
> 
> [ ... ]
> 
> > @@ -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