[PATCH net-next v9 0/7] hardware pacing offload

[PATCH net-next v9 0/7] hardware pacing offload

[Willem de Bruijn]

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

Enable safe hardware pacing offload on a first device (idpf), plus support.

- Add rtnetlink support for admin control over the device feature
- Add device support to the idpf driver

Besides the main feature, also

- Small optimization to FQ offload: avoid unnecessary ktime_.._get().
- Expand so_txtime drv-net test with hw offload testcases.

Details and detailed changelog in the individual patches.

High level changes:
  v8 -> v9
    - graceful fallback to sw pacing if hw pacing becomes unavailable
        - add new FQ patch for this fq_offload_horizon() check
    - simplify dev->pacing_offload_horizon to bool dev->pacing_offload
  v7 -> v8
    - 2 small fixes: unused variable in idpf and new ruff warning in test
  v6 -> v7
    - Rebase onto conflicting patches and address feedback
  v5 -> v6
    - Address substantial opus 5 feedback
  v4 -> v5
    - Rebase + small kdoc fix only
  v3 -> v4
    - Replace ethtool API with rtnetlink
    - Drop/defer pktgen patch: not essential

v8: https://lore.kernel.org/netdev/20260902181747.2483351-1-willemdebruijn.kernel@gmail.com/
v7: https://lore.kernel.org/netdev/20260831213221.1370774-1-willemdebruijn.kernel@gmail.com/
v6: https://lore.kernel.org/netdev/20260813020529.3506539-1-willemdebruijn.kernel@gmail.com/
v5: https://lore.kernel.org/netdev/20260808155217.885299-1-willemdebruijn.kernel@gmail.com/
v4: https://lore.kernel.org/netdev/20260806232913.4020403-1-willemdebruijn.kernel@gmail.com/
v3: https://lore.kernel.org/netdev/20260726202902.760552-1-willemdebruijn.kernel@gmail.com/
v2: https://lore.kernel.org/netdev/20260722204454.3234605-1-willemdebruijn.kernel@gmail.com/
v1: https://lore.kernel.org/netdev/20260706133433.3142805-1-willemdebruijn.kernel@gmail.com/
Willem de Bruijn (7):
  net: rtnetlink: add pacing_offload attribute to net_device
  net_sched: sch_fq: check device pacing offload
  net_sched: sch_fq: clear past skb->tstamp if offloading pacing
  idpf: support pacing offload
  selftests: drv-net: refactor so_txtime errqueue handling
  selftests: drv-net: in so_txtime tell apart sw from hw pacing
  selftests: drv-net: extend so_txtime with hw offload

 Documentation/netlink/specs/rt-link.yaml      |   5 +
 .../networking/net_cachelines/net_device.rst  |   1 +
 drivers/net/ethernet/intel/idpf/idpf.h        |  12 ++
 drivers/net/ethernet/intel/idpf/idpf_lib.c    |  19 +++
 drivers/net/ethernet/intel/idpf/idpf_txrx.c   |  69 +++++++-
 drivers/net/ethernet/intel/idpf/idpf_txrx.h   |   8 +-
 .../net/ethernet/intel/idpf/idpf_virtchnl.c   |  60 ++++++-
 include/linux/netdevice.h                     |   2 +
 include/uapi/linux/if_link.h                  |   1 +
 net/core/rtnetlink.c                          |  22 +++
 net/sched/sch_fq.c                            |  71 ++++++--
 .../testing/selftests/drivers/net/so_txtime.c | 159 +++++++++++++-----
 .../selftests/drivers/net/so_txtime.py        |  80 ++++++++-
 13 files changed, 443 insertions(+), 66 deletions(-)

-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net-next v9 1/7] net: rtnetlink: add pacing_offload attribute to net_device

[Willem de Bruijn]

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

Add a new flag to administratively control pacing offload.

The feature is disabled by default. That does not cause regressions, as no
driver advertises max_pacing_offload_horizon yet.

Also add NLA_REJECT for max_pacing_offload_horizon, in line with
other such read-only members of link-attrs.

Both fields can be read with

    ynl --family rt-link --do getlink \
        --json '{"ifname": "eth0"}' | grep pacing

And pacing offload enabled with

    ynl --family rt-link --do setlink \
        --json '{"ifname": "eth0", "pacing-offload": 1}'

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Changes
  v8 -> v9
    - rename pacing_offload_horizon to pacing_offload and make it boolean
    - leave max_pacing_offload_horizon as is (u64)
    - (minor) update ynl instructions
  v6 -> v7
    - commit-msg: update ynl invocation to installed version
  v5 -> v6
    - bring back fq_change update
    - commit-msg: reword absense of RTM_NEWLINK -> omitted from newlink
---
 Documentation/netlink/specs/rt-link.yaml      |  5 +++++
 .../networking/net_cachelines/net_device.rst  |  1 +
 include/linux/netdevice.h                     |  2 ++
 include/uapi/linux/if_link.h                  |  1 +
 net/core/rtnetlink.c                          | 22 +++++++++++++++++++
 5 files changed, 31 insertions(+)

diff --git a/Documentation/netlink/specs/rt-link.yaml b/Documentation/netlink/specs/rt-link.yaml
index 61ebb9a2bad5..d0559293b3b3 100644
--- a/Documentation/netlink/specs/rt-link.yaml
+++ b/Documentation/netlink/specs/rt-link.yaml
@@ -1089,6 +1089,10 @@ attribute-sets:
       -
         name: tailroom
         type: u16
+      -
+        name: pacing-offload
+        type: u32
+        doc: Enable EDT pacing offload (0 - disabled, 1 - enabled).
   -
     name: prop-list-link-attrs
     subset-of: link-attrs
@@ -2557,6 +2561,7 @@ operations:
             - devlink-port
             - gso-ipv4-max-size
             - gro-ipv4-max-size
+            - pacing-offload
       dump:
         request:
           value: 18
diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
index 512f6d6fa3d8..8eceaa80b686 100644
--- a/Documentation/networking/net_cachelines/net_device.rst
+++ b/Documentation/networking/net_cachelines/net_device.rst
@@ -11,6 +11,7 @@ Type                                Name                        fastpath_tx_acce
 unsigned_long:32                    priv_flags                  read_mostly                             __dev_queue_xmit(tx)
 unsigned_long:1                     lltx                        read_mostly                             HARD_TX_LOCK,HARD_TX_TRYLOCK,HARD_TX_UNLOCK(tx)
 unsigned_long:2                     netmem_tx:2;                read_mostly
+unsigned_long:1                     pacing_offload              read_mostly                             sch_fq
 char                                name[16]
 struct netdev_name_node*            name_node
 struct dev_ifalias*                 ifalias
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 707b2e51c2b9..1f0710eef185 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1840,6 +1840,7 @@ enum netdev_reg_state {
  *			drivers. Mainly used by logical interfaces, such as
  *			bonding and tunnels
  *	@netmem_tx:	device netmem TX mode
+ *	@pacing_offload: enable EDT pacing offload.
  *
  *	@name:	This is the first field of the "visible" part of this structure
  *		(i.e. as seen by users in the "Space.c" file).  It is the name
@@ -2170,6 +2171,7 @@ struct net_device {
 		unsigned long		priv_flags:32;
 		unsigned long		lltx:1;
 		unsigned long		netmem_tx:2;
+		unsigned long		pacing_offload:1;
 	);
 	const struct net_device_ops *netdev_ops;
 	const struct header_ops *header_ops;
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 43cecca49f01..245b36204525 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -381,6 +381,7 @@ enum {
 	IFLA_NETNS_IMMUTABLE,
 	IFLA_HEADROOM,
 	IFLA_TAILROOM,
+	IFLA_PACING_OFFLOAD,
 	__IFLA_MAX
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 81c5a6104dea..aeccfa814f40 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1396,6 +1396,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + rtnl_devlink_port_size(dev)
 	       + rtnl_dpll_pin_size()
 	       + nla_total_size(8)  /* IFLA_MAX_PACING_OFFLOAD_HORIZON */
+	       + nla_total_size(4)  /* IFLA_PACING_OFFLOAD */
 	       + nla_total_size(2)  /* IFLA_HEADROOM */
 	       + nla_total_size(2)  /* IFLA_TAILROOM */
 	       + rtnl_dev_parent_size(dev)
@@ -2176,6 +2177,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 			READ_ONCE(dev->tso_max_segs)) ||
 	    nla_put_uint(skb, IFLA_MAX_PACING_OFFLOAD_HORIZON,
 			 READ_ONCE(dev->max_pacing_offload_horizon)) ||
+	    nla_put_u32(skb, IFLA_PACING_OFFLOAD,
+			dev->pacing_offload) ||
 #ifdef CONFIG_RPS
 	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES,
 			READ_ONCE(dev->num_rx_queues)) ||
@@ -2344,9 +2347,11 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_ALLMULTI]		= { .type = NLA_REJECT },
 	[IFLA_GSO_IPV4_MAX_SIZE]	= NLA_POLICY_MIN(NLA_U32, MAX_TCP_HEADER + 1),
 	[IFLA_GRO_IPV4_MAX_SIZE]	= { .type = NLA_U32 },
+	[IFLA_MAX_PACING_OFFLOAD_HORIZON] = { .type = NLA_REJECT },
 	[IFLA_NETNS_IMMUTABLE]	= { .type = NLA_REJECT },
 	[IFLA_HEADROOM]		= { .type = NLA_REJECT },
 	[IFLA_TAILROOM]		= { .type = NLA_REJECT },
+	[IFLA_PACING_OFFLOAD]	= NLA_POLICY_MAX(NLA_U32, 1),
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -2820,6 +2825,14 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
 		return -EINVAL;
 	}
 
