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:

quoted

From: Willem de Bruijn <willemb@google.com>

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 <anthony.l.nguyen@intel.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: Joshua A Hay <redacted>
Signed-off-by: Willem de Bruijn <willemb@google.com>

Sorry I said it looks good to human eye but *shiko brings up some
extra good points.

quoted

+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)) {
 

quoted

+	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?

quoted

+		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);

quoted

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.