|SUCCESS| [RFC, v4, 6/6] examples/ptpclient: use shared PTP library definitions

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163587

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-05

# Review of PTP Library Patch Series

commit 20260505163858.212158-2-rajesh3.kumar at intel.com
Author: Rajesh Kumar <rajesh3.kumar at intel.com>

ptp: introduce PTP protocol library

This patch adds a new IEEE 1588-2019 Precision Time Protocol library
to DPDK, providing packet classification, header structures, and helper
functions for PTP packet processing. The library supports L2, VLAN-tagged
(single/double), UDP/IPv4, and UDP/IPv6 transports.

> diff --git a/lib/ptp/rte_ptp.c b/lib/ptp/rte_ptp.c
> new file mode 100644
> index 0000000000..5e4fc666fb
> --- /dev/null
> +++ b/lib/ptp/rte_ptp.c
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2026 Intel Corporation
> + *
> + * IEEE 1588 / PTP Protocol Library -- Implementation
> + */
> +
> +#include <eal_export.h>

Does this code compile? The header should be <rte_eal_export.h>, not
<eal_export.h>. Private headers use the format <eal_xxx.h>, but this is a
public library that should use public headers.

> +#include <rte_ether.h>
> +#include <rte_ip.h>
> +#include <rte_udp.h>
> +
> +#include "rte_ptp.h"
> +
> +/*
> + * Internal: find PTP header offset within a packet.
> + * Returns pointer to PTP header or NULL.
> + */
> +static struct rte_ptp_hdr *
> +ptp_hdr_find(const struct rte_mbuf *m)
> +{
> +	const struct rte_ether_hdr *eth;
> +	uint16_t ether_type;
> +	uint32_t offset;
> +
> +	if (rte_pktmbuf_data_len(m) < sizeof(struct rte_ether_hdr))
> +		return NULL;

Does this code correctly handle multi-segment mbufs? The documentation
states that multi-segment mbufs are not supported, but this check only
validates rte_pktmbuf_data_len() which returns the length of the first
segment. Later code uses rte_pktmbuf_mtod_offset() which can fail silently
if the offset extends beyond the first segment.

> +
> +	eth = rte_pktmbuf_mtod(m, const struct rte_ether_hdr *);
> +	ether_type = rte_be_to_cpu_16(eth->ether_type);
> +	offset = sizeof(struct rte_ether_hdr);
> +
> +	/* Strip VLAN / QinQ tags to reach the inner EtherType */
> +	if (ether_type == RTE_ETHER_TYPE_VLAN ||
> +	    ether_type == RTE_ETHER_TYPE_QINQ) {
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_vlan_hdr))
> +			return NULL;
> +		const struct rte_vlan_hdr *vlan =
> +			rte_pktmbuf_mtod_offset(m,
> +				const struct rte_vlan_hdr *, offset);

The aligned whitespace in this rte_pktmbuf_mtod_offset() call does not
follow the DPDK coding style. The second line should be indented with tabs
only, not aligned with the opening parenthesis.

> +		ether_type = rte_be_to_cpu_16(vlan->eth_proto);
> +		offset += sizeof(struct rte_vlan_hdr);
> +
> +		/* Second tag (QinQ inner or stacked VLAN) */
> +		if (ether_type == RTE_ETHER_TYPE_VLAN ||
> +		    ether_type == RTE_ETHER_TYPE_QINQ) {
> +			if (rte_pktmbuf_data_len(m) <
> +			    offset + sizeof(struct rte_vlan_hdr))
> +				return NULL;
> +			vlan = rte_pktmbuf_mtod_offset(m,
> +				const struct rte_vlan_hdr *, offset);

Same aligned whitespace issue here.

> +			ether_type = rte_be_to_cpu_16(vlan->eth_proto);
> +			offset += sizeof(struct rte_vlan_hdr);
> +		}
> +	}

Does this code handle stacked VLANs correctly? The second VLAN check only
runs once. If a packet has three or more VLAN tags, does this code
incorrectly identify the inner payload?

> +
> +	/* L2 PTP: EtherType 0x88F7 (plain, VLAN, or QinQ) */
> +	if (ether_type == RTE_PTP_ETHERTYPE) {
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ptp_hdr))
> +			return NULL;
> +		return rte_pktmbuf_mtod_offset(m,
> +			struct rte_ptp_hdr *, offset);

