Re: [PATCH net-next v5 3/6] idpf: support pacing offload

Jakub Kicinski wrote:
> On Sat,  8 Aug 2026 11:51:41 -0400 Willem de Bruijn wrote:
> > From: Willem de Bruijn <[email protected]>
> > 
> > If skb->tstamp is in the future, program this future delivery txtime
> > in the transmit descriptor.
> > 
> > TCP pacing offload is only offloaded if SK_PACING_FQ is negotiated and
> > the FQ offload_horizon is configured. But device support for pacing
> > offload must be more robust: it can also be reached through SO_TXTIME.
> > 
> > Bounds check txtime. Only packets with timestamp between now and the
> > horizon (pacing_offload_horizon) are offloaded.
> > 
> > Negotiate the feature with the device using virtchnl. Support is
> > conditional on
> > - splitq mode, where tx and tx completion queues are separate, so
> >   completions can be returned out of order.
> > - flow scheduling mode, where completions can arrive out of order.
> > - PTP to ensure the NIC clock is synced to CLOCK_TAI.
> > 
> > Do not explicitly check for these preconditions. Trust the firmware to
> > only advertise EDT when they are met. These features are negotiated
> > per adapter, but expect all vports to uniformly request splitq
> > (req_[rt]x_splitq) and flow scheduling (flow_sch_en) when available.
> > 
> > Disable if in netpoll. It does not need the feature, and the ktime
> > functions are not safe to call in this context.
> > 
> > Cc: Tony Nguyen <[email protected]>
> > Cc: Przemek Kitszel <[email protected]>
> > Cc: Joshua A Hay <[email protected]>
> > Signed-off-by: Willem de Bruijn <[email protected]>
> 
> Sorry I said it looks good to human eye but *shiko brings up some
> extra good points.
> 
> > +static void idpf_tx_splitq_set_txtime(const struct sk_buff *skb,
> > +				      struct idpf_tx_splitq_params *tx_params)
> > +{
> > +	struct idpf_netdev_priv *np = netdev_priv(skb->dev);
> > +	u64 ts, now, horizon;
> > +
> > +	horizon = READ_ONCE(skb->dev->pacing_offload_horizon);
> 
> Per *shiko, the max_pacing.. and pacing.. don't behave like the user
> may expect them to behave. FQ looks at max_pacing.. to decide whether 
> to allow configuring pacing offload. Driver looks at pacing..
> 
> Why are we letting the user create an obviously invalid configuration
> where the FQ is configured to offload but the driver is not respecting
> the requests? Since no upstream driver ever set max_pacing.. we can
> still adjust its semantics.
> 
> What do you expect the driver to use pacing_.. for?
> Currently you use it purely as a boolean but even in this case the
> semantics are unclear - is it purely a user configuration handshake
> between the driver and the qdisc to respect timestamps?

Yes this was the real regression with fq_change I referred to. That
bit got lost during revision. Patch 1 needs:

@@ -1179,7 +1179,8 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
                u64 offload_horizon = (u64)NSEC_PER_USEC *
                                      nla_get_u32(tb[TCA_FQ_OFFLOAD_HORIZON]);
 
-               if (offload_horizon <= qdisc_dev(sch)->max_pacing_offload_horizon) {
+               if (offload_horizon <=
+                   READ_ONCE(qdisc_dev(sch)->pacing_offload_horizon)) {
 
> > +	if (!horizon)
> > +		return;
> > +
> > +	/* Skip if netpoll: not needed and not safe to call ktime helpers */
> > +	if (netpoll_tx_running(skb->dev))
> 
> Is this due to Gemini's complaint? netpoll sending packets with
> timestamp in the future seems unreasonable to me, no?

It is. Makes sense to ignore EDT request when running in netpoll right.
Do you mean that we should not even check for that in the hot path?

> 
> > +		return;
> > +
> > +	switch (skb->tstamp_type) {
> > +	case SKB_CLOCK_REALTIME:
> > +		ts = ktime_to_ns(ktime_add(skb->tstamp,
> > +					   ktime_mono_to_any(0, TK_OFFS_TAI) -
> > +					   ktime_mono_to_any(0, TK_OFFS_REAL)));
> > +		break;
> > +	case SKB_CLOCK_MONOTONIC:
> > +		ts = ktime_to_ns(ktime_mono_to_any(skb->tstamp, TK_OFFS_TAI));
> > +		break;
> > +	case SKB_CLOCK_TAI:
> > +		ts = ktime_to_ns(skb->tstamp);
> > +		break;
> > +	default:
> > +		WARN_ON_ONCE(1);
> > +		return;
> > +	}
> > +
> > +	now = ktime_get_clocktai_ns();
> > +	if (ts < now)
> > +		return;
> 
> makes me wonder if FQ should clear the timestamps for e.g. now + 100ns ?
> IOW I wonder how often we end up taking this exit?

I'll add such a slack value to now in

+       if (q->offload_horizon &&
+           time_next_packet && time_next_packet <= now)
+               __skb_clear_delivery_time(skb, false);

> > diff --git a/drivers/net/ethernet/intel/idpf/virtchnl2.h b/drivers/net/ethernet/intel/idpf/virtchnl2.h
> > index 39fea65c075c..7525146491cd 100644
> > --- a/drivers/net/ethernet/intel/idpf/virtchnl2.h
> > +++ b/drivers/net/ethernet/intel/idpf/virtchnl2.h
> > @@ -457,6 +457,16 @@ struct virtchnl2_edt_caps {
> >  };
> >  VIRTCHNL2_CHECK_STRUCT_LEN(16, virtchnl2_edt_caps);
> >  
> > +/**
> > + * struct virtchnl2_edt_caps_ilog2 - Host parsed EDT caps.
> > + * @time_horizon_ns: Total time window in nanoseconds.
> > + * @tstamp_granularity_pow2: Log2 of timestamp granularity in nanoseconds.
> > + */
> > +struct virtchnl2_edt_caps_ilog2 {
> > +	u32 time_horizon_ns;
> > +	u8 tstamp_granularity_pow2;
> > +};
> 
> *shiko:
> 
> This isn't a bug, but this struct is host internal state: native u32/u8
> fields, no __le types, no VIRTCHNL2_CHECK_STRUCT_LEN assertion, and its only
> user is the edt_caps member of struct idpf_adapter. Every other struct in
> virtchnl2.h mirrors the control plane wire layout and carries a size
> assertion, as the neighbouring virtchnl2_edt_caps does.
> 
> Would idpf.h be a better home for it, with a name that does not carry the
> virtchnl2_ prefix, so a future firmware spec sync does not mistake it for a
> message struct?

Will do.