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

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.

[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

[ ... ]

[ ... ]

[ ... ]

[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