Aligned whitespace issue here as well.

> +	}
> +
> +	/* PTP over UDP/IPv4 (plain or VLAN-tagged) */
> +	if (ether_type == RTE_ETHER_TYPE_IPV4) {
> +		const struct rte_ipv4_hdr *iph;
> +		uint16_t ihl;
> +
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ipv4_hdr))
> +			return NULL;
> +
> +		iph = rte_pktmbuf_mtod_offset(m,
> +			const struct rte_ipv4_hdr *, offset);

Aligned whitespace.

> +		if (iph->next_proto_id != IPPROTO_UDP)
> +			return NULL;
> +
> +		ihl = (iph->version_ihl & 0x0F) * 4;

Does this code validate the IPv4 header checksum? The IPv4 header could be
corrupted, and using an unchecked IHL value could cause incorrect offset
calculations.

> +		if (ihl < 20)
> +			return NULL;
> +		offset += ihl;
> +
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_udp_hdr))
> +			return NULL;
> +
> +		const struct rte_udp_hdr *udp =
> +			rte_pktmbuf_mtod_offset(m,
> +				const struct rte_udp_hdr *, offset);

Aligned whitespace.

> +		uint16_t dst_port = rte_be_to_cpu_16(udp->dst_port);
> +
> +		if (dst_port != RTE_PTP_EVENT_PORT &&
> +		    dst_port != RTE_PTP_GENERAL_PORT)
> +			return NULL;
> +
> +		offset += sizeof(struct rte_udp_hdr);
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ptp_hdr))
> +			return NULL;
> +
> +		return rte_pktmbuf_mtod_offset(m,
> +			struct rte_ptp_hdr *, offset);

Aligned whitespace.

> +	}
> +
> +	/* PTP over UDP/IPv6 (plain or VLAN-tagged) */
> +	if (ether_type == RTE_ETHER_TYPE_IPV6) {
> +		const struct rte_ipv6_hdr *ip6h;
> +
> +		if (rte_pktmbuf_data_len(m) <
> +		    offset + sizeof(struct rte_ipv6_hdr))
> +			return NULL;
> +
> +		ip6h = rte_pktmbuf_mtod_offset(m,
> +			const struct rte_ipv6_hdr *, offset);

Aligned whitespace.

> +		if (ip6h->proto != IPPROTO_UDP)
> +			return NULL;

Does this code skip IPv6 extension headers? The documentation states that
extension headers are not traversed, but should there be explicit checks to
reject packets with extension headers rather than silently misclassifying
them?

> +
> +		offset += sizeof(struct rte_ipv6_hdr);
> +
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_udp_hdr))
> +			return NULL;
> +
> +		const struct rte_udp_hdr *udp =
> +			rte_pktmbuf_mtod_offset(m,
> +				const struct rte_udp_hdr *, offset);

Aligned whitespace.

> +		uint16_t dst_port = rte_be_to_cpu_16(udp->dst_port);
> +
> +		if (dst_port != RTE_PTP_EVENT_PORT &&
> +		    dst_port != RTE_PTP_GENERAL_PORT)
> +			return NULL;
> +
> +		offset += sizeof(struct rte_udp_hdr);
> +		if (rte_pktmbuf_data_len(m) < offset + sizeof(struct rte_ptp_hdr))
> +			return NULL;
> +
> +		return rte_pktmbuf_mtod_offset(m,
> +			struct rte_ptp_hdr *, offset);

Aligned whitespace.

> +	}
> +
> +	return NULL;
> +}

[ ... ]

> diff --git a/lib/ptp/rte_ptp.h b/lib/ptp/rte_ptp.h
> new file mode 100644
> index 0000000000..d9f1cce638
> --- /dev/null
> +++ b/lib/ptp/rte_ptp.h
> @@ -0,0 +1,339 @@

[ ... ]

