|SUCCESS| [RFC, v2, 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/163325

_AI Code Review_

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

# Review of DPDK PTP Protocol Library Patch Series

commit sha: 1531ca509056acda2826a64658779d38c61b5d26
Author: Rajesh Kumar <rajesh3.kumar at intel.com>
Subject: [RFC v2 1/6] ptp: introduce PTP protocol library

This patch introduces a new IEEE 1588-2019 PTP protocol library for DPDK,
providing packet classification, header structures, and helper functions
for PTP packet processing.

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

[ ... ]

> +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 handle multi-segment mbufs correctly? The documentation
states "multi-segment mbufs are not supported," but should
rte_pktmbuf_data_len() be replaced with rte_pktmbuf_pkt_len() in the
check, or should there be an explicit multi-segment check that logs a
warning?

> +	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);
> +		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);
> +			ether_type = rte_be_to_cpu_16(vlan->eth_proto);
> +			offset += sizeof(struct rte_vlan_hdr);
> +		}
> +	}

Can this code handle more than two VLAN tags? If the packet has a third
VLAN tag (which is valid Ethernet), does the code silently fail or
should it explicitly detect and reject this case?

> +	/* 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);
> +		if (iph->next_proto_id != IPPROTO_UDP)
> +			return NULL;
> +
> +		ihl = (iph->version_ihl & 0x0F) * 4;
> +		if (ihl < 20)
> +			return NULL;

Does this code validate that ihl does not exceed the remaining packet
length before advancing offset by ihl?

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

Does this code check the IPv4 header checksum? If a corrupted IPv4
packet arrives with a valid header length but incorrect checksum, does
this code silently accept it as PTP?

> +	/* 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);
> +		if (ip6h->proto != IPPROTO_UDP)
> +			return NULL;

The documentation states "IPv6 extension headers are not traversed."
Should this code check whether ip6h->proto is an extension header
type (IPPROTO_HOPOPTS, IPPROTO_ROUTING, etc.) and explicitly reject
those cases rather than treating them as non-UDP?

> diff --git a/lib/ptp/rte_ptp.h b/lib/ptp/rte_ptp.h
> new file mode 100644
> index 0000000000..526d7f3704
> --- /dev/null
> +++ b/lib/ptp/rte_ptp.h

[ ... ]

> +/**
> + * 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);
> +	hdr->correction = rte_cpu_to_be_64(cf);
> +}

Does this code handle overflow correctly? If cf is near INT64_MAX and
residence_ns is large and positive, can signed overflow occur?

---

commit sha: fd4fc664fd573e01ea5323348991555eaae4569f
Author: Rajesh Kumar <rajesh3.kumar at intel.com>
Subject: [RFC v2 3/6] examples/ptp_tap_relay_sw: add software PTP relay example

This patch adds a minimal PTP Transparent Clock relay between a
DPDK-bound physical NIC and a kernel TAP interface using software
timestamps only.

> 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..eb674e49b9
> --- /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)
> +{
> +	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();

Does this code introduce timestamp inaccuracy when multiple PTP packets
arrive in the same burst? The comment claims "essentially the same
instant," but if two PTP event messages are in the burst, they will
receive identical rx_ts values even though their actual arrival times
may differ by microseconds.

> +	/*
> +	 * Pass 1: Classify packets and remember PTP event header pointers.
> +	 * 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++) {
> +		int ptp_type = rte_ptp_classify(bufs[i]);
> +
> +		if (ptp_type == RTE_PTP_MSGTYPE_INVALID)
> +			continue;
> +
> +		(*rx_ptp_cnt)++;
> +
> +		/* Only event messages carry timestamps that need correction */
> +		if (!rte_ptp_is_event(ptp_type))
> +			continue;
> +
> +		ptp_hdrs[i] = rte_ptp_hdr_get(bufs[i]);
> +	}

Does this code call both rte_ptp_classify() and rte_ptp_hdr_get()
redundantly? The header comment in patch 2/6 states "calling
rte_ptp_hdr_get() alone and then using rte_ptp_msg_type() on the
returned header avoids parsing the packet twice." Is there a
performance issue here?

> +	uint64_t tx_ts = sw_timestamp_ns();
> +	int64_t residence_ns = (int64_t)(tx_ts - rx_ts);
> +
> +	for (i = 0; i < nb_rx; i++) {
> +		if (ptp_hdrs[i] == NULL)
> +			continue;
> +		rte_ptp_add_correction(ptp_hdrs[i], residence_ns);
> +		stats.corrections++;
> +	}
> +
> +	/* Forward the burst */
> +	nb_tx = rte_eth_tx_burst(dst_port, 0, bufs, nb_rx);
> +	*tx_cnt += nb_tx;
> +
> +	/* Free any unsent packets */
> +	for (i = nb_tx; i < nb_rx; i++)
> +		rte_pktmbuf_free(bufs[i]);
> +}

Can this code leak mbufs if rte_eth_tx_burst() fails partially? If
nb_tx is less than nb_rx, the code frees unsent packets, but does it
correctly handle the case where some packets were queued by the TX
burst but later dropped by the NIC?

---

commit sha: 3b557058e28c1952b920fcecfe62b8b033d1fe40
Author: Rajesh Kumar <rajesh3.kumar at intel.com>
Subject: [RFC v2 5/6] app/test: add PTP library unit tests

This patch adds autotests for the PTP library.

> diff --git a/app/test/test_ptp.c b/app/test/test_ptp.c
> new file mode 100644
> index 0000000000..5c24804975
> --- /dev/null
> +++ b/app/test/test_ptp.c

[ ... ]

> +static struct rte_mbuf *
> +build_ipv4_udp_ptp(uint8_t msg_type, uint16_t dst_port)
> +{
> +	struct rte_mbuf *m = rte_pktmbuf_alloc(ptp_mp);
> +	if (!m)
> +		return NULL;
> +
> +	uint32_t pkt_len = sizeof(struct rte_ether_hdr) +
> +			   sizeof(struct rte_ipv4_hdr) +
> +			   sizeof(struct rte_udp_hdr) +
> +			   sizeof(struct rte_ptp_hdr);
> +	uint8_t *data = (uint8_t *)rte_pktmbuf_append(m, pkt_len);
> +
> +	struct rte_ether_hdr *eth = (struct rte_ether_hdr *)data;
> +	memset(eth, 0, sizeof(*eth));
> +	eth->ether_type = rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4);
> +
> +	fill_ipv4_udp_ptp(data, sizeof(*eth), msg_type, dst_port, 400);
> +	return m;
> +}

Does this code verify that rte_pktmbuf_append() returns non-NULL before
dereferencing data? If the mempool is exhausted, can data be NULL?

> +static int
> +test_ptp_classify_ipv4_bad_ihl(void)
> +{
> +	struct rte_mbuf *m = rte_pktmbuf_alloc(ptp_mp);
> +
> +	TEST_ASSERT_NOT_NULL(m, "alloc failed");
> +
> +	uint32_t pkt_len = sizeof(