+	if (tb[IFLA_PACING_OFFLOAD]) {
+		if (nla_get_u32(tb[IFLA_PACING_OFFLOAD]) &&
+		    !dev->max_pacing_offload_horizon) {
+			NL_SET_ERR_MSG(extack, "pacing offload not supported by device");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	if (tb[IFLA_AF_SPEC]) {
 		struct nlattr *af;
 		int rem, err;
@@ -3337,6 +3350,15 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
+	if (tb[IFLA_PACING_OFFLOAD]) {
+		bool val = nla_get_u32(tb[IFLA_PACING_OFFLOAD]);
+
+		if (dev->pacing_offload != val) {
+			dev->pacing_offload = val;
+			status |= DO_SETLINK_MODIFIED;
+		}
+	}
+
 	if (tb[IFLA_OPERSTATE])
 		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
 
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net-next v9 2/7] net_sched: sch_fq: check device pacing offload

[Willem de Bruijn]

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

Network devices can modify pacing offload administratively or device
renegotiation.

In FQ, only offload if the feature is enabled and within the supported
device horizon.

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/sched/sch_fq.c | 53 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index 6144b5686f13..fdd9380e445c 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -301,6 +301,26 @@ static void fq_gc(struct fq_sched_data *q,
 	q->stat_gc_flows += fcnt;
 }
 
+static u64 fq_offload_horizon(const struct Qdisc *sch,
+			      const struct fq_sched_data *q)
+{
+	const struct net_device *dev;
+	u64 offload_horizon;
+
+	offload_horizon = READ_ONCE(q->offload_horizon);
+	if (!offload_horizon)
+		return 0;
+
+	dev = qdisc_dev(sch);
+	if (!dev->pacing_offload)
+		return 0;
+
+	if (offload_horizon > READ_ONCE(dev->max_pacing_offload_horizon))
+		return 0;
+
+	return offload_horizon;
+}
+
 /* Fast path can be used if :
  * 1) Packet tstamp is in the past, or within the pacing offload horizon.
  * 2) FQ qlen == 0   OR
@@ -312,12 +332,12 @@ static void fq_gc(struct fq_sched_data *q,
  * FQ can not use generic TCQ_F_CAN_BYPASS infrastructure.
  */
 static bool fq_fastpath_check(const struct Qdisc *sch, struct sk_buff *skb,
-			      u64 now)
+			      u64 now, u64 offload_horizon)
 {
 	const struct fq_sched_data *q = qdisc_priv(sch);
 	const struct sock *sk;
 
-	if (fq_skb_cb(skb)->time_to_send > now + q->offload_horizon)
+	if (fq_skb_cb(skb)->time_to_send > now + offload_horizon)
 		return false;
 
 	if (sch->q.qlen != 0) {
@@ -338,7 +358,7 @@ static bool fq_fastpath_check(const struct Qdisc *sch, struct sk_buff *skb,
 		/* Ordering invariants fall apart if some delayed flows
 		 * are ready but we haven't serviced them, yet.
 		 */
-		if (q->time_next_delayed_flow <= now + q->offload_horizon)
+		if (q->time_next_delayed_flow <= now + offload_horizon)
 			return false;
 	}
 
@@ -357,6 +377,7 @@ static struct fq_flow *fq_classify(struct Qdisc *sch, struct sk_buff *skb,
 				   u64 now)
 {
 	struct fq_sched_data *q = qdisc_priv(sch);
+	u64 offload_horizon = fq_offload_horizon(sch, q);
 	struct rb_node **p, *parent;
 	struct sock *sk = skb->sk;
 	struct rb_root *root;
@@ -393,12 +414,13 @@ static struct fq_flow *fq_classify(struct Qdisc *sch, struct sk_buff *skb,
 		sk = (struct sock *)((hash << 1) | 1UL);
 	}
 
-	if (fq_fastpath_check(sch, skb, now)) {
+	if (fq_fastpath_check(sch, skb, now, offload_horizon)) {
 		q->internal.stat_fastpath_packets++;
 		if (skb->sk == sk && q->rate_enable &&
 		    READ_ONCE(sk->sk_pacing_status) != SK_PACING_FQ)
 			smp_store_release(&sk->sk_pacing_status,
 					  SK_PACING_FQ);
+
 		return &q->internal;
 	}
 
@@ -661,12 +683,13 @@ static int fq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	return NET_XMIT_SUCCESS;
 }
 
-static void fq_check_throttled(struct fq_sched_data *q, u64 now)
+static void fq_check_throttled(struct fq_sched_data *q, u64 now,
+			       u64 offload_horizon)
 {
 	unsigned long sample;
 	struct rb_node *p;
 
-	if (q->time_next_delayed_flow > now + q->offload_horizon)
+	if (q->time_next_delayed_flow > now + offload_horizon)
 		return;
 
 	/* Update unthrottle latency EWMA.
@@ -677,7 +700,7 @@ static void fq_check_throttled(struct fq_sched_data *q, u64 now)
 		q->unthrottle_latency_ns -= q->unthrottle_latency_ns >> 3;
 		q->unthrottle_latency_ns += sample >> 3;
 	}
-	now += q->offload_horizon;
+	now += offload_horizon;
 
 	q->time_next_delayed_flow = ~0ULL;
 	while ((p = rb_first(&q->delayed)) != NULL) {
@@ -705,6 +728,7 @@ static struct fq_flow_head *fq_pband_head_select(struct fq_perband_flows *pband)
 static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 {
 	struct fq_sched_data *q = qdisc_priv(sch);
+	u64 offload_horizon = fq_offload_horizon(sch, q);
 	struct fq_perband_flows *pband;
 	struct fq_flow_head *head;
 	struct sk_buff *skb;
@@ -725,7 +749,7 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 	}
 
 	now = ktime_get_ns();
-	fq_check_throttled(q, now);
+	fq_check_throttled(q, now, offload_horizon);
 	retry = 0;
 	pband = &q->band_flows[q->band_nr];
 begin:
@@ -761,7 +785,7 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 		u64 time_next_packet = max_t(u64, fq_skb_cb(skb)->time_to_send,
 					     f->time_next_packet);
 
-		if (now + q->offload_horizon < time_next_packet) {
+		if (now + offload_horizon < time_next_packet) {
 			head->first = f->next;
 			f->time_next_packet = time_next_packet;
 			fq_flow_set_throttled(q, f);
@@ -836,6 +860,7 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 		}
 		f->time_next_packet = now + len;
 	}
+
 out:
 	return skb;
 }
@@ -1183,11 +1208,15 @@ 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) {
-			WRITE_ONCE(q->offload_horizon, offload_horizon);
-		} else {
+		if (offload_horizon && !qdisc_dev(sch)->pacing_offload) {
+			NL_SET_ERR_MSG_MOD(extack, "device pacing offload is disabled");
+			err = -EINVAL;
+		} else if (offload_horizon >
+			   qdisc_dev(sch)->max_pacing_offload_horizon) {
 			NL_SET_ERR_MSG_MOD(extack, "invalid offload_horizon");
 			err = -EINVAL;
+		} else {
+			WRITE_ONCE(q->offload_horizon, offload_horizon);
 		}
 	}
 	if (!err) {
-- 
2.55.0.1007.g17ff1f9808-goog

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

[Willem de Bruijn]

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

When hardware offload is enabled, FQ will forward packets to the
netdevice for pacing. The device has to test that skb->tstamp is
in the future.

Avoid this cost for packets whose txtime has already passed, by
clearing skb->tstamp.

Also clear slightly into the future, for EDT timestamps that are
so close to now that they fall within a reasonable normal Tx
latency. This slack is set to 400 nsec.

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Changes
  v8 -> v9
    - (minor) use SKB_CLOCK_REALTIME instead of 0
    - (minor) remove comments to bot below the --- separator
  v6 -> v7
    - timer drift logic fix moved to separate commit
  v5 -> v6
    - switch to skb_set_delivery_time to also clear SKB_CLOCK_REALTIME
    - add fq_offload_slack_ns
  v3 -> v4
    - also reset tstamp_type
    - minor: initialize time_next_packet for more obvious correctness
---
 net/sched/sch_fq.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index fdd9380e445c..60ad87cb1275 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;
 	}
 
@@ -731,6 +738,7 @@ static struct sk_buff *fq_dequeue(struct Qdisc *sch)
 	u64 offload_horizon = fq_offload_horizon(sch, q);
 	struct fq_perband_flows *pband;
 	struct fq_flow_head *head;
+	u64 time_next_packet = 0;
 	struct sk_buff *skb;
 	struct fq_flow *f;
 	unsigned long rate;
@@ -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();
@@ -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;
 }
 
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net-next v9 4/7] idpf: support pacing offload

[Willem de Bruijn]

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 (max_pacing_offload_horizon) are offloaded when pacing offload
is enabled on the device via pacing_offload.

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.

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.

Packets beyond the horizon are sent immediately with the overflow bit
set.

On device reset, EDT capabilities are re-negotiated with firmware.
If re-negotiation succeeds, dev->max_pacing_offload_horizon is refreshed.
If pacing offload is no longer supported, dev->max_pacing_offload_horizon is set to 0.

Must not be called from netpoll due to ktime_get. But netpoll does not
generate packets with EDT, so no explicit test is needed.

Do not fail device initialization on EDT init error. Log an error, but
continue without EDT, similar to PTP.

Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: Joshua A Hay <joshua.a.hay@intel.com>
Cc: intel-wired-lan@lists.osuosl.org
Cc: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Changes
  v8 -> v9
    - on device reset, re-evaluate and refresh pacing horizons
    - check boolean pacing_offload and use max_pacing_offload_horizon
    - update commit message regarding reset re-negotiation
    - small typo in changelog
  v7 -> v8
    - remove unused variable np in idpf_tx_splitq_set_txtime
  v6 -> v7
    - rebase onto libie changes: adjust idpf_send_get_edt_caps_msg
    - only set horizon if splitq with flow scheduling
    - fail on illegal granularity 0, rather than use hardcoded default
    - zero edt_caps on negotation error, in case of call after reset
    - cache ts_gran_pow2 in idpf_tx_queue to be in hot cache line
    - similar to FQ, add offload slack to avoid pacing < 400ns of now
    - remove now superfluous include netpoll.h
    - reverse xmas tree (1x)
  v5 -> v6
    - move EDT caps from virtchnl2_edt_caps_ilog2 to idpf_edt_caps_ilog2
    - do not fail device init if EDT init failed, same as PTP
    - replace netpoll branch with commit-msg comment
    - add commit-msg comment about device reset
    - add commit-msg about behavior beyond horizon
    - add commit-msg about trusting EDT capability response
  v4 -> v5
    - fix kdoc: idpf.h:738 struct member 'edt_caps' not described..
  v3 -> v4
    - add EDT virtchnl negotiation
    - move endianness fix to its own iwl-net patch
    - update commit-msg: check is against pacing_offload_horizon, not max_..
    - simplify assignment, avoid the le32 assignment in favor of u8 inits
    - replace open coded constant with IDPF_TXD_FLOW_SCH_HORIZON_OVERFLOW_M
    - (minor) fix 24b timestamp comment: is 23b + overflow bit
    - (minor) remove unused parameter
  v2 -> v3
    - use READ_ONCE and read pacing_offload_horizon only once
  v1 -> v2
    - move special zero case up and return early
---
 drivers/net/ethernet/intel/idpf/idpf.h        | 12 ++++
 drivers/net/ethernet/intel/idpf/idpf_lib.c    | 19 +++++
 drivers/net/ethernet/intel/idpf/idpf_txrx.c   | 69 ++++++++++++++++++-
 drivers/net/ethernet/intel/idpf/idpf_txrx.h   |  8 ++-
 .../net/ethernet/intel/idpf/idpf_virtchnl.c   | 60 +++++++++++++++-
 5 files changed, 165 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
index 470bc23c844c..f214023095ee 100644
--- a/drivers/net/ethernet/intel/idpf/idpf.h
+++ b/drivers/net/ethernet/intel/idpf/idpf.h
@@ -604,6 +604,16 @@ struct idpf_vport_config {
 	DECLARE_BITMAP(flags, IDPF_VPORT_CONFIG_FLAGS_NBITS);
 };
 
+/**
+ * struct idpf_edt_caps_ilog2 - Host parsed EDT capabilities.
+ * @time_horizon_ns: Total time window in nanoseconds.
+ * @tstamp_granularity_pow2: Log2 of timestamp granularity in nanoseconds.
+ */
+struct idpf_edt_caps_ilog2 {
+	u32 time_horizon_ns;
+	u8 tstamp_granularity_pow2;
+};
+
 #define idpf_for_each_vport(adapter, iter) \
 	for (struct idpf_vport **__##iter = &(adapter)->vports[0], \
 	     *iter = (adapter)->max_vports ? *__##iter : NULL; \
@@ -657,6 +667,7 @@ struct idpf_vport_config {
  * @stats_task: Periodic statistics retrieval task
  * @stats_wq: Workqueue for statistics task
  * @caps: Negotiated capabilities with device
+ * @edt_caps: Negotiated EDT capabilities with device
  * @dev_ops: See idpf_dev_ops
  * @cdev_info: IDC core device info pointer
  * @num_vfs: Number of allocated VFs through sysfs. PF does not directly talk
@@ -720,6 +731,7 @@ struct idpf_adapter {
 	struct delayed_work stats_task;
 	struct workqueue_struct *stats_wq;
 	struct virtchnl2_get_capabilities caps;
+	struct idpf_edt_caps_ilog2 edt_caps;
 
 	struct idpf_dev_ops dev_ops;
 	struct iidc_rdma_core_dev_info *cdev_info;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 827c795afcb6..cd1b173e3b27 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -817,6 +817,21 @@ static void idpf_attach_and_open(struct idpf_adapter *adapter)
 	}
 }
 
+static void idpf_vport_set_pacing_offload(struct idpf_vport *vport,
+					  struct net_device *netdev)
+{
+	struct idpf_adapter *adapter = vport->adapter;
+	u32 max_horizon = 0;
+
+	if (adapter->edt_caps.time_horizon_ns &&
+	    idpf_is_queue_model_split(vport->dflt_qv_rsrc.txq_model) &&
+	    !idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
+			     VIRTCHNL2_CAP_SPLITQ_QSCHED))
+		max_horizon = adapter->edt_caps.time_horizon_ns;
+
+	WRITE_ONCE(netdev->max_pacing_offload_horizon, max_horizon);
+}
+
 /**
  * idpf_cfg_netdev - Allocate, configure and register a netdev
  * @vport: main vport structure
@@ -850,6 +865,8 @@ static int idpf_cfg_netdev(struct idpf_vport *vport)
 		np->max_tx_hdr_size = idpf_get_max_tx_hdr_size(adapter);
 		vport->netdev = netdev;
 
+		idpf_vport_set_pacing_offload(vport, netdev);
+
 		return idpf_init_mac_addr(vport, netdev);
 	}
 
@@ -890,6 +907,8 @@ static int idpf_cfg_netdev(struct idpf_vport *vport)
 	netdev->min_mtu = ETH_MIN_MTU;
 	netdev->max_mtu = vport->max_mtu;
 
+	idpf_vport_set_pacing_offload(vport, netdev);
+
 	dflt_features = NETIF_F_SG	|
 			NETIF_F_HIGHDMA;
 
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index 24b91be25676..97e23ff9faf6 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -1742,6 +1742,7 @@ static int idpf_txq_group_alloc(struct idpf_vport *vport,
 			q->desc_count = rsrc->txq_desc_count;
 			q->tx_max_bufs = idpf_get_max_tx_bufs(adapter);
 			q->tx_min_pkt_len = idpf_get_min_tx_pkt_len(adapter);
+			q->ts_gran_pow2 = adapter->edt_caps.tstamp_granularity_pow2;
 			q->netdev = vport->netdev;
 			q->txq_grp = tx_qgrp;
 			q->rel_q_id = j;
@@ -2408,7 +2409,12 @@ void idpf_tx_splitq_build_flow_desc(union idpf_tx_flex_desc *desc,
 				    struct idpf_tx_splitq_params *params,
 				    u16 td_cmd, u16 size)
 {
-	*(__le32 *)&desc->flow.qw1.cmd_dtype = cpu_to_le32((u8)(params->dtype | td_cmd));
+	desc->flow.qw1.cmd_dtype = (u8)(params->dtype | td_cmd);
+
+	desc->flow.qw1.ts[0] = params->offload.desc_ts[0];
+	desc->flow.qw1.ts[1] = params->offload.desc_ts[1];
+	desc->flow.qw1.ts[2] = params->offload.desc_ts[2];
+
 	desc->flow.qw1.rxr_bufsize = cpu_to_le16((u16)size);
 	desc->flow.qw1.compl_tag = cpu_to_le16(params->compl_tag);
 }
@@ -3011,6 +3017,63 @@ static bool idpf_tx_splitq_need_re(struct idpf_tx_queue *tx_q)
 	return gap >= IDPF_TX_SPLITQ_RE_MIN_GAP;
 }
 
+static void idpf_tx_splitq_set_txtime(const struct sk_buff *skb,
+				      const struct idpf_tx_queue *tx_q,
+				      struct idpf_tx_splitq_params *tx_params)
+{
+	const int offload_slack_ns = 400;
+	u64 ts, now, horizon;
+
+	if (!tx_q->netdev->pacing_offload)
+		return;
+
+	horizon = READ_ONCE(tx_q->netdev->max_pacing_offload_horizon);
+	if (!horizon)
+		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 + offload_slack_ns)
+		return;
+
+	/* beyond offload horizon? set overflow bit only */
+	if (ts > now + horizon) {
+		tx_params->offload.desc_ts[2] =
+			IDPF_TXD_FLOW_SCH_HORIZON_OVERFLOW_M;
+		return;
+	}
+
+	ts >>= tx_q->ts_gran_pow2;
+
+	/* 0 is valid 23b timestamp, but also means field unset.
+	 * Increase by one to avoid this case
+	 */
+	if ((ts & 0x7fffff) == 0) {
+		tx_params->offload.desc_ts[0] = 1;
+		return;
+	}
+
+	tx_params->offload.desc_ts[0] = ts & 0xff;
+	tx_params->offload.desc_ts[1] = (ts >> 8) & 0xff;
+	tx_params->offload.desc_ts[2] = ((ts >> 16) & 0x7f);
+}
+
 /**
  * idpf_tx_splitq_frame - Sends buffer on Tx ring using flex descriptors
  * @skb: send buffer
@@ -3097,6 +3160,10 @@ static netdev_tx_t idpf_tx_splitq_frame(struct sk_buff *skb,
 
 		tx_params.dtype = IDPF_TX_DESC_DTYPE_FLEX_FLOW_SCHE;
 		tx_params.eop_cmd = IDPF_TXD_FLEX_FLOW_CMD_EOP;
+
+		if (skb->tstamp)
+			idpf_tx_splitq_set_txtime(skb, tx_q, &tx_params);
+
 		/* Set the RE bit periodically to "clean" the descriptor ring */
 		if (idpf_tx_splitq_need_re(tx_q)) {
 			tx_params.eop_cmd |= IDPF_TXD_FLEX_FLOW_CMD_RE;
diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
index 93547597efd2..fe0c913f9bb9 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
@@ -161,6 +161,7 @@ union idpf_tx_flex_desc {
  * @tso_segs: Number of segments to be sent
  * @tso_hdr_len: Length of headers to be duplicated
  * @td_cmd: Command field to be inserted into descriptor
+ * @desc_ts: Flow scheduling offload timestamp
  */
 struct idpf_tx_offload_params {
 	u32 tx_flags;
@@ -174,6 +175,7 @@ struct idpf_tx_offload_params {
 	u16 tso_hdr_len;
 
 	u16 td_cmd;
+	u8 desc_ts[3];
 };
 
 /**
@@ -608,6 +610,7 @@ libeth_cacheline_set_assert(struct idpf_rx_queue,
  *	 hot path TX pointers stored in vport. Used in both singleq/splitq.
  * @desc_count: Number of descriptors
  * @tx_min_pkt_len: Min supported packet length
+ * @ts_gran_pow2: Txtime timestamp granularity in nanoseconds (log2).
  * @thresh: XDP queue cleaning threshold
  * @netdev: &net_device corresponding to this queue
  * @next_to_use: Next descriptor to use
@@ -666,7 +669,10 @@ struct idpf_tx_queue {
 	u16 desc_count;
 
 	union {
-		u16 tx_min_pkt_len;
+		struct {
+			u16 tx_min_pkt_len;
+			u8 ts_gran_pow2;
+		};
 		u32 thresh;
 	};
 
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 1caf52706973..928de868fa62 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -3,6 +3,7 @@
 
 #include <linux/export.h>
 #include <linux/net/intel/libie/pci.h>
+#include <linux/log2.h>
 #include <net/libeth/rx.h>
 
 #include "idpf.h"
@@ -553,7 +554,8 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
 			    VIRTCHNL2_CAP_SPLITQ_QSCHED		|
 			    VIRTCHNL2_CAP_PROMISC		|
 			    VIRTCHNL2_CAP_LOOPBACK		|
-			    VIRTCHNL2_CAP_PTP);
+			    VIRTCHNL2_CAP_PTP			|
+			    VIRTCHNL2_CAP_EDT);
 
 	err = idpf_send_mb_msg_stack(adapter, &xn_params, &caps);
 	if (err)
@@ -573,6 +575,54 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
 	return err;
 }
 
+/**
+ * idpf_send_get_edt_caps_msg - Send virtchnl get EDT caps msg
+ * @adapter: Driver specific private struct
+ *
+ * Return: 0 on success or error code on failure.
+ */
+static int idpf_send_get_edt_caps_msg(struct idpf_adapter *adapter)
+{
+	struct libie_ctlq_xn_send_params xn_params = {
+		.timeout_ms	= IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC,
+		.chnl_opcode	= VIRTCHNL2_OP_GET_EDT_CAPS,
+	};
+	struct virtchnl2_edt_caps caps = {};
+	u64 gran_ns, horizon_ns;
+	int err;
+
+	err = idpf_send_mb_msg_stack(adapter, &xn_params, &caps);
+	if (err)
+		return err;
+
+	if (xn_params.recv_mem.iov_len < sizeof(caps)) {
+		err = -EIO;
+		goto free_rx_buf;
+	}
+
+	memcpy(&caps, xn_params.recv_mem.iov_base, sizeof(caps));
+	horizon_ns = le64_to_cpu(caps.time_horizon_ns);
+	gran_ns = le64_to_cpu(caps.tstamp_granularity_ns);
+	if (horizon_ns > U32_MAX) {
+		dev_warn(&adapter->pdev->dev, "EDT horizon exceeds U32\n");
+		err = -EINVAL;
+		goto free_rx_buf;
+	}
+	if (!gran_ns || !is_power_of_2(gran_ns)) {
+		dev_warn(&adapter->pdev->dev, "Invalid EDT granularity\n");
+		err = -EINVAL;
+		goto free_rx_buf;
+	}
+
+	adapter->edt_caps.time_horizon_ns = horizon_ns;
+	adapter->edt_caps.tstamp_granularity_pow2 = ilog2(gran_ns);
+
+free_rx_buf:
+	libie_ctlq_release_rx_buf(&xn_params.recv_mem);
+
+	return err;
+}
+
 /**
  * idpf_mmio_region_non_static - Check if region is not static
  * @mmio_info: PCI resources info
@@ -3085,6 +3135,14 @@ int idpf_vc_core_init(struct idpf_adapter *adapter)
 		}
 	}
 
+	memset(&adapter->edt_caps, 0, sizeof(adapter->edt_caps));
+	if (idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS, VIRTCHNL2_CAP_EDT)) {
+		err = idpf_send_get_edt_caps_msg(adapter);
+		if (err)
+			dev_err(&adapter->pdev->dev,
+				"EDT init failed, err=%d\n", err);
+	}
+
 	pci_sriov_set_totalvfs(adapter->pdev, idpf_get_max_vfs(adapter));
 	num_max_vports = idpf_get_max_vports(adapter);
 	adapter->vports = kzalloc_objs(*adapter->vports, num_max_vports);
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net-next v9 5/7] selftests: drv-net: refactor so_txtime errqueue handling

[Willem de Bruijn]

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

Prepare error queue handling for upcoming SO_EE_ORIGIN_TIMESTAMPING
messages in the next patch in this series.

Convert do_recv_errqueue_timeout into dispatcher do_recv_errqueue
and move SO_EE_ORIGIN_TXTIME specific code into a separate helper.

This will make the next patch a lot more readable.

No functional changes.

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Changes
  v8 -> v9
    - fix separator (---)
  v3 -> v4
    - rename func to do_recv_errqueue_txtime
---
 .../testing/selftests/drivers/net/so_txtime.c | 66 ++++++++++---------
 1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/so_txtime.c b/tools/testing/selftests/drivers/net/so_txtime.c
index 55a386f3d1b9..f6f3253ed174 100644
--- a/tools/testing/selftests/drivers/net/so_txtime.c
+++ b/tools/testing/selftests/drivers/net/so_txtime.c
@@ -158,7 +158,40 @@ static void do_recv_verify_empty(int fdr)
 		error(1, 0, "recv: not empty as expected (%d, %d)", ret, errno);
 }
 
-static int do_recv_errqueue_timeout(int fdt)
+static int do_recv_errqueue_txtime(struct sock_extended_err *err,
+				   const char payload_char)
+{
+	const char *reason = NULL;
+	int64_t tstamp = 0;
+
+	switch (err->ee_errno) {
+	case ECANCELED:
+		if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
+			error(1, 0, "errqueue: unknown ECANCELED %u\n",
+			      err->ee_code);
+		reason = "missed txtime";
+	break;
+	case EINVAL:
+		if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
+			error(1, 0, "errqueue: unknown EINVAL %u\n",
+			      err->ee_code);
+		reason = "invalid txtime";
+	break;
+	default:
+		error(1, 0, "errqueue: errno %u code %u\n",
+		      err->ee_errno, err->ee_code);
+	}
+
+	tstamp = ((int64_t)err->ee_data) << 32 | err->ee_info;
+	tstamp -= (int64_t)glob_tstart;
+	tstamp /= 1000 * 1000;
+	fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
+		payload_char, tstamp, reason);
+
+	return 1;
+}
+
+static int do_recv_errqueue(int fdt)
 {
 	char control[CMSG_SPACE(sizeof(struct sock_extended_err)) +
 		     CMSG_SPACE(sizeof(struct sockaddr_in6))] = {0};
@@ -169,7 +202,6 @@ static int do_recv_errqueue_timeout(int fdt)
 	struct msghdr msg = {0};
 	struct iovec iov = {0};
 	struct cmsghdr *cm;
-	int64_t tstamp = 0;
 
 	iov.iov_base = data;
 	iov.iov_len = sizeof(data);
@@ -181,8 +213,6 @@ static int do_recv_errqueue_timeout(int fdt)
 	msg.msg_controllen = sizeof(control);
 
 	while (1) {
-		const char *reason = NULL;
-
 		ret = recvmsg(fdt, &msg, MSG_ERRQUEUE);
 		if (ret == -1 && errno == EAGAIN)
 			break;
@@ -201,33 +231,9 @@ static int do_recv_errqueue_timeout(int fdt)
 		if (err->ee_origin != SO_EE_ORIGIN_TXTIME)
 			error(1, 0, "errqueue: origin 0x%x\n", err->ee_origin);
 
-		switch (err->ee_errno) {
-		case ECANCELED:
-			if (err->ee_code != SO_EE_CODE_TXTIME_MISSED)
-				error(1, 0, "errqueue: unknown ECANCELED %u\n",
-				      err->ee_code);
-			reason = "missed txtime";
-		break;
-		case EINVAL:
-			if (err->ee_code != SO_EE_CODE_TXTIME_INVALID_PARAM)
-				error(1, 0, "errqueue: unknown EINVAL %u\n",
-				      err->ee_code);
-			reason = "invalid txtime";
-		break;
-		default:
-			error(1, 0, "errqueue: errno %u code %u\n",
-			      err->ee_errno, err->ee_code);
-		}
-
-		tstamp = ((int64_t) err->ee_data) << 32 | err->ee_info;
-		tstamp -= (int64_t) glob_tstart;
-		tstamp /= 1000 * 1000;
-		fprintf(stderr, "send: pkt %c at %" PRId64 "ms dropped: %s\n",
-			data[ret - 1], tstamp, reason);
-
+		num_tstamp += do_recv_errqueue_txtime(err, data[ret - 1]);
 		msg.msg_flags = 0;
 		msg.msg_controllen = sizeof(control);
-		num_tstamp++;
 	}
 
 	return num_tstamp;
@@ -245,7 +251,7 @@ static void recv_errqueue_msgs(int fdt)
 			error(1, errno, "poll");
 
 		if (ret && (pfd.revents & POLLERR))
-			num_tstamp += do_recv_errqueue_timeout(fdt);
+			num_tstamp += do_recv_errqueue(fdt);
 
 		if (num_tstamp == cfg_num_pkt)
 			break;
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net-next v9 6/7] selftests: drv-net: in so_txtime tell apart sw from hw pacing

[Willem de Bruijn]

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

Detect software pacing in so_txtime.c using SO_TIMESTAMPING.

If '-H' (hw) is passed

1. measure sw tx delay with SOF_TIMESTAMPING_TX_SOFTWARE, and
2. fail if delay exceeds a threshold, indicating pacing

This will be used in the next patch in the series.

Also extend while condition to account for possible variance.
This applies to all tests, not just the new '-H' variants.

Also reorder getopt parameters to make them alphabetical.

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Changes
  v6 -> v7
    - gate missing tstamp errors++ on !KSFT_MACHINE_SLOW
  v5 -> v6
    - initialize tdeliver_max to now (glob_tstart),
      as otherwise not set for -H tests without delay
  v3 -> v4
    - rename -h to -H to avoid aliasing implicit getopt -h
    - count sw timestamps to verify that they were actually checked
    - (minor) do not ask for SOF_TIMESTAMPING_OPT_ID as not used
    - (minor) remove unused parameter
---
 .../testing/selftests/drivers/net/so_txtime.c | 95 +++++++++++++++----
 1 file changed, 79 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/so_txtime.c b/tools/testing/selftests/drivers/net/so_txtime.c
index f6f3253ed174..9ebe9f91227c 100644
--- a/tools/testing/selftests/drivers/net/so_txtime.c
+++ b/tools/testing/selftests/drivers/net/so_txtime.c
@@ -42,8 +42,10 @@ static bool	cfg_machine_slow;
 static uint64_t	cfg_start_time_ns;
 static int	cfg_mark;
 static bool	cfg_rx;
+static bool	cfg_verify_hw_offload;
 
 static uint64_t glob_tstart;
+static uint64_t glob_tstart_real;
 static uint64_t tdeliver_max;
 
 static int errors;
@@ -191,12 +193,32 @@ static int do_recv_errqueue_txtime(struct sock_extended_err *err,
 	return 1;
 }
 
-static int do_recv_errqueue(int fdt)
+static int do_recv_errqueue_timestamping(struct scm_timestamping *tss)
+{
+	int64_t ts;
+
+	ts = tss->ts[0].tv_sec * 1000ULL * 1000 * 1000;
+	ts += tss->ts[0].tv_nsec;
+	ts -= glob_tstart_real;
+	ts /= 1000;
+
+	if (ts > cfg_variance_us) {
+		fprintf(stderr, "sw delay %" PRId64 "us exceeds bounds\n", ts);
+		if (!cfg_machine_slow)
+			errors++;
+	}
+
+	return 1;
+}
+
+static int do_recv_errqueue(int fdt, int *num_ts)
 {
 	char control[CMSG_SPACE(sizeof(struct sock_extended_err)) +
+		     CMSG_SPACE(sizeof(struct scm_timestamping)) +
 		     CMSG_SPACE(sizeof(struct sockaddr_in6))] = {0};
 	char data[sizeof(struct ethhdr) + sizeof(struct ipv6hdr) +
 		  sizeof(struct udphdr) + 1];
+	struct scm_timestamping *tss;
 	struct sock_extended_err *err;
 	int ret, num_tstamp = 0;
 	struct msghdr msg = {0};
@@ -222,16 +244,29 @@ static int do_recv_errqueue(int fdt)
 			error(1, 0, "errqueue: flags 0x%x\n", msg.msg_flags);
 
 		cm = CMSG_FIRSTHDR(&msg);
+		tss = NULL;
+
+		if (cm->cmsg_level == SOL_SOCKET &&
+		    cm->cmsg_type == SCM_TIMESTAMPING) {
+			tss = (void *)CMSG_DATA(cm);
+			cm = CMSG_NXTHDR(&msg, cm);
+			if (!cm)
+				error(1, 0, "timestamp missing ip err\n");
+		}
+
 		if (cm->cmsg_level != cfg_errq_level ||
 		    cm->cmsg_type != cfg_errq_type)
 			error(1, 0, "errqueue: type 0x%x.0x%x\n",
 				    cm->cmsg_level, cm->cmsg_type);
 
 		err = (struct sock_extended_err *)CMSG_DATA(cm);
-		if (err->ee_origin != SO_EE_ORIGIN_TXTIME)
+		if (err->ee_origin == SO_EE_ORIGIN_TXTIME)
+			num_tstamp += do_recv_errqueue_txtime(err, data[ret - 1]);
+		else if (err->ee_origin == SO_EE_ORIGIN_TIMESTAMPING && tss)
+			*num_ts += do_recv_errqueue_timestamping(tss);
+		else
 			error(1, 0, "errqueue: origin 0x%x\n", err->ee_origin);
 
-		num_tstamp += do_recv_errqueue_txtime(err, data[ret - 1]);
 		msg.msg_flags = 0;
 		msg.msg_controllen = sizeof(control);
 	}
@@ -243,7 +278,7 @@ static void recv_errqueue_msgs(int fdt)
 {
 	struct pollfd pfd = { .fd = fdt, .events = POLLERR };
 	const int timeout_ms = 10;
-	int ret, num_tstamp = 0;
+	int ret, num_tstamp = 0, num_ts = 0;
 
 	do {
 		ret = poll(&pfd, 1, timeout_ms);
@@ -251,12 +286,20 @@ static void recv_errqueue_msgs(int fdt)
 			error(1, errno, "poll");
 
 		if (ret && (pfd.revents & POLLERR))
-			num_tstamp += do_recv_errqueue(fdt);
+			num_tstamp += do_recv_errqueue(fdt, &num_ts);
 
-		if (num_tstamp == cfg_num_pkt)
+		if (num_tstamp == cfg_num_pkt || num_ts == cfg_num_pkt)
 			break;
 
-	} while (gettime_ns(cfg_clockid) < tdeliver_max);
+	} while (gettime_ns(cfg_clockid) <
+		 tdeliver_max + (cfg_variance_us * 1000));
+
+	if (cfg_verify_hw_offload && num_ts != cfg_num_pkt) {
+		fprintf(stderr, "missing timestamps: expected %d, got %d\n",
+			cfg_num_pkt, num_ts);
+		if (!cfg_machine_slow)
+			errors++;
+	}
 }
 
 static void start_time_wait(void)
@@ -301,6 +344,17 @@ static void setsockopt_txtime(int fd)
 		error(1, 0, "getsockopt txtime: mismatch");
 }
 
+static void setsockopt_txtimestamping(int fd)
+{
+	int val = SOF_TIMESTAMPING_TX_SOFTWARE |
+		  SOF_TIMESTAMPING_SOFTWARE |
+		  SOF_TIMESTAMPING_OPT_TSONLY;
+
+	if (setsockopt(fd, SOL_SOCKET, SO_TIMESTAMPING,
+		       &val, sizeof(val)))
+		error(1, errno, "setsockopt timestamping");
+}
+
 static int setup_tx(struct sockaddr *addr, socklen_t alen)
 {
 	int fd;
@@ -314,6 +368,9 @@ static int setup_tx(struct sockaddr *addr, socklen_t alen)
 
 	setsockopt_txtime(fd);
 
+	if (cfg_verify_hw_offload)
+		setsockopt_txtimestamping(fd);
+
 	if (cfg_mark &&
 	    setsockopt(fd, SOL_SOCKET, SO_MARK, &cfg_mark, sizeof(cfg_mark)))
 		error(1, errno, "setsockopt mark");
@@ -354,6 +411,8 @@ static void do_test_tx(struct sockaddr *addr, socklen_t alen)
 
 	start_time_wait();
 	glob_tstart = gettime_ns(cfg_clockid);
+	glob_tstart_real = gettime_ns(CLOCK_REALTIME);
+	tdeliver_max = glob_tstart;
 
 	for (i = 0; i < cfg_num_pkt; i++)
 		do_send_one(fdt, &cfg_buf[i]);
@@ -446,10 +505,11 @@ static void usage(const char *progname)
 			"  -6            only IPv6\n"
 			"  -c <clock>    monotonic or tai (default)\n"
 			"  -D <addr>     destination IP address (server)\n"
-			"  -S <addr>     source IP address (client)\n"
+			"  -H            verify hardware offload (tx)\n"
+			"  -m <mark>     socket mark\n"
 			"  -r            run rx mode\n"
+			"  -S <addr>     source IP address (client)\n"
 			"  -t <nsec>     start time (UTC nanoseconds)\n"
-			"  -m <mark>     socket mark\n"
 			"\n",
 			progname);
 	exit(1);
@@ -461,7 +521,7 @@ static void parse_opts(int argc, char **argv)
 	int domain = PF_UNSPEC;
 	int c;
 
-	while ((c = getopt(argc, argv, "46c:S:D:rt:m:")) != -1) {
+	while ((c = getopt(argc, argv, "46c:D:Hm:rS:t:")) != -1) {
 		switch (c) {
 		case '4':
 			if (domain != PF_UNSPEC)
@@ -488,21 +548,24 @@ static void parse_opts(int argc, char **argv)
 			else
 				error(1, 0, "unknown clock id %s", optarg);
 			break;
-		case 'S':
-			saddr = optarg;
-			break;
 		case 'D':
 			daddr = optarg;
 			break;
+		case 'H':
+			cfg_verify_hw_offload = true;
+			break;
+		case 'm':
+			cfg_mark = strtol(optarg, NULL, 0);
+			break;
 		case 'r':
 			cfg_rx = true;
 			break;
+		case 'S':
+			saddr = optarg;
+			break;
 		case 't':
 			cfg_start_time_ns = strtoll(optarg, NULL, 0);
 			break;
-		case 'm':
-			cfg_mark = strtol(optarg, NULL, 0);
-			break;
 		default:
 			usage(argv[0]);
 		}
-- 
2.55.0.1007.g17ff1f9808-goog

[PATCH net-next v9 7/7] selftests: drv-net: extend so_txtime with hw offload

[Willem de Bruijn]

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

Add two pacing hardware offload variants

1. one that uses FQ to safely offload when within bounds.
2. one that uses pfifo_fast and thus forwards all packets.

Verify that the packets are paced in hardware with new flag '-H'.

Also increase rcvtimeout significantly to reduce flakiness. Especially
for the new beyond_hw_horizon test, which is close to the 100ms limit.
But update recv_verify_empty to take MSG_DONTWAIT. That last empty
check must not delay each testcase by the receive timeout.

Hardware pacing offload can complete packets out of order. So the
reverse_order test is expected to pass with pfifo_fast too.

Do not test ETF, which does not change its dequeue behavior based on
pacing_offload.

The pfifo_fast beyond_hw_horizon testcase expects a failure because
the packet exceeds the hardware horizon and is transmitted immediately,
violating receiver arrival bounds. On slow machines (KSFT_MACHINE_SLOW),
timing variance errors are suppressed by the receiver, so relax the
failure expectation only for this timing-sensitive case while preserving
deterministic checks for other tests (such as ETF invalid txtime).

Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Changes
  v8 -> v9
    - scope expect_fail relaxation to timing_sensitive tests
    - move beyond_hw_horizon slow machine rationale into commit message
    - update to pacing-offload (boolean enable)
  v7 -> v8
    - simplify "not a == b" test to "a != b" (ruff)
  v5 -> v6
    - do not suppress tx failures on expected failure:
      all expect the receiver process to signal failure
  v3 -> v4
    - replace ethtool with rtnetlink APIs
    - expect_fail: correctly handle negative test pfifofast beyond_hw_horizon,
      also when KSFT_MACHINE_SLOW suppresses timing errors
    - commit-msg: clarify that rcvtimeout increase is also needed for
      beyond_hw_horizon test
    - define the horizon (50ms) once, rather than three times
    - leave cfg.require_ipver in place
  v2 -> v3
    - remove drivers/net/settings timeout change: superseded by recent commit
    - add reverse_order comment
  v1 -> v2
    - re-raise NlError from e (patchwork pylint)
    - simplify expect_pass test (patchwork pylint)
---
 .../testing/selftests/drivers/net/so_txtime.c |  4 +-
 .../selftests/drivers/net/so_txtime.py        | 80 ++++++++++++++++++-
 2 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/so_txtime.c b/tools/testing/selftests/drivers/net/so_txtime.c
index 9ebe9f91227c..5bb15498da30 100644
--- a/tools/testing/selftests/drivers/net/so_txtime.c
+++ b/tools/testing/selftests/drivers/net/so_txtime.c
@@ -155,7 +155,7 @@ static void do_recv_verify_empty(int fdr)
 	char rbuf[1];
 	int ret;
 
-	ret = recv(fdr, rbuf, sizeof(rbuf), 0);
+	ret = recv(fdr, rbuf, sizeof(rbuf), MSG_DONTWAIT);
 	if (ret != -1 || errno != EAGAIN)
 		error(1, 0, "recv: not empty as expected (%d, %d)", ret, errno);
 }
@@ -380,7 +380,7 @@ static int setup_tx(struct sockaddr *addr, socklen_t alen)
 
 static int setup_rx(struct sockaddr *addr, socklen_t alen)
 {
-	struct timeval tv = { .tv_usec = 100 * 1000 };
+	struct timeval tv = { .tv_usec = 600 * 1000 };
 	int fd;
 
 	fd = socket(addr->sa_family, SOCK_DGRAM, 0);
diff --git a/tools/testing/selftests/drivers/net/so_txtime.py b/tools/testing/selftests/drivers/net/so_txtime.py
index a097fae0b335..66a87205e02d 100755
--- a/tools/testing/selftests/drivers/net/so_txtime.py
+++ b/tools/testing/selftests/drivers/net/so_txtime.py
@@ -12,10 +12,12 @@ import time
 from lib.py import ksft_exit, ksft_run, ksft_variants
 from lib.py import KsftNamedVariant, KsftSkipEx
 from lib.py import NetDrvEpEnv, bkg, cmd, defer, tc
-from lib.py import CmdExitFailure
+from lib.py import CmdExitFailure, RtnlFamily, NlError
 
+_HW_OFFLOAD_HORIZON_MS = 50
 
-def test_so_txtime(cfg, clockid, ipver, args_tx, args_rx, expect_success):
+def test_so_txtime(cfg, clockid, ipver, args_tx, args_rx, expect_success,
+                   timing_sensitive=False):
     """Main function. Run so_txtime as sender and receiver."""
     slow_machine = os.environ.get('KSFT_MACHINE_SLOW')
 
@@ -33,12 +35,42 @@ def test_so_txtime(cfg, clockid, ipver, args_tx, args_rx, expect_success):
     expect_fail = not expect_success
     if slow_machine:
         expect_success = False
+        if timing_sensitive:
+            expect_fail = None
 
     with bkg(cmd_rx, host=cfg.remote, fail=expect_success,
              expect_fail=expect_fail, exit_wait=True):
         cmd(cmd_tx)
 
 
+def _dev_setup_pacing_offload(cfg):
+    """Configure pacing-offload."""
+    rtnl = RtnlFamily()
+
+    try:
+        link = rtnl.getlink({'ifi-index': cfg.ifindex})
+    except NlError as e:
+        raise KsftSkipEx('getlink not supported by device') from e
+
+    if 'pacing-offload' not in link or \
+       'max-pacing-offload-horizon' not in link:
+        raise KsftSkipEx('pacing offload not supported by device')
+
+    horizon = _HW_OFFLOAD_HORIZON_MS * 1000_000
+    if link['max-pacing-offload-horizon'] < horizon:
+        raise KsftSkipEx('pacing offload max horizon too small')
+
+    cur_offload = link['pacing-offload']
+    rtnl.setlink({
+        'ifi-index': cfg.ifindex,
+        'pacing-offload': 1,
+    })
+    defer(rtnl.setlink, {
+        'ifi-index': cfg.ifindex,
+        'pacing-offload': cur_offload
+    })
+
+
 def _qdisc_setup(ifname, qdisc, optargs=""):
     """Replace root qdisc. Restore the original after the test.
 
@@ -61,6 +93,7 @@ def _test_variants_fq():
             ["one_pkt", "a,10", "a,10"],
             ["in_order", "a,10,b,20", "a,10,b,20"],
             ["reverse_order", "a,20,b,10", "b,10,a,20"],
+            ["beyond_hw_horizon", "a,70", "a,70"],
         ]:
             name = f"v{ipver}_{testcase[0]}"
             yield KsftNamedVariant(name, ipver, testcase[1], testcase[2])
@@ -74,6 +107,41 @@ def test_so_txtime_fq_mono(cfg, ipver, args_tx, args_rx):
     test_so_txtime(cfg, "mono", ipver, args_tx, args_rx, True)
 
 
+@ksft_variants(_test_variants_fq())
+def test_so_txtime_fq_mono_hw(cfg, ipver, args_tx, args_rx):
+    """Run all variants of monotonic fq tests, with offload horizon."""
+    cfg.require_ipver(ipver)
+    cfg.require_nsim(nsim_test=False)
+
+    _dev_setup_pacing_offload(cfg)
+    try:
+        _qdisc_setup(cfg.ifname, "fq", f"offload_horizon {_HW_OFFLOAD_HORIZON_MS}ms")
+    except Exception as e:
+        raise KsftSkipEx("netdev does not support offload. skipping") from e
+
+    # Expect all tests to use only hw pacing, except beyond_hw_horizon.
+    # Do not pass -H to that test so that with sw pacing fallback it passes.
+    hw_only = "-H" if args_tx != "a,70" else ""
+    test_so_txtime(cfg, "mono", ipver, f"{hw_only} {args_tx}", args_rx, True)
+
+
+@ksft_variants(_test_variants_fq())
+def test_so_txtime_pfifofast_mono_hw(cfg, ipver, args_tx, args_rx):
+    """Run all variants of monotonic tests, without fq pacing sw backup."""
+    cfg.require_ipver(ipver)
+    cfg.require_nsim(nsim_test=False)
+
+    _dev_setup_pacing_offload(cfg)
+    _qdisc_setup(cfg.ifname, "pfifo_fast")
+
+    # Expect all tests to pass, except beyond_hw_horizon without sw fallback.
+    # It will send immediately, failing the receiver arrival bounds check.
+    expect_pass = args_tx != "a,70"
+    timing_sensitive = not expect_pass
+    test_so_txtime(cfg, "mono", ipver, f"-H {args_tx}", args_rx, expect_pass,
+                   timing_sensitive=timing_sensitive)
+
+
 @ksft_variants(_test_variants_fq())
 def test_so_txtime_fq_tai(cfg, ipver, args_tx, args_rx):
     """Run all variants of fq tests, but pass CLOCK_TAI to test conversion."""
@@ -123,7 +191,13 @@ def main() -> None:
     """Boilerplate ksft main."""
     with NetDrvEpEnv(__file__) as cfg:
         ksft_run(
-            [test_so_txtime_fq_mono, test_so_txtime_fq_tai, test_so_txtime_etf],
+            [
+                test_so_txtime_fq_mono,
+                test_so_txtime_fq_mono_hw,
+                test_so_txtime_pfifofast_mono_hw,
+                test_so_txtime_fq_tai,
+                test_so_txtime_etf,
+            ],
             args=(cfg,),
         )
     ksft_exit()
-- 
2.55.0.1007.g17ff1f9808-goog

Re: [PATCH net-next v9 1/7] net: rtnetlink: add pacing_offload attribute to net_device

[netdev-bot+sashiko]

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

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

- [Medium] `pacing_offload:1` is added as the first *runtime-mutable* bit
  of the `struct_group(priv_flags_fast, ...)` bitfield word in struct…
- [Low] Concern (verified as NOT a defect): dev->pacing_offload /
  IFLA_PACING_OFFLOAD is claimed to have no in-tree consumer at this…

Pre-existing issues:
- [Low] Pre-existing YAML spec gap adjacent to the change: the shared
  getlink do/dump reply list (`&link-all-attrs`,…

--

net: rtnetlink: add pacing_offload attribute to net_device

This adds a new IFLA_PACING_OFFLOAD link attribute plus a matching
pacing_offload:1 bitfield in struct net_device, so pacing offload can be
turned on and off administratively.  It also marks
IFLA_MAX_PACING_OFFLOAD_HORIZON as NLA_REJECT and documents the new
attribute in the rt-link YAML spec.

A few questions below.

> diff --git a/Documentation/netlink/specs/rt-link.yaml b/Documentation/netlink/specs/rt-link.yaml
> index 5217d0bb4a3a5..7a72cd1b7e1ee 100644
> --- a/Documentation/netlink/specs/rt-link.yaml
> +++ b/Documentation/netlink/specs/rt-link.yaml
> @@ -1091,6 +1091,10 @@ attribute-sets:
>        -
>          name: tailroom
>          type: u16
> +      -
> +        name: pacing-offload
> +        type: u32
> +        doc: Enable EDT pacing offload (0 - disabled, 1 - enabled).
>    -
>      name: prop-list-link-attrs
>      subset-of: link-attrs
> @@ -2559,6 +2563,7 @@ operations:
>              - devlink-port
>              - gso-ipv4-max-size
>              - gro-ipv4-max-size
> +            - pacing-offload

[Severity: Low]
This isn't something introduced by this patch, but while the shared
getlink reply list (&link-all-attrs) is being extended here, should
max-pacing-offload-horizon be added as well?

rtnl_fill_ifinfo() emits it unconditionally in the same nla_put chain as
the new attribute:

	    nla_put_uint(skb, IFLA_MAX_PACING_OFFLOAD_HORIZON,
			 READ_ONCE(dev->max_pacing_offload_horizon)) ||

The same holds for netns-immutable, headroom and tailroom, which are also
always emitted but not listed in the reply set.

The commit message advertises reading the horizon with

    ynl --family rt-link --do getlink ... | grep pacing

which works, because the Python decoder resolves reply members from
attribute-set: link-attrs, but consumers generated from the per-operation
reply list would not see these members.

> diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
> index 512f6d6fa3d8f..8eceaa80b6860 100644
> --- a/Documentation/networking/net_cachelines/net_device.rst
> +++ b/Documentation/networking/net_cachelines/net_device.rst
> @@ -11,6 +11,7 @@ Type                                Name                        fastpath_tx_acce
>  unsigned_long:32                    priv_flags                  read_mostly                             __dev_queue_xmit(tx)
>  unsigned_long:1                     lltx                        read_mostly                             HARD_TX_LOCK,HARD_TX_TRYLOCK,HARD_TX_UNLOCK(tx)
>  unsigned_long:2                     netmem_tx:2;                read_mostly
> +unsigned_long:1                     pacing_offload              read_mostly                             sch_fq
>  char                                name[16]
>  struct netdev_name_node*            name_node
>  struct dev_ifalias*                 ifalias
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 707b2e51c2b97..1f0710eef185b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h

[ ... ]

> @@ -2170,6 +2171,7 @@ struct net_device {
>  		unsigned long		priv_flags:32;
>  		unsigned long		lltx:1;
>  		unsigned long		netmem_tx:2;
> +		unsigned long		pacing_offload:1;
>  	);

[Severity: Medium]
This puts the first runtime-mutable bit into the priv_flags_fast group.  On
64-bit, priv_flags, lltx, netmem_tx and pacing_offload all live in one
unsigned long, and adjacent bitfields are a single memory location, so a
store to one of them is a read-modify-write of the others.

do_setlink() writes the new bit under RTNL only:

		if (dev->pacing_offload != val) {
			dev->pacing_offload = val;

but priv_flags in the same word is modified from softirq context with no
RTNL held, in net/ncsi/ncsi-rsp.c:

	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;

That appears in ncsi_rsp_handler_oem_bcm() and ncsi_rsp_handler_gmcma(),
both reached from ncsi_rcv_rsp(), which is a packet_type receive handler.

Can these two read-modify-writes lose each other's update, so that either
IFF_LIVE_ADDR_CHANGE is silently dropped, or pacing_offload reads back 0
after do_setlink() already set DO_SETLINK_MODIFIED and sent a notification?

A second point on the same field: a 1-bit bitfield cannot be accessed with
READ_ONCE()/WRITE_ONCE(), yet the net_cachelines row added above documents
pacing_offload as a read_mostly field consumed lockless by sch_fq.  Every
neighbouring netdev field in these paths is accessed with
READ_ONCE()/WRITE_ONCE(), including max_pacing_offload_horizon right next
to it.

Would a plain bool/u8, or a dedicated unsigned long used with
set_bit()/test_bit(), work here instead?  That would make the update
independent of the neighbouring bits and allow marked lockless reads.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index be9d1625bac31..e3444fd240615 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c

[ ... ]

> @@ -2176,6 +2177,8 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>  			READ_ONCE(dev->tso_max_segs)) ||
>  	    nla_put_uint(skb, IFLA_MAX_PACING_OFFLOAD_HORIZON,
>  			 READ_ONCE(dev->max_pacing_offload_horizon)) ||
> +	    nla_put_u32(skb, IFLA_PACING_OFFLOAD,
> +			dev->pacing_offload) ||
>  #ifdef CONFIG_RPS
>  	    nla_put_u32(skb, IFLA_NUM_RX_QUEUES,
>  			READ_ONCE(dev->num_rx_queues)) ||

[ ... ]

> @@ -3342,6 +3355,15 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
>  		}
>  	}
>  
> +	if (tb[IFLA_PACING_OFFLOAD]) {
> +		bool val = nla_get_u32(tb[IFLA_PACING_OFFLOAD]);
> +
> +		if (dev->pacing_offload != val) {
> +			dev->pacing_offload = val;
> +			status |= DO_SETLINK_MODIFIED;
> +		}
> +	}
> +
>  	if (tb[IFLA_OPERSTATE])
>  		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));

[Severity: Low]
This isn't a bug, just a note on reading the patch in isolation: at this
commit do_setlink() is the only place that touches dev->pacing_offload, so
the new knob has no reader, and the net_cachelines row that names sch_fq as
the consumer has nothing to point at yet.

Checking the rest of the series shows the consumers arrive right after:
fq_offload_horizon() in net/sched/sch_fq.c returns 0 when
!dev->pacing_offload and is called from fq_classify() and fq_dequeue(),
fq_change() rejects TCA_FQ_OFFLOAD_HORIZON with "device pacing offload is
disabled", and idpf adds both the first advertiser of
max_pacing_offload_horizon and the

	if (!tx_q->netdev->pacing_offload)
		return;

check in its TX path.  So the annotation and the setlink example do become
accurate once the whole series is applied, and the commit message already
says no driver advertises max_pacing_offload_horizon yet.

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

Re: [PATCH net-next v9 2/7] net_sched: sch_fq: check device pacing offload

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

- [Medium] New, undocumented netlink rejection in fq_change(): `tc qdisc
  {add,replace,change} ... fq offload_horizon N` with N != 0 now returns…
- [Medium] Configured vs. effective offload horizon are now inconsistent
  across the three FQ interfaces.
- [Low] The patch introduces a per-packet TX fast-path read of
  `dev->max_pacing_offload_horizon` (fq_offload_horizon(),…
- [Low] fq_offload_horizon() reads `dev->pacing_offload` as a plain,
  unannotated load from the qdisc data path (net/sched/sch_fq.c:315,…

--

net_sched: sch_fq: check device pacing offload

FQ now consults the device's pacing offload state before applying the
configured offload horizon, via a new fq_offload_horizon() helper that is
evaluated once per packet in fq_classify() and fq_dequeue().  The helper
returns 0 when the device flag is clear or when the configured horizon
exceeds the device maximum, and fq_change() gains a new rejection when a
non-zero horizon is requested on a device with pacing offload disabled.

A few questions below.

> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 35f940b2205df..dfc7f98b600f8 100644
> --- a/net/sched/sch_fq.c
> +++ b/net/sched/sch_fq.c
> @@ -301,6 +301,26 @@ static void fq_gc(struct fq_sched_data *q,
>  	q->stat_gc_flows += fcnt;
>  }
>  
> +static u64 fq_offload_horizon(const struct Qdisc *sch,
> +			      const struct fq_sched_data *q)
> +{
> +	const struct net_device *dev;
> +	u64 offload_horizon;
> +
> +	offload_horizon = READ_ONCE(q->offload_horizon);
> +	if (!offload_horizon)
> +		return 0;
> +
> +	dev = qdisc_dev(sch);
> +	if (!dev->pacing_offload)
> +		return 0;

[Severity: Low]
Is this read of dev->pacing_offload racy?

The two neighbouring loads in this same helper use READ_ONCE(), but
pacing_offload is read plainly here.  It is a 1-bit bitfield inside
struct_group(priv_flags_fast) in struct net_device, and the sibling
rtnetlink change makes it runtime-writable:

net/core/rtnetlink.c:do_setlink() {
	if (tb[IFLA_PACING_OFFLOAD]) {
		bool val = nla_get_u32(tb[IFLA_PACING_OFFLOAD]);

		if (dev->pacing_offload != val) {
			dev->pacing_offload = val;
	...
}

The writer holds only RTNL, while this reader runs from fq_classify() and
fq_dequeue() under the qdisc root lock, so the two sides share no lock and
both accesses are plain.  Would KCSAN flag this?

Because it is a bitfield, READ_ONCE()/WRITE_ONCE() cannot be applied to it.
Would a dedicated non-bitfield member (or bitops on a separate unsigned
long), or at minimum data_race() on this read, be preferable?

Separately, storing a single bit is a read-modify-write of the whole
declared unsigned long storage unit, which also holds priv_flags:32,
lltx:1 and netmem_tx:2 - fields read locklessly on the TX path.  Which
lock is intended to serialize RMWs of that word now that one of its bits
changes at runtime?  I did not find a non-RTNL writer of the neighbouring
bits, so this part is speculative.

> +
> +	if (offload_horizon > READ_ONCE(dev->max_pacing_offload_horizon))
> +		return 0;
> +
> +	return offload_horizon;
> +}
> +

[Severity: Low]
Should Documentation/networking/net_cachelines/net_device.rst be updated
here?

This adds a per-packet TX read of dev->max_pacing_offload_horizon, but the
row for that field in net_device.rst still has empty fastpath_tx_access and
Comments columns.  The companion bit added by the preceding patch in the
series was annotated:

  unsigned_long:1        pacing_offload    read_mostly    ...    sch_fq

struct net_device also carries a comment asking for the document to be kept
in sync ("Please update the document when adding new fields").

max_pacing_offload_horizon still lives in the cold region of struct
net_device next to irq_moder/napi_config, outside the net_device_read_tx
cacheline group, so for qdiscs that configured an offload horizon the TX
path now touches an extra otherwise-cold cache line per enqueue and per
dequeue.  Was that placement considered?

>  /* Fast path can be used if :
>   * 1) Packet tstamp is in the past, or within the pacing offload horizon.
>   * 2) FQ qlen == 0   OR

[ ... ]

> @@ -357,6 +377,7 @@ static struct fq_flow *fq_classify(struct Qdisc *sch, struct sk_buff *skb,
>  				   u64 now)
>  {
>  	struct fq_sched_data *q = qdisc_priv(sch);
> +	u64 offload_horizon = fq_offload_horizon(sch, q);
>  	struct rb_node **p, *parent;
>  	struct sock *sk = skb->sk;
>  	struct rb_root *root;

[ ... ]

> @@ -705,6 +728,7 @@ static struct fq_flow_head *fq_pband_head_select(struct fq_perband_flows *pband)
>  static struct sk_buff *fq_dequeue(struct Qdisc *sch)
>  {
>  	struct fq_sched_data *q = qdisc_priv(sch);
> +	u64 offload_horizon = fq_offload_horizon(sch, q);
>  	struct fq_perband_flows *pband;
>  	struct fq_flow_head *head;
>  	struct sk_buff *skb;

[ ... ]

> @@ -1179,11 +1204,15 @@ 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) {
> -			WRITE_ONCE(q->offload_horizon, offload_horizon);
> -		} else {
> +		if (offload_horizon && !qdisc_dev(sch)->pacing_offload) {
> +			NL_SET_ERR_MSG_MOD(extack, "device pacing offload is disabled");
> +			err = -EINVAL;

[Severity: Medium]
Is this new configuration-time rejection intended, and should the commit
message mention it?

The changelog only describes runtime behaviour:

    In FQ, only offload if the feature is enabled and within the supported
    device horizon.

but this hunk also makes

  tc qdisc replace dev X root fq offload_horizon 500us

return -EINVAL whenever qdisc_dev(sch)->pacing_offload is 0.  Before the
patch the only gate was offload_horizon <= max_pacing_offload_horizon.
Since the only setter of that bit is do_setlink() via IFLA_PACING_OFFLOAD,
does this create an ordering dependency where the link attribute must be
enabled before the qdisc can be configured?

The new fq_offload_horizon() already degrades to 0 when the device flag is
clear, so the datapath is safe without the hard rejection:

	dev = qdisc_dev(sch);
	if (!dev->pacing_offload)
		return 0;

Would accepting the value (and letting it stay inert until the device flag
is enabled) be preferable to failing the command?

> +		} else if (offload_horizon >
> +			   qdisc_dev(sch)->max_pacing_offload_horizon) {
>  			NL_SET_ERR_MSG_MOD(extack, "invalid offload_horizon");
>  			err = -EINVAL;
> +		} else {
> +			WRITE_ONCE(q->offload_horizon, offload_horizon);
>  		}
>  	}
>  	if (!err) {

[Severity: Medium]
Does this make the value reported by fq_dump() inaccurate and no longer
re-appliable?

fq_dump() is untouched and still reports the raw stored value:

	offload_horizon = READ_ONCE(q->offload_horizon);
	do_div(offload_horizon, NSEC_PER_USEC);
	...
	    nla_put_u32(skb, TCA_FQ_OFFLOAD_HORIZON, (u32)offload_horizon) ||

Consider:

  ip link set dev X pacing_offload on
  tc qdisc replace dev X root fq offload_horizon 500us   /* stored */
  ip link set dev X pacing_offload off                   /* do_setlink() */

Nothing clears q->offload_horizon, so fq_offload_horizon() returns 0 and
the datapath does pure software pacing while tc qdisc show still prints
offload_horizon 500us.  Replaying that same dump then hits the new -EINVAL
above, where it previously succeeded.

The same silent divergence appears when a device lowers
max_pacing_offload_horizon on re-init, for example
idpf_vport_set_pacing_offload() doing
WRITE_ONCE(netdev->max_pacing_offload_horizon, max_horizon) with
max_horizon possibly 0 - the "device renegotiation" case named in the
changelog.

The uAPI comment also still promises unconditional behaviour:

include/uapi/linux/pkt_sched.h:
	TCA_FQ_OFFLOAD_HORIZON,	/* dequeue packets in the past, if within horizon */

Should fq_dump() report the effective horizon, or should the comment note
the dependency on device administrative state?

One smaller inconsistency: fq_offload_horizon() uses READ_ONCE() for
dev->max_pacing_offload_horizon while fq_change() reads the same field, and
pacing_offload, plainly.

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

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

[netdev-bot+sashiko]

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?

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?

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

Re: [PATCH net-next v9 4/7] idpf: support pacing offload

[netdev-bot+sashiko]

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

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

- [High] idpf advertises a non-zero netdev->max_pacing_offload_horizon
  (and therefore programs absolute CLOCK_TAI departure times into the…
- [Medium] The EDT horizon advertised to the networking core is taken
  verbatim from firmware and only bounds-checked against U32_MAX, while…
- [Low] idpf_vport_set_pacing_offload() writes
  netdev->max_pacing_offload_horizon (idpf_lib.c:832) from the…
- [Low] In idpf_send_get_edt_caps_msg() the firmware-supplied granularity
  is a u64 (gran_ns = le64_to_cpu(caps.tstamp_granularity_ns)) but is…

--

idpf: support pacing offload

This adds EDT (earliest departure time) support to idpf: EDT capabilities
are negotiated over virtchnl, the negotiated horizon is published as
netdev->max_pacing_offload_horizon, and skb->tstamp is programmed into the
23-bit timestamp field of the flow scheduling Tx descriptor.

A few questions below on the capability gating and on the horizon
encoding.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 827c795afcb62..cd1b173e3b276 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -817,6 +817,21 @@ static void idpf_attach_and_open(struct idpf_adapter *adapter)
>  	}
>  }
>  
> +static void idpf_vport_set_pacing_offload(struct idpf_vport *vport,
> +					  struct net_device *netdev)
> +{
> +	struct idpf_adapter *adapter = vport->adapter;
> +	u32 max_horizon = 0;
> +
> +	if (adapter->edt_caps.time_horizon_ns &&
> +	    idpf_is_queue_model_split(vport->dflt_qv_rsrc.txq_model) &&
> +	    !idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
> +			     VIRTCHNL2_CAP_SPLITQ_QSCHED))
> +		max_horizon = adapter->edt_caps.time_horizon_ns;
> +

[Severity: High]
The commit message says:

    Negotiate the feature with the device using virtchnl. Support is
    conditional on
    ...
    - PTP to ensure the NIC clock is synced to CLOCK_TAI.

Where is that PTP condition implemented?  idpf_vport_set_pacing_offload()
gates only on adapter->edt_caps.time_horizon_ns, split txq model and flow
scheduling.  It does not consult VIRTCHNL2_CAP_PTP, adapter->ptp, or the
result of idpf_ptp_init().

In idpf_vc_core_init() the EDT negotiation is gated only on
VIRTCHNL2_CAP_EDT and runs before idpf_ptp_init(), whose failure is
deliberately non-fatal:

	err = idpf_ptp_init(adapter);
	if (err)
		pci_err(adapter->pdev, "PTP init failed, err=%pe\n",
			ERR_PTR(err));

So with a control plane that advertises EDT but not PTP, or when
idpf_ptp_get_caps()/clock creation fails, edt_caps.time_horizon_ns stays
non-zero and the horizon is still published.

With CONFIG_PTP_1588_CLOCK=n the stub in idpf_ptp.h returns success while
leaving adapter->ptp NULL:

	static inline int idpf_ptp_init(struct idpf_adapter *adapter)
	{
		return 0;
	}

In that build there is no PHC at all and no way for userspace to steer the
device clock, yet pacing offload remains advertised and
idpf_tx_splitq_set_txtime() keeps writing host CLOCK_TAI values into the
descriptor.  Should the helper also require a working PTP clock, or should
the commit message drop the PTP claim?

Related: the device clock is seeded from CLOCK_REALTIME in
idpf_ptp_init():

		ts = ktime_to_timespec64(ktime_get_real());
		err = idpf_ptp_settime64(&adapter->ptp->info, &ts);

while the new Tx path converts everything to CLOCK_TAI.  Is the epoch
assumed by the descriptor only correct after an external synchronizer has
corrected the PHC?

> +	WRITE_ONCE(netdev->max_pacing_offload_horizon, max_horizon);
> +}
> +

[Severity: Low]
Should this write be done under rtnl_lock()?  The RTNL side reads the same
field with a plain load in validate_linkmsg() before do_setlink() sets
dev->pacing_offload:

	net/core/rtnetlink.c:validate_linkmsg() {
		if (nla_get_u32(tb[IFLA_PACING_OFFLOAD]) &&
		    !dev->max_pacing_offload_horizon) {
	...
	}

idpf_cfg_netdev() is reached from idpf_init_task with no RTNL held, and on
the reset path it re-runs the helper on an already registered netdev, so an
"ip link set dev X pacing_offload on" validated against a non-zero horizon
can be followed immediately by the driver zeroing it.

That also leaves dev->pacing_offload set with
max_pacing_offload_horizon == 0, which is then dumped to userspace as
IFLA_PACING_OFFLOAD=1 together with IFLA_MAX_PACING_OFFLOAD_HORIZON=0.
Should the driver clear dev->pacing_offload when the horizon drops to 0?

>  /**
>   * idpf_cfg_netdev - Allocate, configure and register a netdev
>   * @vport: main vport structure
> @@ -850,6 +865,8 @@ static int idpf_cfg_netdev(struct idpf_vport *vport)
>  		np->max_tx_hdr_size = idpf_get_max_tx_hdr_size(adapter);
>  		vport->netdev = netdev;
>  
> +		idpf_vport_set_pacing_offload(vport, netdev);
> +
>  		return idpf_init_mac_addr(vport, netdev);
>  	}
>  

[ ... ]

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 4311ffa30bb18..215a62161a049 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c

[ ... ]

> @@ -3011,6 +3017,63 @@ static bool idpf_tx_splitq_need_re(struct idpf_tx_queue *tx_q)
>  	return gap >= IDPF_TX_SPLITQ_RE_MIN_GAP;
>  }
>  
> +static void idpf_tx_splitq_set_txtime(const struct sk_buff *skb,
> +				      const struct idpf_tx_queue *tx_q,
> +				      struct idpf_tx_splitq_params *tx_params)
> +{

[ ... ]

> +	/* beyond offload horizon? set overflow bit only */
> +	if (ts > now + horizon) {
> +		tx_params->offload.desc_ts[2] =
> +			IDPF_TXD_FLOW_SCH_HORIZON_OVERFLOW_M;
> +		return;
> +	}
> +
> +	ts >>= tx_q->ts_gran_pow2;

[Severity: Medium]
Can the advertised horizon exceed what the descriptor can encode?  The
hardware field is 23 bits of granularity units:

	drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h:
		/* [23:23] Horizon Overflow bit, [22:0] timestamp */
		u8 ts[3];

idpf_send_get_edt_caps_msg() checks time_horizon_ns only against U32_MAX
and never cross-checks it against (1 << 23) << ilog2(tstamp_granularity_ns).
For a control plane reporting tstamp_granularity_ns = 1 (passes both
checks) with time_horizon_ns = 1000000000, the encodable window is only
about 8.39 ms, but 1 s is published as netdev->max_pacing_offload_horizon
and accepted by sch_fq and SO_TXTIME.

Anything at or below the advertised horizon takes the non-overflow branch
here and gets truncated:

	tx_params->offload.desc_ts[2] = ((ts >> 16) & 0x7f);

so the departure time aliases back into the current window instead of being
flagged as overflow.  Would clamping help, e.g.
max_horizon = min(time_horizon_ns, (1ULL << 23) << tstamp_granularity_pow2)?

Separately, is the boundary test off by one even for self-consistent caps?
With horizon == (1 << 23) << gran, a timestamp exactly at now + horizon
passes the strict "ts > now + horizon" test, and after the shift it differs
from the scaled value of now by exactly 2^23, which masks to the same 23-bit
value, so the device would see "now" and release the packet a whole horizon
early.  Should the comparison be >= ?

> +
> +	/* 0 is valid 23b timestamp, but also means field unset.
> +	 * Increase by one to avoid this case
> +	 */
> +	if ((ts & 0x7fffff) == 0) {
> +		tx_params->offload.desc_ts[0] = 1;
> +		return;
> +	}
> +
> +	tx_params->offload.desc_ts[0] = ts & 0xff;
> +	tx_params->offload.desc_ts[1] = (ts >> 8) & 0xff;
> +	tx_params->offload.desc_ts[2] = ((ts >> 16) & 0x7f);
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 1caf527069737..928de868fa624 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

[ ... ]

> @@ -573,6 +575,54 @@ static int idpf_send_get_caps_msg(struct idpf_adapter *adapter)
>  	return err;
>  }
>  

[ ... ]

> +	memcpy(&caps, xn_params.recv_mem.iov_base, sizeof(caps));
> +	horizon_ns = le64_to_cpu(caps.time_horizon_ns);
> +	gran_ns = le64_to_cpu(caps.tstamp_granularity_ns);
> +	if (horizon_ns > U32_MAX) {
> +		dev_warn(&adapter->pdev->dev, "EDT horizon exceeds U32\n");
> +		err = -EINVAL;
> +		goto free_rx_buf;
> +	}
> +	if (!gran_ns || !is_power_of_2(gran_ns)) {
> +		dev_warn(&adapter->pdev->dev, "Invalid EDT granularity\n");
> +		err = -EINVAL;
> +		goto free_rx_buf;
> +	}
> +
> +	adapter->edt_caps.time_horizon_ns = horizon_ns;
> +	adapter->edt_caps.tstamp_granularity_pow2 = ilog2(gran_ns);

[Severity: Low]
Does this validate a different value than the one it then uses?  gran_ns is
a u64, but is_power_of_2() takes unsigned long:

	include/linux/log2.h:
	static __always_inline __attribute__((const))
	bool is_power_of_2(unsigned long n)
	{
		return n - 1 < (n ^ (n - 1));
	}

IDPF has no "depends on 64BIT" in its Kconfig, so on 32-bit builds the
upper 32 bits are dropped for the check while ilog2(gran_ns) takes the
__ilog2_u64 path on the full value.  A response of 0x100000001 truncates to
1, passes the check, and yields tstamp_granularity_pow2 = 32, which is
copied into idpf_tx_queue.ts_gran_pow2 and used as the shift in
idpf_tx_splitq_set_txtime().  The same truncation rejects a legitimate
granularity of 2^32 on 32-bit.  Would testing the u64 directly, e.g.
gran_ns & (gran_ns - 1), plus an upper bound, be better here?

> +
> +free_rx_buf:
> +	libie_ctlq_release_rx_buf(&xn_params.recv_mem);
> +
> +	return err;
> +}
> +

[ ... ]

> @@ -3085,6 +3135,14 @@ int idpf_vc_core_init(struct idpf_adapter *adapter)
>  		}
>  	}
>  
> +	memset(&adapter->edt_caps, 0, sizeof(adapter->edt_caps));
> +	if (idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS, VIRTCHNL2_CAP_EDT)) {
> +		err = idpf_send_get_edt_caps_msg(adapter);
> +		if (err)
> +			dev_err(&adapter->pdev->dev,
> +				"EDT init failed, err=%d\n", err);
> +	}
> +

[Severity: High]
This is the other half of the PTP question above: EDT caps are fetched here
gated only on VIRTCHNL2_CAP_EDT, and this runs before the idpf_ptp_init()
call further down whose error is only logged.  Should the EDT negotiation
either be gated on VIRTCHNL2_CAP_PTP, or moved after idpf_ptp_init() so the
caps can be zeroed when there is no usable PHC?

>  	pci_sriov_set_totalvfs(adapter->pdev, idpf_get_max_vfs(adapter));
>  	num_max_vports = idpf_get_max_vports(adapter);
>  	adapter->vports = kzalloc_objs(*adapter->vports, num_max_vports);

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

Re: [PATCH net-next v9 6/7] selftests: drv-net: in so_txtime tell apart sw from hw pacing

[netdev-bot+sashiko]

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

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

- [Medium] `-H` is documented in usage() as 'verify hardware offload
  (tx)' and the commit message claims it tells software apart from…

--

selftests: drv-net: in so_txtime tell apart sw from hw pacing

This adds a '-H' flag to so_txtime.c that enables SO_TIMESTAMPING with
SOF_TIMESTAMPING_TX_SOFTWARE, and flags an error if the software transmit
timestamp is more than cfg_variance_us after the send start time.  It also
extends the errqueue polling deadline by cfg_variance_us and sorts the
getopt string and usage text alphabetically.

> diff --git a/tools/testing/selftests/drivers/net/so_txtime.c b/tools/testing/selftests/drivers/net/so_txtime.c
> index f6f3253ed1748..9ebe9f91227c7 100644
> --- a/tools/testing/selftests/drivers/net/so_txtime.c
> +++ b/tools/testing/selftests/drivers/net/so_txtime.c

[ ... ]

> @@ -191,12 +193,32 @@ static int do_recv_errqueue_txtime(struct sock_extended_err *err,
>  	return 1;
>  }
>  
> -static int do_recv_errqueue(int fdt)
> +static int do_recv_errqueue_timestamping(struct scm_timestamping *tss)
> +{
> +	int64_t ts;
> +
> +	ts = tss->ts[0].tv_sec * 1000ULL * 1000 * 1000;
> +	ts += tss->ts[0].tv_nsec;
> +	ts -= glob_tstart_real;
> +	ts /= 1000;
> +
> +	if (ts > cfg_variance_us) {
> +		fprintf(stderr, "sw delay %" PRId64 "us exceeds bounds\n", ts);
> +		if (!cfg_machine_slow)
> +			errors++;
> +	}
> +
> +	return 1;
> +}

[Severity: Medium]

Can this check actually tell software pacing apart from hardware pacing for
every workload the next patch runs with '-H'?

The only positive criterion here is that the software transmit timestamp
lands within cfg_variance_us (8000 us) of glob_tstart_real.  For a request
with no delay or a negative delay there is no pacing interval to observe at
all.  do_send_one() does not even attach SCM_TXTIME when delay_us is
negative:

	if (ts->delay_us >= 0) {
		...
		tdeliver = glob_tstart + ts->delay_us * 1000;
		...
		cm->cmsg_type = SCM_TXTIME;

So for the 'a,-1' and 'a,0' variants that the follow-up patch passes '-H'
to, ts comes out near 0 and the ts > cfg_variance_us test can never fire,
whether the transmit was hardware paced, software paced, or not paced at
all.  Would it make sense to skip or reject '-H' when the requested delay
is not positive, so those variants do not report a pass without having
verified anything?

For the 10 ms variants there are only 2 ms between the requested delay and
the 8 ms threshold.  Is that margin intentional?

A related question on cfg_variance_us: it is also the receiver side
delivery jitter tolerance used in do_recv_one():

	if (llabs(tstop - texpect) > cfg_variance_us) {
		fprintf(stderr, "exceeds variance (%d us)\n", cfg_variance_us);

Reusing it as the sender side software pacing threshold means changing one
tolerance silently changes the other.  Would a separate constant for the
sender side threshold be clearer?

[ ... ]

> @@ -446,10 +505,11 @@ static void usage(const char *progname)
>  			"  -6            only IPv6\n"
>  			"  -c <clock>    monotonic or tai (default)\n"
>  			"  -D <addr>     destination IP address (server)\n"
> -			"  -S <addr>     source IP address (client)\n"
> +			"  -H            verify hardware offload (tx)\n"

Given the above, is "verify hardware offload (tx)" an accurate description
of what the flag does?  The implemented behaviour is a heuristic on the
software transmit delay, and it degrades to a no-op for zero and negative
delays.  Would wording that describes the software delay check be closer to
the code?

> +			"  -m <mark>     socket mark\n"
>  			"  -r            run rx mode\n"
> +			"  -S <addr>     source IP address (client)\n"
>  			"  -t <nsec>     start time (UTC nanoseconds)\n"
> -			"  -m <mark>     socket mark\n"
>  			"\n",
>  			progname);
>  	exit(1);

[ ... ]

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

Re: [PATCH net-next v9 7/7] selftests: drv-net: extend so_txtime with hw offload

[netdev-bot+sashiko]

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

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

- [Medium] The new `beyond_hw_horizon` variant hard-codes a 70ms txtime,
  but `_dev_setup_pacing_offload()`…

--

selftests: drv-net: extend so_txtime with hw offload

Adds two hardware pacing offload variants of the so_txtime driver test, one
on fq with an offload_horizon and one on pfifo_fast, plus a new
beyond_hw_horizon testcase with a 70ms txtime. Also raises the receive
timeout and switches the final empty-queue check to MSG_DONTWAIT.

> diff --git a/tools/testing/selftests/drivers/net/so_txtime.py b/tools/testing/selftests/drivers/net/so_txtime.py
> index a097fae0b335a..66a87205e02db 100755
> --- a/tools/testing/selftests/drivers/net/so_txtime.py
> +++ b/tools/testing/selftests/drivers/net/so_txtime.py

[ ... ]

> @@ -33,12 +35,42 @@ def test_so_txtime(cfg, clockid, ipver, args_tx, args_rx, expect_success):
>      expect_fail = not expect_success
>      if slow_machine:
>          expect_success = False
> +        if timing_sensitive:
> +            expect_fail = None
>  
>      with bkg(cmd_rx, host=cfg.remote, fail=expect_success,
>               expect_fail=expect_fail, exit_wait=True):
>          cmd(cmd_tx)
>  
>  
> +def _dev_setup_pacing_offload(cfg):
> +    """Configure pacing-offload."""
> +    rtnl = RtnlFamily()
> +
> +    try:
> +        link = rtnl.getlink({'ifi-index': cfg.ifindex})
> +    except NlError as e:
> +        raise KsftSkipEx('getlink not supported by device') from e
> +
> +    if 'pacing-offload' not in link or \
> +       'max-pacing-offload-horizon' not in link:
> +        raise KsftSkipEx('pacing offload not supported by device')
> +
> +    horizon = _HW_OFFLOAD_HORIZON_MS * 1000_000
> +    if link['max-pacing-offload-horizon'] < horizon:
> +        raise KsftSkipEx('pacing offload max horizon too small')
> +
> +    cur_offload = link['pacing-offload']
> +    rtnl.setlink({
> +        'ifi-index': cfg.ifindex,
> +        'pacing-offload': 1,
> +    })
> +    defer(rtnl.setlink, {
> +        'ifi-index': cfg.ifindex,
> +        'pacing-offload': cur_offload
> +    })
> +
> +
>  def _qdisc_setup(ifname, qdisc, optargs=""):

[ ... ]

> @@ -61,6 +93,7 @@ def _test_variants_fq():
>              ["one_pkt", "a,10", "a,10"],
>              ["in_order", "a,10,b,20", "a,10,b,20"],
>              ["reverse_order", "a,20,b,10", "b,10,a,20"],
> +            ["beyond_hw_horizon", "a,70", "a,70"],
>          ]:
>              name = f"v{ipver}_{testcase[0]}"
>              yield KsftNamedVariant(name, ipver, testcase[1], testcase[2])
> @@ -74,6 +107,41 @@ def test_so_txtime_fq_mono(cfg, ipver, args_tx, args_rx):
>      test_so_txtime(cfg, "mono", ipver, args_tx, args_rx, True)
>  
>  
> +@ksft_variants(_test_variants_fq())
> +def test_so_txtime_fq_mono_hw(cfg, ipver, args_tx, args_rx):
> +    """Run all variants of monotonic fq tests, with offload horizon."""
> +    cfg.require_ipver(ipver)
> +    cfg.require_nsim(nsim_test=False)
> +
> +    _dev_setup_pacing_offload(cfg)
> +    try:
> +        _qdisc_setup(cfg.ifname, "fq", f"offload_horizon {_HW_OFFLOAD_HORIZON_MS}ms")
> +    except Exception as e:
> +        raise KsftSkipEx("netdev does not support offload. skipping") from e
> +
> +    # Expect all tests to use only hw pacing, except beyond_hw_horizon.
> +    # Do not pass -H to that test so that with sw pacing fallback it passes.
> +    hw_only = "-H" if args_tx != "a,70" else ""
> +    test_so_txtime(cfg, "mono", ipver, f"{hw_only} {args_tx}", args_rx, True)
> +
> +
> +@ksft_variants(_test_variants_fq())
> +def test_so_txtime_pfifofast_mono_hw(cfg, ipver, args_tx, args_rx):
> +    """Run all variants of monotonic tests, without fq pacing sw backup."""
> +    cfg.require_ipver(ipver)
> +    cfg.require_nsim(nsim_test=False)
> +
> +    _dev_setup_pacing_offload(cfg)
> +    _qdisc_setup(cfg.ifname, "pfifo_fast")
> +
> +    # Expect all tests to pass, except beyond_hw_horizon without sw fallback.
> +    # It will send immediately, failing the receiver arrival bounds check.
> +    expect_pass = args_tx != "a,70"
> +    timing_sensitive = not expect_pass
> +    test_so_txtime(cfg, "mono", ipver, f"-H {args_tx}", args_rx, expect_pass,
> +                   timing_sensitive=timing_sensitive)

[Severity: Medium]
Is the beyond_hw_horizon expectation guaranteed on all devices that pass the
capability gate in _dev_setup_pacing_offload()?

That helper only checks a lower bound on the device horizon:

    horizon = _HW_OFFLOAD_HORIZON_MS * 1000_000
    if link['max-pacing-offload-horizon'] < horizon:
        raise KsftSkipEx('pacing offload max horizon too small')

so a device advertising, say, 100ms or more is accepted. In
test_so_txtime_pfifofast_mono_hw() the root qdisc is pfifo_fast, so there is
no qdisc offload_horizon gate and the immediate-send decision is left to the
driver's own horizon check, for example in idpf_tx_splitq_set_txtime():

    /* beyond offload horizon? set overflow bit only */
    if (ts > now + horizon) {
            tx_params->offload.desc_ts[2] =
                    IDPF_TXD_FLOW_SCH_HORIZON_OVERFLOW_M;

On a device whose reported max-pacing-offload-horizon covers 70ms, the packet
is paced in hardware, arrives on time, the receiver exits 0, and with
expect_fail set by:

    expect_pass = args_tx != "a,70"

cmd.process() in tools/testing/selftests/net/lib/py/utils.py turns that into
a failure:

    elif self.proc.returncode == 0 and expect_fail:
        raise CmdExitZeroFailure("Command succeeded (expected fail)", self)

The timing_sensitive escape only clears expect_fail under
KSFT_MACHINE_SLOW, so on a normal machine the failure is unconditional.
Would it make sense for _dev_setup_pacing_offload() to also skip when
max-pacing-offload-horizon is at or above the 70ms txtime used by
beyond_hw_horizon?

Related, in test_so_txtime_fq_mono_hw() the same case drops the -H flag:

    hw_only = "-H" if args_tx != "a,70" else ""

If fq incorrectly handed the 70ms packet to hardware and the device horizon
covers 70ms, the packet still arrives on time and the subtest passes, so is
the intended 50ms fq offload_horizon boundary actually asserted here?

The commit message states this as a fact:

    The pfifo_fast beyond_hw_horizon testcase expects a failure because
    the packet exceeds the hardware horizon and is transmitted immediately,

which appears to hold only when the device horizon is between 50ms and 70ms.
Could that constraint be spelled out, or enforced in the capability check?

[ ... ]

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

Re: [PATCH net-next v9 1/7] net: rtnetlink: add pacing_offload attribute to net_device

[Willem de Bruijn]

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 (1)
> 
> - [Medium] `pacing_offload:1` is added as the first *runtime-mutable* bit
>   of the `struct_group(priv_flags_fast, ...)` bitfield word in struct…
> - [Low] Concern (verified as NOT a defect): dev->pacing_offload /
>   IFLA_PACING_OFFLOAD is claimed to have no in-tree consumer at this…

TL;DR: I think all reports to this series are false positives.

I'll respond in detail, skipping only some too obvious low ones
(e.g., "this feature is not used yet, but will be in a next patch").
 
> Pre-existing issues:
> - [Low] Pre-existing YAML spec gap adjacent to the change: the shared
>   getlink do/dump reply list (`&link-all-attrs`,…
> 
> --
> 
> net: rtnetlink: add pacing_offload attribute to net_device
> 
> This adds a new IFLA_PACING_OFFLOAD link attribute plus a matching
> pacing_offload:1 bitfield in struct net_device, so pacing offload can be
> turned on and off administratively.  It also marks
> IFLA_MAX_PACING_OFFLOAD_HORIZON as NLA_REJECT and documents the new
> attribute in the rt-link YAML spec.
> 
> A few questions below.
> 
> > diff --git a/Documentation/netlink/specs/rt-link.yaml b/Documentation/netlink/specs/rt-link.yaml
> > index 5217d0bb4a3a5..7a72cd1b7e1ee 100644
> > --- a/Documentation/netlink/specs/rt-link.yaml
> > +++ b/Documentation/netlink/specs/rt-link.yaml
> > @@ -1091,6 +1091,10 @@ attribute-sets:
> >        -
> >          name: tailroom
> >          type: u16
> > +      -
> > +        name: pacing-offload
> > +        type: u32
> > +        doc: Enable EDT pacing offload (0 - disabled, 1 - enabled).
> >    -
> >      name: prop-list-link-attrs
> >      subset-of: link-attrs
> > @@ -2559,6 +2563,7 @@ operations:
> >              - devlink-port
> >              - gso-ipv4-max-size
> >              - gro-ipv4-max-size
> > +            - pacing-offload
> 
> [Severity: Low]
> This isn't something introduced by this patch, but while the shared
> getlink reply list (&link-all-attrs) is being extended here, should
> max-pacing-offload-horizon be added as well?
> 
> rtnl_fill_ifinfo() emits it unconditionally in the same nla_put chain as
> the new attribute:
> 
> 	    nla_put_uint(skb, IFLA_MAX_PACING_OFFLOAD_HORIZON,
> 			 READ_ONCE(dev->max_pacing_offload_horizon)) ||
> 
> The same holds for netns-immutable, headroom and tailroom, which are also
> always emitted but not listed in the reply set.
> 
> The commit message advertises reading the horizon with
> 
>     ynl --family rt-link --do getlink ... | grep pacing
> 
> which works, because the Python decoder resolves reply members from
> attribute-set: link-attrs, but consumers generated from the per-operation
> reply list would not see these members.

This patch follows the established behavior of other recent read-only
rt-link attrs.

It's a limitation of how get-link and set-link use the same attrs.

This is a damned if you do, damned if you don't issue where the bot
will call you out either way.

> > diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
> > index 512f6d6fa3d8f..8eceaa80b6860 100644
> > --- a/Documentation/networking/net_cachelines/net_device.rst
> > +++ b/Documentation/networking/net_cachelines/net_device.rst
> > @@ -11,6 +11,7 @@ Type                                Name                        fastpath_tx_acce
> >  unsigned_long:32                    priv_flags                  read_mostly                             __dev_queue_xmit(tx)
> >  unsigned_long:1                     lltx                        read_mostly                             HARD_TX_LOCK,HARD_TX_TRYLOCK,HARD_TX_UNLOCK(tx)
> >  unsigned_long:2                     netmem_tx:2;                read_mostly
> > +unsigned_long:1                     pacing_offload              read_mostly                             sch_fq
> >  char                                name[16]
> >  struct netdev_name_node*            name_node
> >  struct dev_ifalias*                 ifalias
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 707b2e51c2b97..1f0710eef185b 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> 
> [ ... ]
> 
> > @@ -2170,6 +2171,7 @@ struct net_device {
> >  		unsigned long		priv_flags:32;
> >  		unsigned long		lltx:1;
> >  		unsigned long		netmem_tx:2;
> > +		unsigned long		pacing_offload:1;
> >  	);
> 
> [Severity: Medium]
> This puts the first runtime-mutable bit into the priv_flags_fast group.  On
> 64-bit, priv_flags, lltx, netmem_tx and pacing_offload all live in one
> unsigned long, and adjacent bitfields are a single memory location, so a
> store to one of them is a read-modify-write of the others.
> 
> do_setlink() writes the new bit under RTNL only:
> 
> 		if (dev->pacing_offload != val) {
> 			dev->pacing_offload = val;
> 
> but priv_flags in the same word is modified from softirq context with no
> RTNL held, in net/ncsi/ncsi-rsp.c:
> 
> 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> 
> That appears in ncsi_rsp_handler_oem_bcm() and ncsi_rsp_handler_gmcma(),
> both reached from ncsi_rcv_rsp(), which is a packet_type receive handler.

The bug here is in ncsi_ imho.

The other fields are written at net_device init and immutable
otherwise. The rule for these fields is immutable or RTNL held, AFAIK.

> Can these two read-modify-writes lose each other's update, so that either
> IFF_LIVE_ADDR_CHANGE is silently dropped, or pacing_offload reads back 0
> after do_setlink() already set DO_SETLINK_MODIFIED and sent a notification?
> 
> A second point on the same field: a 1-bit bitfield cannot be accessed with
> READ_ONCE()/WRITE_ONCE(), yet the net_cachelines row added above documents
> pacing_offload as a read_mostly field consumed lockless by sch_fq.  Every
> neighbouring netdev field in these paths is accessed with
> READ_ONCE()/WRITE_ONCE(), including max_pacing_offload_horizon right next
> to it.
> 
> Would a plain bool/u8, or a dedicated unsigned long used with
> set_bit()/test_bit(), work here instead?  That would make the update
> independent of the neighbouring bits and allow marked lockless reads.

Re: [PATCH net-next v9 2/7] net_sched: sch_fq: check device pacing offload

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
> 
> - [Medium] New, undocumented netlink rejection in fq_change(): `tc qdisc
>   {add,replace,change} ... fq offload_horizon N` with N != 0 now returns…
> - [Medium] Configured vs. effective offload horizon are now inconsistent
>   across the three FQ interfaces.
> - [Low] The patch introduces a per-packet TX fast-path read of
>   `dev->max_pacing_offload_horizon` (fq_offload_horizon(),…
> - [Low] fq_offload_horizon() reads `dev->pacing_offload` as a plain,
>   unannotated load from the qdisc data path (net/sched/sch_fq.c:315,…
> 
> --
> 
> net_sched: sch_fq: check device pacing offload
> 
> FQ now consults the device's pacing offload state before applying the
> configured offload horizon, via a new fq_offload_horizon() helper that is
> evaluated once per packet in fq_classify() and fq_dequeue().  The helper
> returns 0 when the device flag is clear or when the configured horizon
> exceeds the device maximum, and fq_change() gains a new rejection when a
> non-zero horizon is requested on a device with pacing offload disabled.
> 
> A few questions below.
> 
> > diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> > index 35f940b2205df..dfc7f98b600f8 100644
> > --- a/net/sched/sch_fq.c
> > +++ b/net/sched/sch_fq.c
> > @@ -301,6 +301,26 @@ static void fq_gc(struct fq_sched_data *q,
> >  	q->stat_gc_flows += fcnt;
> >  }
> >  
> > +static u64 fq_offload_horizon(const struct Qdisc *sch,
> > +			      const struct fq_sched_data *q)
> > +{
> > +	const struct net_device *dev;
> > +	u64 offload_horizon;
> > +
> > +	offload_horizon = READ_ONCE(q->offload_horizon);
> > +	if (!offload_horizon)
> > +		return 0;
> > +
> > +	dev = qdisc_dev(sch);
> > +	if (!dev->pacing_offload)
> > +		return 0;
> 
> [Severity: Low]
> Is this read of dev->pacing_offload racy?
> 
> The two neighbouring loads in this same helper use READ_ONCE(), but
> pacing_offload is read plainly here.  It is a 1-bit bitfield inside
> struct_group(priv_flags_fast) in struct net_device, and the sibling
> rtnetlink change makes it runtime-writable:
> 
> net/core/rtnetlink.c:do_setlink() {
> 	if (tb[IFLA_PACING_OFFLOAD]) {
> 		bool val = nla_get_u32(tb[IFLA_PACING_OFFLOAD]);
> 
> 		if (dev->pacing_offload != val) {
> 			dev->pacing_offload = val;
> 	...
> }
> 
> The writer holds only RTNL, while this reader runs from fq_classify() and
> fq_dequeue() under the qdisc root lock, so the two sides share no lock and
> both accesses are plain.  Would KCSAN flag this?
> 
> Because it is a bitfield, READ_ONCE()/WRITE_ONCE() cannot be applied to it.

Since it is a bitfield, READ_ONCE/WRITE_ONCE cannot be applied, indeed.

> Would a dedicated non-bitfield member (or bitops on a separate unsigned
> long), or at minimum data_race() on this read, be preferable?
> 
> Separately, storing a single bit is a read-modify-write of the whole
> declared unsigned long storage unit, which also holds priv_flags:32,
> lltx:1 and netmem_tx:2 - fields read locklessly on the TX path.  Which
> lock is intended to serialize RMWs of that word now that one of its bits
> changes at runtime?  I did not find a non-RTNL writer of the neighbouring
> bits, so this part is speculative.
> 
> > +
> > +	if (offload_horizon > READ_ONCE(dev->max_pacing_offload_horizon))
> > +		return 0;
> > +
> > +	return offload_horizon;
> > +}
> > +
> 
> [Severity: Low]
> Should Documentation/networking/net_cachelines/net_device.rst be updated
> here?
> 
> This adds a per-packet TX read of dev->max_pacing_offload_horizon, but the
> row for that field in net_device.rst still has empty fastpath_tx_access and
> Comments columns.  The companion bit added by the preceding patch in the
> series was annotated:
> 
>   unsigned_long:1        pacing_offload    read_mostly    ...    sch_fq

This is fair. Very minor, but will do if respinning.
 
> struct net_device also carries a comment asking for the document to be kept
> in sync ("Please update the document when adding new fields").
> 
> max_pacing_offload_horizon still lives in the cold region of struct
> net_device next to irq_moder/napi_config, outside the net_device_read_tx
> cacheline group, so for qdiscs that configured an offload horizon the TX
> path now touches an extra otherwise-cold cache line per enqueue and per
> dequeue.  Was that placement considered?
> 
> >  /* Fast path can be used if :
> >   * 1) Packet tstamp is in the past, or within the pacing offload horizon.
> >   * 2) FQ qlen == 0   OR
> 
> [ ... ]
> 
> > @@ -357,6 +377,7 @@ static struct fq_flow *fq_classify(struct Qdisc *sch, struct sk_buff *skb,
> >  				   u64 now)
> >  {
> >  	struct fq_sched_data *q = qdisc_priv(sch);
> > +	u64 offload_horizon = fq_offload_horizon(sch, q);
> >  	struct rb_node **p, *parent;
> >  	struct sock *sk = skb->sk;
> >  	struct rb_root *root;
> 
> [ ... ]
> 
> > @@ -705,6 +728,7 @@ static struct fq_flow_head *fq_pband_head_select(struct fq_perband_flows *pband)
> >  static struct sk_buff *fq_dequeue(struct Qdisc *sch)
> >  {
> >  	struct fq_sched_data *q = qdisc_priv(sch);
> > +	u64 offload_horizon = fq_offload_horizon(sch, q);
> >  	struct fq_perband_flows *pband;
> >  	struct fq_flow_head *head;
> >  	struct sk_buff *skb;
> 
> [ ... ]
> 
> > @@ -1179,11 +1204,15 @@ 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) {
> > -			WRITE_ONCE(q->offload_horizon, offload_horizon);
> > -		} else {
> > +		if (offload_horizon && !qdisc_dev(sch)->pacing_offload) {
> > +			NL_SET_ERR_MSG_MOD(extack, "device pacing offload is disabled");
> > +			err = -EINVAL;
> 
> [Severity: Medium]
> Is this new configuration-time rejection intended, and should the commit
> message mention it?

Yes, this block rejecting enabling of pacing offload in FQ if the
device does not support it is intentional.

If respinning I'll add the statement to the commit msg.

> The changelog only describes runtime behaviour:
> 
>     In FQ, only offload if the feature is enabled and within the supported
>     device horizon.
> 
> but this hunk also makes
> 
>   tc qdisc replace dev X root fq offload_horizon 500us
> 
> return -EINVAL whenever qdisc_dev(sch)->pacing_offload is 0.  Before the
> patch the only gate was offload_horizon <= max_pacing_offload_horizon.
> Since the only setter of that bit is do_setlink() via IFLA_PACING_OFFLOAD,
> does this create an ordering dependency where the link attribute must be
> enabled before the qdisc can be configured?
> 
> The new fq_offload_horizon() already degrades to 0 when the device flag is
> clear, so the datapath is safe without the hard rejection:
> 
> 	dev = qdisc_dev(sch);
> 	if (!dev->pacing_offload)
> 		return 0;
> 
> Would accepting the value (and letting it stay inert until the device flag
> is enabled) be preferable to failing the command?
> 
> > +		} else if (offload_horizon >
> > +			   qdisc_dev(sch)->max_pacing_offload_horizon) {
> >  			NL_SET_ERR_MSG_MOD(extack, "invalid offload_horizon");
> >  			err = -EINVAL;
> > +		} else {
> > +			WRITE_ONCE(q->offload_horizon, offload_horizon);
> >  		}
> >  	}
> >  	if (!err) {
> 
> [Severity: Medium]
> Does this make the value reported by fq_dump() inaccurate and no longer
> re-appliable?
> 
> fq_dump() is untouched and still reports the raw stored value:
> 
> 	offload_horizon = READ_ONCE(q->offload_horizon);
> 	do_div(offload_horizon, NSEC_PER_USEC);
> 	...
> 	    nla_put_u32(skb, TCA_FQ_OFFLOAD_HORIZON, (u32)offload_horizon) ||
> 
> Consider:
> 
>   ip link set dev X pacing_offload on
>   tc qdisc replace dev X root fq offload_horizon 500us   /* stored */
>   ip link set dev X pacing_offload off                   /* do_setlink() */

The choice is a conscious one to check all three conditions at runtime

1. q->offload_horizon is set
2. dev->pacing_offload is enabled
3. dev->max_pacing_offload >= q->pacing_offload

and keeping them independent variables that are not disabled
cascadingly at runtime.

As a result, fq_dump will indeed dump the state of the fq variable,
which != whether offloading happens.

For that the user will also have to read the dev rt-link settings
and/or device counters.

I can make this explicit in the commit message.

> Nothing clears q->offload_horizon, so fq_offload_horizon() returns 0 and
> the datapath does pure software pacing while tc qdisc show still prints
> offload_horizon 500us.  Replaying that same dump then hits the new -EINVAL
> above, where it previously succeeded.
> 
> The same silent divergence appears when a device lowers
> max_pacing_offload_horizon on re-init, for example
> idpf_vport_set_pacing_offload() doing
> WRITE_ONCE(netdev->max_pacing_offload_horizon, max_horizon) with
> max_horizon possibly 0 - the "device renegotiation" case named in the
> changelog.
> 
> The uAPI comment also still promises unconditional behaviour:
> 
> include/uapi/linux/pkt_sched.h:
> 	TCA_FQ_OFFLOAD_HORIZON,	/* dequeue packets in the past, if within horizon */
> 
> Should fq_dump() report the effective horizon, or should the comment note
> the dependency on device administrative state?
> 
> One smaller inconsistency: fq_offload_horizon() uses READ_ONCE() for
> dev->max_pacing_offload_horizon while fq_change() reads the same field, and
> pacing_offload, plainly.

I will look at this
 

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

[Willem de Bruijn]

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

Re: [PATCH net-next v9 4/7] idpf: support pacing offload

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 2
> 
> - [High] idpf advertises a non-zero netdev->max_pacing_offload_horizon
>   (and therefore programs absolute CLOCK_TAI departure times into the…
> - [Medium] The EDT horizon advertised to the networking core is taken
>   verbatim from firmware and only bounds-checked against U32_MAX, while…
> - [Low] idpf_vport_set_pacing_offload() writes
>   netdev->max_pacing_offload_horizon (idpf_lib.c:832) from the…
> - [Low] In idpf_send_get_edt_caps_msg() the firmware-supplied granularity
>   is a u64 (gran_ns = le64_to_cpu(caps.tstamp_granularity_ns)) but is…
> 
> --
> 
> idpf: support pacing offload
> 
> This adds EDT (earliest departure time) support to idpf: EDT capabilities
> are negotiated over virtchnl, the negotiated horizon is published as
> netdev->max_pacing_offload_horizon, and skb->tstamp is programmed into the
> 23-bit timestamp field of the flow scheduling Tx descriptor.
> 
> A few questions below on the capability gating and on the horizon
> encoding.
> 
> > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > index 827c795afcb62..cd1b173e3b276 100644
> > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > @@ -817,6 +817,21 @@ static void idpf_attach_and_open(struct idpf_adapter *adapter)
> >  	}
> >  }
> >  
> > +static void idpf_vport_set_pacing_offload(struct idpf_vport *vport,
> > +					  struct net_device *netdev)
> > +{
> > +	struct idpf_adapter *adapter = vport->adapter;
> > +	u32 max_horizon = 0;
> > +
> > +	if (adapter->edt_caps.time_horizon_ns &&
> > +	    idpf_is_queue_model_split(vport->dflt_qv_rsrc.txq_model) &&
> > +	    !idpf_is_cap_ena(adapter, IDPF_OTHER_CAPS,
> > +			     VIRTCHNL2_CAP_SPLITQ_QSCHED))
> > +		max_horizon = adapter->edt_caps.time_horizon_ns;
> > +
> 
> [Severity: High]
> The commit message says:
> 
>     Negotiate the feature with the device using virtchnl. Support is
>     conditional on
>     ...
>     - PTP to ensure the NIC clock is synced to CLOCK_TAI.

This is the only [High] in the series.

The issue is that PTP support is expected, but this is not an explicit
condition checked before advertising EDT.

Previous versions of the commit message explicitly stated that
preconditions are not always explicitly verified (also: split-queue,
flow-based scheduling, firmware advertises EDT):

  "Do not explicitly check all preconditions. Trust the firmware to
   only advertise EDT when all are met."

The patch now verifies these three other preconditions, so I dropped
that statement from the commit message (on flagging by another bot).

Only now it flags that one condition is still not checked.
If I have to respin, I'll add back a comment.
 
> [Severity: Medium]
> Can the advertised horizon exceed what the descriptor can encode?  The
> hardware field is 23 bits of granularity units:
> 
> 	drivers/net/ethernet/intel/idpf/idpf_lan_txrx.h:
> 		/* [23:23] Horizon Overflow bit, [22:0] timestamp */
> 		u8 ts[3];

In practice this is not a concern with this hardware. The driver
does not check every condition of a potential faulty or malicious
firmware.
 
> [Severity: Low]
> Does this validate a different value than the one it then uses?  gran_ns is
> a u64, but is_power_of_2() takes unsigned long:
> 
> 	include/linux/log2.h:
> 	static __always_inline __attribute__((const))
> 	bool is_power_of_2(unsigned long n)
> 	{
> 		return n - 1 < (n ^ (n - 1));
> 	}
> 
> IDPF has no "depends on 64BIT" in its Kconfig, so on 32-bit builds the
> upper 32 bits are dropped for the check while ilog2(gran_ns) takes the
> __ilog2_u64 path on the full value.  A response of 0x100000001 truncates to
> 1, passes the check, and yields tstamp_granularity_pow2 = 32, which is
> copied into idpf_tx_queue.ts_gran_pow2 and used as the shift in
> idpf_tx_splitq_set_txtime().  The same truncation rejects a legitimate
> granularity of 2^32 on 32-bit.  Would testing the u64 directly, e.g.
> gran_ns & (gran_ns - 1), plus an upper bound, be better here?

Low severity, but good idea if respinning.
 

Re: [PATCH net-next v9 6/7] selftests: drv-net: in so_txtime tell apart sw from hw pacing

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> - [Medium] `-H` is documented in usage() as 'verify hardware offload
>   (tx)' and the commit message claims it tells software apart from…
> 
> --
> 
> selftests: drv-net: in so_txtime tell apart sw from hw pacing
> 
> This adds a '-H' flag to so_txtime.c that enables SO_TIMESTAMPING with
> SOF_TIMESTAMPING_TX_SOFTWARE, and flags an error if the software transmit
> timestamp is more than cfg_variance_us after the send start time.  It also
> extends the errqueue polling deadline by cfg_variance_us and sorts the
> getopt string and usage text alphabetically.
> 
> > diff --git a/tools/testing/selftests/drivers/net/so_txtime.c b/tools/testing/selftests/drivers/net/so_txtime.c
> > index f6f3253ed1748..9ebe9f91227c7 100644
> > --- a/tools/testing/selftests/drivers/net/so_txtime.c
> > +++ b/tools/testing/selftests/drivers/net/so_txtime.c
> 
> [ ... ]
> 
> > @@ -191,12 +193,32 @@ static int do_recv_errqueue_txtime(struct sock_extended_err *err,
> >  	return 1;
> >  }
> >  
> > -static int do_recv_errqueue(int fdt)
> > +static int do_recv_errqueue_timestamping(struct scm_timestamping *tss)
> > +{
> > +	int64_t ts;
> > +
> > +	ts = tss->ts[0].tv_sec * 1000ULL * 1000 * 1000;
> > +	ts += tss->ts[0].tv_nsec;
> > +	ts -= glob_tstart_real;
> > +	ts /= 1000;
> > +
> > +	if (ts > cfg_variance_us) {
> > +		fprintf(stderr, "sw delay %" PRId64 "us exceeds bounds\n", ts);
> > +		if (!cfg_machine_slow)
> > +			errors++;
> > +	}
> > +
> > +	return 1;
> > +}
> 
> [Severity: Medium]
> 
> Can this check actually tell software pacing apart from hardware pacing for
> every workload the next patch runs with '-H'?

Key here is "for every workload".

Yes, passing -H is superfluous for the non-paced tests.

No, that is not worth complicating the test to add a branch.
 
> The only positive criterion here is that the software transmit timestamp
> lands within cfg_variance_us (8000 us) of glob_tstart_real.  For a request
> with no delay or a negative delay there is no pacing interval to observe at
> all.  do_send_one() does not even attach SCM_TXTIME when delay_us is
> negative:
> 
> 	if (ts->delay_us >= 0) {
> 		...
> 		tdeliver = glob_tstart + ts->delay_us * 1000;
> 		...
> 		cm->cmsg_type = SCM_TXTIME;
> 
> So for the 'a,-1' and 'a,0' variants that the follow-up patch passes '-H'
> to, ts comes out near 0 and the ts > cfg_variance_us test can never fire,
> whether the transmit was hardware paced, software paced, or not paced at
> all.  Would it make sense to skip or reject '-H' when the requested delay
> is not positive, so those variants do not report a pass without having
> verified anything?
> 
> For the 10 ms variants there are only 2 ms between the requested delay and

Re: [PATCH net-next v9 7/7] selftests: drv-net: extend so_txtime with hw offload

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> - [Medium] The new `beyond_hw_horizon` variant hard-codes a 70ms txtime,
>   but `_dev_setup_pacing_offload()`…
> 
> --
> 
> selftests: drv-net: extend so_txtime with hw offload
> 
> Adds two hardware pacing offload variants of the so_txtime driver test, one
> on fq with an offload_horizon and one on pfifo_fast, plus a new
> beyond_hw_horizon testcase with a 70ms txtime. Also raises the receive
> timeout and switches the final empty-queue check to MSG_DONTWAIT.
> 
> > diff --git a/tools/testing/selftests/drivers/net/so_txtime.py b/tools/testing/selftests/drivers/net/so_txtime.py
> > index a097fae0b335a..66a87205e02db 100755
> > --- a/tools/testing/selftests/drivers/net/so_txtime.py
> > +++ b/tools/testing/selftests/drivers/net/so_txtime.py
> 
> [ ... ]
> 
> 
> [Severity: Medium]
> Is the beyond_hw_horizon expectation guaranteed on all devices that pass the
> capability gate in _dev_setup_pacing_offload()?
> 
> That helper only checks a lower bound on the device horizon:
> 
>     horizon = _HW_OFFLOAD_HORIZON_MS * 1000_000
>     if link['max-pacing-offload-horizon'] < horizon:
>         raise KsftSkipEx('pacing offload max horizon too small')
> 
> so a device advertising, say, 100ms or more is accepted. In
> test_so_txtime_pfifofast_mono_hw() the root qdisc is pfifo_fast, so there is
> no qdisc offload_horizon gate and the immediate-send decision is left to the
> driver's own horizon check, for example in idpf_tx_splitq_set_txtime():
> 
>     /* beyond offload horizon? set overflow bit only */
>     if (ts > now + horizon) {
>             tx_params->offload.desc_ts[2] =
>                     IDPF_TXD_FLOW_SCH_HORIZON_OVERFLOW_M;
> 
> On a device whose reported max-pacing-offload-horizon covers 70ms, the packet
> is paced in hardware, arrives on time, the receiver exits 0, and with
> expect_fail set by:
> 
>     expect_pass = args_tx != "a,70"
> 
> cmd.process() in tools/testing/selftests/net/lib/py/utils.py turns that into
> a failure:
> 
>     elif self.proc.returncode == 0 and expect_fail:
>         raise CmdExitZeroFailure("Command succeeded (expected fail)", self)
> 
> The timing_sensitive escape only clears expect_fail under
> KSFT_MACHINE_SLOW, so on a normal machine the failure is unconditional.

This is a good point.

With the replacement of configurable limit dev->pacing_offload_horizon
by boolean dev->pacing_offload, the horizon is now always the
max_pacing_offload_horizon.

The test fails, but perhaps for the wrong reason. Will take a look.
I have to take back my "all false positives", though this is fairly
small.