Re: [RFC v1 0/4] introduce PTP protocol library and software relay example

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 28 Apr 2026 06:31:02 +0530
Rajesh Kumar <rajesh3.kumar@intel.com> wrote:

> This series introduces a new DPDK library (lib/ptp) for IEEE 1588-2019
> PTP protocol packet processing and a companion example application
> (ptp_tap_relay_sw) that demonstrates its usage.
> 
> Motivation
> ----------
> Several DPDK applications need to classify and manipulate PTP packets
> (e.g. ptpclient, ptp_tap_relay, custom Transparent Clocks). Today each
> application re-implements its own PTP header parsing and correctionField
> handling. A shared library avoids duplication and provides a tested,
> standards-compliant foundation.
> 
> Library: lib/ptp
> ----------------
> The library provides:
>   - PTP header structures (IEEE 1588-2019 common header, timestamp,
>     port identity)
>   - Packet classification: rte_ptp_classify() detects PTP over L2
>     (EtherType 0x88F7), VLAN-tagged L2, and UDP/IPv4 (ports 319/320)
>   - Header access: rte_ptp_hdr_get() returns a pointer to the PTP
>     header inside an mbuf
>   - Inline helpers: correctionField manipulation (48.16 fixed-point),
>     message type extraction, two-step flag check, timestamp conversion
>   - Debug: rte_ptp_msg_type_str() for human-readable message names

The things I noticed were:
Do not use older PTP terminology master/slave; we fixed this in DPDK.
This would cause a NAK for me, and checkpatch would flag it.

AI had lots more to say...

Subject: Re: [RFC v1 0/4] introduce PTP protocol library and software relay example

Some review comments on this RFC.  Overall direction (shared PTP
header+classification library) is good, but there are a couple of
correctness bugs in patch 1/4 that need to be fixed before this can
move out of RFC.

Patch 1/4: ptp: introduce PTP protocol library
==============================================

Error: flag bit positions wrong for the host-order representation
-----------------------------------------------------------------

  +#define RTE_PTP_FLAG_TWO_STEP    (1 << 1)  /**< Two-step flag (bit 1) */
  +#define RTE_PTP_FLAG_UNICAST     (1 << 2)  /**< Unicast flag */
  +#define RTE_PTP_FLAG_LI_61       (1 << 0)  /**< Leap indicator 61 */
  +#define RTE_PTP_FLAG_LI_59       (1 << 1)  /**< Leap indicator 59 (byte 1) */

  +static inline bool
  +rte_ptp_is_two_step(const struct rte_ptp_hdr *hdr)
  +{
  +     return (rte_be_to_cpu_16(hdr->flags) & RTE_PTP_FLAG_TWO_STEP) != 0;
  +}

twoStepFlag and unicastFlag live in byte 0 of the wire flag field
(IEEE 1588-2019 Table 37). leap61/leap59 live in byte 1. After
rte_be_to_cpu_16(), byte 0 of the wire ends up in the upper 8 bits of
the host value, so:

   TWO_STEP needs to be (1 << 9), not (1 << 1)
   UNICAST  needs to be (1 << 10), not (1 << 2)
   LI_61    stays at    (1 << 0)
   LI_59    stays at    (1 << 1)

As written, rte_ptp_is_two_step() always returns false for any real
two-step Sync from the wire, which silently breaks any Transparent
Clock or Boundary Clock built on this library.  TWO_STEP and LI_59
also collide on (1 << 1), which by itself shows the macro layout
isn't right.

Worth adding a unit test that constructs a packet with twoStepFlag
set and checks that the helper returns true.

Error: QinQ classification path is unreachable for standard QinQ
----------------------------------------------------------------