> +/**
> + * Add a residence time (in nanoseconds) to the correctionField.
> + * Used by Transparent Clocks to account for relay transit delay.
> + * The correctionField uses IEEE 1588 scaled nanoseconds (48.16 fixed-point).
> + *
> + * @param hdr
> + *   Pointer to PTP header (will be modified in-place).
> + * @param residence_ns
> + *   Residence time in nanoseconds to add.
> + */
> +static inline void
> +rte_ptp_add_correction(struct rte_ptp_hdr *hdr, int64_t residence_ns)
> +{
> +	int64_t cf = (int64_t)rte_be_to_cpu_64(hdr->correction);
> +	cf += (int64_t)((uint64_t)residence_ns << 16);

Does this code correctly handle signed arithmetic overflow? If residence_ns
is a large negative value, or if the existing correctionField is near
INT64_MAX, can the addition overflow?

> +	hdr->correction = rte_cpu_to_be_64(cf);
> +}

[ ... ]

commit 20260505163858.212158-4-rajesh3.kumar at intel.com
Author: Rajesh Kumar <rajesh3.kumar at intel.com>

examples/ptp_tap_relay_sw: add software PTP relay example

[ ... ]

> diff --git a/examples/ptp_tap_relay_sw/ptp_tap_relay_sw.c
> b/examples/ptp_tap_relay_sw/ptp_tap_relay_sw.c
> new file mode 100644
> index 0000000000..591007b1d5
> --- /dev/null
> +++ b/examples/ptp_tap_relay_sw/ptp_tap_relay_sw.c

[ ... ]

> +static void
> +relay_burst(uint16_t src_port, uint16_t dst_port,
> +	    uint64_t *rx_cnt, uint64_t *rx_ptp_cnt,
> +	    uint64_t *tx_cnt, uint64_t *corr_cnt)
> +{
> +	struct rte_mbuf *bufs[BURST_SIZE];
> +	struct rte_ptp_hdr *ptp_hdrs[BURST_SIZE];
> +	uint64_t rx_ts;
> +	uint16_t nb_rx, nb_tx, i;
> +
> +	nb_rx = rte_eth_rx_burst(src_port, 0, bufs, BURST_SIZE);
> +	if (nb_rx == 0)
> +		return;
> +
> +	/* Record a single RX software timestamp for the whole burst.
> +	 * All packets in one burst arrived at essentially the same instant
> +	 * from rte_eth_rx_burst()'s perspective.
> +	 */
> +	rx_ts = sw_timestamp_ns();
> +
> +	*rx_cnt += nb_rx;
> +
> +	/*
> +	 * Pass 1: Parse each packet once and remember PTP event headers.
> +	 * This avoids taking the TX timestamp too early -- we want it as
> +	 * close to the actual rte_eth_tx_burst() call as possible.
> +	 */
> +	memset(ptp_hdrs, 0, sizeof(ptp_hdrs[0]) * nb_rx);
> +	for (i = 0; i < nb_rx; i++) {
> +		struct rte_ptp_hdr *hdr = rte_ptp_hdr_get(bufs[i]);
> +
> +		if (hdr == NULL)
> +			continue;
> +
> +		(*rx_ptp_cnt)++;
> +
> +		/* Only event messages carry timestamps that need correction */
> +		if (!rte_ptp_is_event(rte_ptp_msg_type(hdr)))
> +			continue;
> +
> +		ptp_hdrs[i] = hdr;
> +	}
> +
> +	/*
> +	 * Pass 2: Take a single TX timestamp right before transmission.
> +	 * This minimises the gap between the measured tx_ts and the
> +	 * actual kernel write inside rte_eth_tx_burst(), giving the
> +	 * most accurate residence time we can achieve with SW timestamps.
> +	 *
> +	 * residence_time = tx_ts - rx_ts
> +	 *
> +	 * Remaining untracked delays:
> +	 *   - Pre-RX:  NIC DMA - rx_burst return  (~1-5 us, unavoidable)
> +	 *   - Post-TX:  tx_ts - kernel TAP write   (~1-2 us)
> +	 * Both are symmetric for Sync and Delay_Req so they largely
> +	 * cancel in the ptp4l offset calculation.
> +	 */
> +	uint64_t tx_ts = sw_timestamp_ns();
> +	int64_t residence_ns = (int64_t)(tx_ts - rx_ts);

Does this code handle the case where rx_ts > tx_ts? If the system clock is
adjusted backward