[PATCH v6] api: packet: redefine packet data sharing with packet references #2287

JannePeltonen · 2025-11-11T09:48:04Z

Current API regarding dynamic packet references is not easy to efficiently support in some ODP implementations with HW accelerated packet output due to the need to update reference counts after TX completion and based on that conditionally free packet buffers. odp_packet_ref() has been implemented only as a full packet copy in all ODP implementation.

Redefine the semantics of odp_packet_ref() and the packets involved in packet data sharing in a way that hopefully makes it simpler to support packet data sharing with restrictions in packet output and with per-packet reference counting instead of per-segment reference counting. The new semantics also enable use cases that were not previously possible.

Specify better the rules regarding packet data layout manipulation operations, allowing private packet data also in the tail of the packets that reference other packets. Pull and truncate operations may involve shared packet data, in which case they just remove that data from the packet, not affecting the packet owning the data. All extend and truncate operations always produce non-shared packet data.

Change odp_packet_has_ref() to indicate whether a packet is referenced by another packet, not whether the packet shares data with another packet. In partivular, odp_packet_ref() returns a packet that shares data with the given packet but is not itself referenced by other packets. Specify that packet data and layout of packets referenced by other packets can not be modified at all.

Introduce pktio capability bits to indicate whether a pktio supports sending packets with shared data without using the dont_free flag. Sending with the dont_free flag is supported with all types of packets.

ashwinyes · 2025-11-20T06:34:52Z

include/odp/api/spec/packet.h

+ * Packets returned by this function can be used normally in ODP API functions
+ * unless otherwise noted. There are restrictions in packet output.
+ *
+ * Referenced packets can be used in ODP API functions that treat packet data


I think we should use one word to refer. Either base packet or referenced packet.

Changed in v2 so that we talk about referenced packets, not base packets.

ashwinyes · 2025-11-20T06:37:21Z

include/odp/api/spec/packet.h

+ * of a static reference it also shares metadata. Packet data and data layout
+ * of packet that have references must be treated as read only.
+ *
+ * Packets created through odp_packet_ref() or odp_packet_ref_hdr() share data


odp_packet_ref_hdr() --> odp_packet_ref_pkt()

Fixed in v2

JannePeltonen · 2025-12-02T14:08:44Z

v2: Many text clarifications and typo fixes. Removed base packet from terminology. Made the packet I/O capabilities more granular. Added reference related text in odp_packet_concat() specification. Added a new function to test whether a packet is a referencing packet. Mentioned that the functions that create referencing packets and static references can be called simultaneously in multiple threads for the same packet.

JannePeltonen · 2025-12-09T11:11:08Z

v3: Specified that sending shared packets to TM must obey the restrictions of the egress pktio (but currently TM does not support the dont_free flag, which maybe needs to change).

JannePeltonen · 2025-12-10T17:04:52Z

v4:

Renamed odp_pktout_shared_packet_capability_t to odp_pktout_packet_ref_capability_t and rewrote the description for better clarity.
Renamed odp_pktio_capability_t::shared_pkt_consume to odp_pktio_capability_t::packet_ref
Added text to odp_packet_free() to describe handling of packet references
Removed the restriction that the ODP_PACKET_FREE_CTRL_DONT_FREE may not be set in packets with multiple references.
Added TM capability bits that tell whether different types of packet references may be enqueued to TM. Since TM capabilities are separate for different egresses or TM systems, the capability to handle packet references can depend on the underlying pktio. The don't_free packet option continues to not have an effect in odp_tm_enqueue().
fixed a couple of typos

TuomasTaipale · 2025-12-15T10:25:31Z

include/odp/api/spec/ipsec.h

+ * Packets that share data with other packets may not be sent using this
+ * function unless the pktout to be used has the relevant capability.
+ * See odp_pktio_capability_t::packet_ref.
+ *


Similar to the the odp_tm_enq() spec, I think the "what you can do" style is more understandable (instead of the "what you cannot do").

Could be useful also to use the "referenced" and "referencing" terms here as with odp_tm_enq().

reworded in v5

TuomasTaipale · 2025-12-15T10:26:59Z

include/odp/api/spec/packet.h

 *
 * Frees the packet into the packet pool it was allocated from.
 *