In ptp_hdr_find(), the VLAN block is only entered when the outer
EtherType is RTE_ETHER_TYPE_VLAN (0x8100):

  +     if (ether_type == RTE_ETHER_TYPE_VLAN) {
  ...
  +             /* Double-tagged VLAN (QinQ) */
  +             if (ether_type == RTE_ETHER_TYPE_QINQ ||
  +                 ether_type == RTE_ETHER_TYPE_VLAN) {

Standard 802.1ad QinQ uses outer S-tag 0x88A8 (RTE_ETHER_TYPE_QINQ)
followed by a C-tag 0x8100, then the payload EtherType.  Packets with
the typical S-tag outer never enter the VLAN branch at all, so the
"QinQ supported" claim in the cover letter and the prog_guide does
not match what the code does.

Either accept 0x88A8 as an entry point to the VLAN parser, or drop
the QinQ claim from the documentation.

Warning: VLAN-tagged PTP-over-UDP not handled
---------------------------------------------

The classifier handles plain L2, VLAN-L2, IPv4-UDP, and IPv6-UDP, but
there is no path for VLAN -> IPv4/IPv6 -> UDP -> PTP.  This is a
common deployment pattern.  Not fatal, but please mention it in the
limitations list if it's intentional.

Warning: signed left shift in rte_ptp_add_correction()
------------------------------------------------------

  +     int64_t cf = (int64_t)rte_be_to_cpu_64(hdr->correction);
  +     cf += (residence_ns << 16);

residence_ns is int64_t.  Left-shift of a signed value where the
shifted value is not representable in the result type is undefined
behaviour (C11 6.5.7p4).  In the relay example residence_ns is
computed from (uint64_t)tx_ts - (uint64_t)rx_ts cast to int64_t, so
clock noise or a tx_ts < rx_ts edge case turns this negative.  GCC
and clang happen to produce arithmetic shift, but please make it
well-defined:

   cf += (int64_t)((uint64_t)residence_ns << 16);

or just multiply by (1LL << 16).

Same issue, lower severity, in rte_ptp_correction_ns():

  +     return (int64_t)rte_be_to_cpu_64(hdr->correction) >> 16;

Right-shift of a negative signed value is implementation-defined
(not UB), but a divide expresses intent more clearly.

Info: include order
-------------------

  +#include "rte_ptp.h"
  +
  +#include <eal_export.h>

eal_export.h is conventionally included with the other system /
EAL headers, before the local rte_ptp.h.

Info: overlap with examples/ptpclient
-------------------------------------

ptpclient.c defines its own struct ptp_header / ptp_message.  Since
this series adds a shared library, it would be worth converting
ptpclient as part of the series (or in a follow-up) to avoid
duplicating the same definitions in-tree.


Patch 2/4: doc: add PTP library programmer's guide
==================================================

Just the QinQ wording carryover - prog_guide claims "Double VLAN
(QinQ) + EtherType 0x88F7" but the code as posted only handles
0x8100/0x8100 nesting.  Update either the code or the doc to match.


Patch 3/4: examples/ptp_tap_relay_sw
====================================

Warning: promiscuous enable failure on PHY port is not fatal
------------------------------------------------------------

  +     ret = rte_eth_promiscuous_enable(port);
  +     if (ret != 0)
  +             LOG_ERR("Failed to enable promiscuous on port %u", port);
  +
  +     return 0;

For the PHY port this is what lets the relay see the PTP multicast
group (01:1B:19:00:00:00 / 01:80:C2:00:00:0E).  If it fails, the
relay starts up looking healthy but never receives anything.  At
least exit on failure for the PHY port, or enable allmulticast
explicitly so the failure mode is visible.

Info: per-burst single timestamp
--------------------------------

A single rx_ts and tx_ts is taken for the whole burst.  At
logSyncInterval=-4 (16 packets/sec) bursts are essentially always 1
packet so this doesn't matter, but at higher rates the early
packets in a burst get under-corrected and the late ones
over-corrected by up to a burst's worth of poll latency.  Worth
calling out in the comment block; the test results already imply
single-packet bursts.

Info: missing release notes for new library
-------------------------------------------

Targeted at 26.07 per RTE_EXPORT_EXPERIMENTAL_SYMBOL.  A new library
should add an entry under "New Features" in
doc/guides/rel_notes/release_26_07.rst.