Re: [PATCH v5 0/2] net/af_xdp: add Rx timestamping and read_clock support

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 12 Aug 2026 23:36:34 +0000
Mark Blasko <blasko@google.com> wrote:

> This patch series introduces support for dynamic RX timestamping and
> clock querying in the AF_XDP Poll Mode Driver.
> 
> The first patch introduces three new vdev devargs to specify
> layout-agnostic metadata offsets and bitmasks for extracting hardware
> RX timestamps from XDP metadata into the mbuf dynamic timestamp field.
> 
> The second patch implements the read_clock ethdev operation, querying
> ethtool for the interface's PTP Hardware Clock index at start and using
> clock_gettime to query the NIC hardware clock time.
> ---

AI still reports some things that should be addressed.
I can fixup the long lines and check-git-log complaints if needed.


Patch 1/2: net/af_xdp: add af_xdp rx metadata and dynamic timestamping
 
Warning: rx_queue_offload_capa still claims a per-queue capability the
driver does not implement.
 
	if (internals->rx_timestamp_offset >= 0) {
		dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
		dev_info->rx_queue_offload_capa |= RTE_ETH_RX_OFFLOAD_TIMESTAMP;
	}
 
  eth_rx_queue_setup() still takes rx_conf as __rte_unused and derives
  rx_timestamp_enabled solely from dev_conf.rxmode.offloads.  An
  application that enables the offload per queue rather than port-wide
  gets a silent no-op: no dynfield write, no SIOCSHWTSTAMP, no error.
  doc/guides/nics/features.rst is explicit that "Timestamp offload"
  [uses] rte_eth_rxconf as well as rte_eth_rxmode, so claiming it in
  af_xdp.ini implies honouring rx_conf->offloads.  Either OR
  rx_conf->offloads into the condition, or drop rx_queue_offload_capa.
 
Warning: eth_af_xdp_enable_hw_timestamping() still treats any
pre-existing filter as good enough.
 
	ret = ioctl(fd, SIOCGHWTSTAMP, &ifr);
	if (ret == 0) {
		if (config.rx_filter != HWTSTAMP_FILTER_NONE) {
			close(fd);
			return 0;
		}
	}
 
  If the netdev is already set to a narrow filter -- ptp4l leaves
  HWTSTAMP_FILTER_PTP_V2_EVENT, for instance -- most packets carry no
  valid timestamp, but the PMD reports the offload as active.  With no
  validity mask configured it then copies whatever is in the metadata
  area into every mbuf and sets RTE_MBUF_F_RX_TIMESTAMP, which is worse
  than reporting nothing.  Accept only HWTSTAMP_FILTER_ALL and
  HWTSTAMP_FILTER_SOME, or at minimum log the filter that was found so
  the mismatch is diagnosable.
 
  Related: config.flags = 0 on the line below discards the flags read
  back by SIOCGHWTSTAMP.
 
Warning: doc/guides/nics/features/af_xdp.ini entry is in the wrong
place.  The order in the .ini files has to match default.ini, where
"Timestamp offload" sits between "Promiscuous mode" and "Basic stats".
It is currently immediately after "Link status".
 
Warning: parse_hex_arg() does not validate the conversion.
 
	unsigned long val = strtoul(value, &end, 16);
	if (val > UINT8_MAX) {
 
  end is passed to strtoul() and then never looked at, and errno is
  neither cleared nor checked.  "=junk" silently yields 0 and "=0x1zz"
  silently yields 1.  The zero case is caught later only when a validity
  offset was also given.  Checking end != value && *end == '\0' is two
  lines.  parse_integer_arg() above has the same gap, so if you would
  rather fix both in one place that is fine by me.
 
Info: "RX" should be "Rx" per devtools/words-case.txt, in the
af_xdp.rst sentence about the 64-bit timestamp and in the release notes
entry.
 
Info: the accepted ranges are not documented.  probe rejects a
timestamp offset outside 8..256 and a validity offset outside 1..256,
but af_xdp.rst does not say so, and the lower bound on the timestamp
offset in particular is non-obvious.  Worth one sentence per argument.
 
Info: nothing rejects a validity offset that lands inside the eight
timestamp bytes.  The example was fixed in v5, but
xdp_meta_rx_ts_offset=8,xdp_meta_valid_hint_offset=4 is still accepted
and cannot be what anyone meant.  A check at probe would be cheap.
 
Info: the CAP_NET_ADMIN and HWTSTAMP_FILTER_ALL paragraph is appended
to the xdp_meta_rx_ts_valid_mask subsection but describes behaviour of
all three arguments.  It reads as if it applied only to the mask.
 
Info: errno is read after close() in
eth_af_xdp_enable_hw_timestamping().
 
	ret = ioctl(fd, SIOCSHWTSTAMP, &ifr);
	close(fd);
 
	if (ret < 0)
		return -errno;
 
  close() may overwrite errno.  Save it before closing.
 
Info: the zc and cp paths express the same computation two different
ways (rte_pktmbuf_mtod_offset() vs manual (char *)pkt - off).  A small
static inline taking a base pointer would keep them from drifting.
 
 
Patch 2/2: net/af_xdp: add read_clock support to AF_XDP PMD
 
Info: the ptp_fd cleanup in eth_dev_close() still sits above the "out:"
label, so it is skipped on the path that does
 
	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
		goto out;
 
  and "out:" then frees process_private with the fd still in it.  This
  is unreachable today because the secondary installs dummy burst
  functions and never starts, but the patch does initialise ptp_fd in
  the secondary probe path, which implies otherwise.  Moving the close
  below "out:" and before rte_free() makes the pairing obvious and
  costs nothing.
 
Info: the PTP device is opened on every dev_start regardless of whether
the application will ever call rte_eth_read_clock(), and a failure is
logged at WARNING.  On a PTP-capable NIC without permission to open
/dev/ptpX that is a warning on every start for applications that do not
use the feature.  INFO or DEBUG would be quieter, and the -ENOTSUP from
read_clock still tells anyone who cares.