+ * Packets that participate in packet data sharing (see odp_packet_ref()


Same here, maybe use "referenced" and "referencing" terms instead of "Packets that participate in packet data sharing"?

reworded in v5

TuomasTaipale · 2025-12-15T10:28:41Z

include/odp/api/spec/packet.h

+ *
+ * - If the packet being freed is referenced by other packets, then the packet
+ *   is not actually put back to the pool until there are no more references
+ *   to it left. This is done using implementation internal reference counting.


Unnecessary perhaps "This is done using implementation internal reference counting."?

reworded the whole paragraph in v5

TuomasTaipale · 2025-12-15T10:29:43Z

include/odp/api/spec/packet.h

+ *   packets or packet segments are also freed to the packet pool if this
+ *   packet was the last reference to them.
+ *
+ * The packet handle may not be used in any ODP API function after this call.


Perhaps just "The packet handle should not be used after this call."

reworded in v5

TuomasTaipale · 2025-12-15T10:32:03Z

include/odp/api/spec/packet.h

+ * the result packet will also be a normal packet. If the destination packet
+ * is a referencing packet, the result packet will also be a referencing packet.
+ * Otherwise the result packet is either a normal or referencing packet
+ * depending on the packets and the ODP implementation.


The "ODP" is probably unnecessary.

removed in v5

TuomasTaipale · 2025-12-15T11:05:43Z

include/odp/api/spec/packet.h

+/**
+ * Test if a packet references another packet
+ *
+ * Return nonzero if the packet references another packet.


@retval defines "1" for this case so this could also say "1" instead of "nonzero".

changed in v5

TuomasTaipale · 2025-12-15T11:10:03Z

include/odp/api/spec/packet_io.h

+ * Packets that share data with other packets may be sent without the
+ * dont_free option only if the relevant packet_ref capability is set
+ * (see odp_pktout_packet_ref_capability_t).
+ *


Just use the same text as in odp_pktout_event_queue() and odp_pktout_send() spec?

TuomasTaipale · 2025-12-15T11:13:10Z

include/odp/api/spec/packet_io_types.h

+ *  left).
+ *
+ *  If these capabilities are not set, then the respective packet types may be
+ *  sent only if the pktout supports the dont_free option and the option is set


Could have the ' ' around "dont_free".

added in v5

TuomasTaipale · 2025-12-15T11:16:21Z

include/odp/api/spec/packet_io_types.h

+typedef struct {
+	uint8_t static_ref  :1; /**< Static packet references can be consumed */
+	uint8_t referenced  :1; /**< Referenced packets can be consumed */
+	uint8_t referencing :1; /**< Referencing packets can be consumed */
+
+} odp_pktout_packet_ref_capability_t;


Better have the same style used elsewhere in file (spec above the field, newlines after fields):

typedef struct { /** Static packet references can be consumed */ uint8_t static_ref :1; /** Referenced packets can be consumed */ uint8_t referenced :1; /** Referencing packets can be consumed */ uint8_t referencing :1; } odp_pktout_packet_ref_capability_t;

Same comment for the traffic manager capa structure.

I do not wee why it would be better. It would just take more space, do did not change it in v5. There are also a few existing instances of doxygen comments in the same line, especially in the traffic manager API header.

Argee with Tuomas that we should prefer /** style. I think we have cleaned up /**< comments away over the years, but there are still some left.

Rationale being that as soon as someone need to add another sentence into one of those one liner comments. That format becomes ugly/unreadable, and that person would need to modify all other comments into normal format as well.

Changed in v6.

TuomasTaipale · 2025-12-15T11:18:20Z

include/odp/api/spec/packet_io.h

 * are enqueued. Application cannot dequeue from these queues.
 *
+ * Static references, referencing and referenced packets may be sent without
+ * the dont_free option only if the relevant packet_ref capability is set


Refer to the full path of dont_free?

I am not sure what path you mean but added a reference to odp_packet_free_ctrl_set() in v5.

psavol · 2025-12-22T15:13:03Z

include/odp/api/spec/packet.h

 *
+ * Packets that participate in packet data sharing (see odp_packet_ref()
+ * and odp_packet_ref_static()) may not get freed back to the packet pool
+ * immediately when this function is called:


Since reference to a packet is a special case, only minimal documentation of it should be here and majority under e.g. odp_packet_ref().

E.g. here we could just say: "In case of packet references, the referenced packet(s) is freed back to the packet pool only after there are no more references to it."

Shortened and reworded in v5.

I avoided saying that the packet won't be freed until references to it disappear so that the text cannot be interpreted to mean that everything gets postponed until that point and that the packet could still be used (and be in a not-freed state) until that.

JannePeltonen · 2025-12-23T12:43:40Z

v5: Updated based on review comments. Text clarifications only, no semantic changes.

psavol · 2025-12-23T13:33:38Z

include/odp/api/spec/packet.h

+ *
+ * The packet passed to this function must not reference another packet or
+ * be a static reference (see odp_packet_ref_static()). It can already be
+ * a referenced packet.


Function documentation would be cleared for the (first time) reader, if this limitation is stated in the beginning. I'd move this just after the first paragraph (definition of referenced/referencing) and change:
"... must not reference another packet..." -> "...must not be a referencing packet or a static reference..."

Moved and changed in v6

psavol · 2025-12-23T13:44:33Z

include/odp/api/spec/packet.h

- *               reference. Must be a unique reference.
+ * @param hdr    Handle of the header packet to be prefixed
 *
 * @return New reference the reference packet


Something wrong in our current return value documentation. This and odp_packet_ref() return value could be documented as "Handle of the newly created referencing packet".

Fixed in v6

JannePeltonen · 2025-12-31T11:32:17Z

v6: cosmetic changes based on review comments, rebased

Current API regarding dynamic packet references is not easy to efficiently support in some ODP implementations with HW accelerated packet output due to the need to update reference counts after TX completion and based on that conditionally free packet buffers. odp_packet_ref() has been implemented only as a full packet copy in all ODP implementations. Redefine the semantics of odp_packet_ref() and the packets involved in packet data sharing in a way that hopefully makes it simpler to support packet data sharing with restrictions in packet output and with per-packet reference counting instead of per-segment reference counting. The new semantics also enable use cases that were not previously possible. Specify better the rules regarding packet data layout manipulation operations, allowing private packet data also in the tail of the packets that reference other packets. Pull and truncate operations may involve shared packet data, in which case they just remove that data from the packet, not affecting the packet owning the data. All extend and truncate operations always produce non-shared packet data. Change odp_packet_has_ref() to indicate whether a packet is referenced by another packet, not whether the packet shares data with another packet. In particular, odp_packet_ref() returns a packet that shares data with the given packet but is not itself referenced by other packets. Specify that packet data and layout of packets referenced by other packets cannot be modified at all. Add new odp_packet_is_referencing() to test whether a packet references another packet. Introduce pktio capability bits to indicate whether a pktio supports sending packets with shared data without using the dont_free flag. Sending with the dont_free flag is supported with all types of packets. Signed-off-by: Janne Peltonen <[email protected]>

odpbuild changed the title ~~api: packet: redefine packet data sharing~~ [PATCH v1] api: packet: redefine packet data sharing Nov 11, 2025

MatiasElo added this to the v1.49.0 milestone Nov 19, 2025

ashwinyes reviewed Nov 20, 2025

View reviewed changes

JannePeltonen force-pushed the api-pkt-ref branch from fc5ce20 to 3a94a2c Compare December 2, 2025 14:02

odpbuild changed the title ~~[PATCH v1] api: packet: redefine packet data sharing~~ [PATCH v2] api: packet: redefine packet data sharing Dec 2, 2025

JannePeltonen force-pushed the api-pkt-ref branch from 3a94a2c to 3a39a0c Compare December 9, 2025 11:09

odpbuild changed the title ~~[PATCH v2] api: packet: redefine packet data sharing~~ [PATCH v3] api: packet: redefine packet data sharing Dec 9, 2025

JannePeltonen force-pushed the api-pkt-ref branch from 3a39a0c to a728f44 Compare December 10, 2025 16:59

odpbuild changed the title ~~[PATCH v3] api: packet: redefine packet data sharing~~ [PATCH v4] api: packet: redefine packet data sharing Dec 10, 2025

JannePeltonen marked this pull request as ready for review December 12, 2025 12:05

TuomasTaipale reviewed Dec 15, 2025

View reviewed changes

psavol reviewed Dec 22, 2025

View reviewed changes

JannePeltonen force-pushed the api-pkt-ref branch from a728f44 to 1fec78c Compare December 23, 2025 12:42

odpbuild changed the title ~~[PATCH v4] api: packet: redefine packet data sharing~~ [PATCH v5] api: packet: redefine packet data sharing Dec 23, 2025

psavol reviewed Dec 23, 2025

View reviewed changes

MatiasElo removed this from the v1.49.0 milestone Dec 29, 2025

JannePeltonen force-pushed the api-pkt-ref branch from 1fec78c to 25f5e64 Compare December 31, 2025 11:33

odpbuild changed the title ~~[PATCH v5] api: packet: redefine packet data sharing~~ [PATCH v6] api: packet: redefine packet data sharing Dec 31, 2025

JannePeltonen changed the title ~~[PATCH v6] api: packet: redefine packet data sharing~~ [PATCH v6] api: packet: redefine packet data sharing with packet references Dec 31, 2025

[PATCH v6] api: packet: redefine packet data sharing with packet references #2287

Are you sure you want to change the base?

[PATCH v6] api: packet: redefine packet data sharing with packet references #2287

Uh oh!

Conversation

JannePeltonen commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JannePeltonen commented Dec 2, 2025

Uh oh!

JannePeltonen commented Dec 9, 2025

Uh oh!

JannePeltonen commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psavol Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JannePeltonen commented Dec 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JannePeltonen commented Dec 10, 2025 •

edited

Loading

psavol Dec 23, 2025 •

edited

Loading