On 30 Jul 2026, at 19:49, Aaron Conole wrote:
> Currently, if a conntrack submodule wants to add per-connection
> private details, the pattern looks like:
>
> struct private_conn {
> struct conn conn_;
> ... private data ...
> }
>
> ...
>
> new_conn = xalloc(sizeof struct private_conn);
> ...
> return &new_conn->conn_;
>
> ...
>
> struct private_conn *module_conn = (struct private_conn *)conn_;
>
> This is a common pattern where the underlying allocations are
> delegated to the submodule areas, and the main processing module
> always assumes that each module allocates a conn_ storage area at the
> head of the connection struct anyway.
>
> However, this means that some storage details can't be shared in a
> convenient way between modules without leaking details about the
> underlying implementations of the module. For example, TCP based
> connections may want to share some TCP block details, but not want to
> expose the full private TCP connection module internals.
>
> To facilitate this, introduce a private storage section into
> connection objects. This will allow storing pre-defined details that
> each module can fill and guarantee some kind of compatibility without
> needing to completely expose the internals. Because this affects how
> the conn objects get initialized, add a new init routine and call it
> at all the init sites.
Thanks Aaron for taking a look at my v1 comments and fixing them.
I still have some doubts on the API part, as I think it needs better
integration with the offload providers, especially the init/open/close
lifecycle and the coupling between the ct-offload and dpif-offload layers.
Looking at this patch again, I found some minor comments. See below.
//Eelco
> ---
> lib/conntrack-icmp.c | 2 +-
> lib/conntrack-other.c | 2 +-
> lib/conntrack-private.h | 46 +++++++++
> lib/conntrack-tcp.c | 1 +
> lib/conntrack.c | 46 ++++++++-
> lib/conntrack.h | 42 ++++++++
> tests/library.at | 24 +++++
> tests/test-conntrack.c | 177 +++++++++++++++++++++++++++++++++-
> utilities/checkpatch_dict.txt | 1 +
> 9 files changed, 336 insertions(+), 5 deletions(-)
>
> diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c
> index b402970398..12b480bc00 100644
> --- a/lib/conntrack-icmp.c
> +++ b/lib/conntrack-icmp.c
> @@ -88,7 +88,7 @@ icmp_new_conn(struct conntrack *ct, struct dp_packet *pkt
> OVS_UNUSED,
> struct conn_icmp *conn = xzalloc(sizeof *conn);
> conn->state = ICMPS_FIRST;
> conn->up.tp_id = tp_id;
> -
> + conn_init(&conn->up);
nit: should we keep the blank line between setting the struct
fields and calling init functions?
> conn_init_expiration(ct, &conn->up, icmp_timeouts[conn->state], now);
> return &conn->up;
> }
> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
> index 7f3e63c384..788cbbe0e5 100644
> --- a/lib/conntrack-other.c
> +++ b/lib/conntrack-other.c
> @@ -78,7 +78,7 @@ other_new_conn(struct conntrack *ct, struct dp_packet *pkt
> OVS_UNUSED,
> conn = xzalloc(sizeof *conn);
> conn->state = OTHERS_FIRST;
> conn->up.tp_id = tp_id;
> -
> + conn_init(&conn->up);
nit: should we keep the blank line between setting the struct
fields and calling init functions?
> conn_init_expiration(ct, &conn->up, other_timeouts[conn->state], now);
>
> return &conn->up;
[...]
> +ct_private_id_t
> +conn_private_id_alloc(void (*destructor)(void *))
> +{
> + uint32_t id;
> +
> + atomic_add(&ct_private_next_id, 1u, &id);
> + if (id >= CT_CONN_PRIVATE_MAX) {
> + /* Undo the increment so the counter doesn't overflow.
> + * Because we are not supposed to call this after ct initialization,
> + * there shouldn't be an access race here. */
> + atomic_sub(&ct_private_next_id, 1u, &id);ovsrcu_exit
Does this mean single-threaded? If so, we might not even
need atomics here, like n_ct_update_hooks. All callers
run under ovsthread_once in conntrack_init(), so a plain
unsigned int/size_t with a bounds check should be sufficient.
Maybe the function comment in .h should be more strict on
when this can be used?
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> + VLOG_ERR_RL(&rl, "conntrack: all %d private storage slots are in
> use; "
> + "cannot allocate a new one", CT_CONN_PRIVATE_MAX);
Since this function is only called at init time and slot
exhaustion is permanent, should this use VLOG_ERR_ONCE
rather than a rate limiter?
> + return CT_PRIVATE_ID_INVALID;
> + }
> +
> + ct_private_slots[id].destructor = destructor;
> + return id;
> +}
> +
> /* Destroys the connection tracker 'ct' and frees all the allocated memory.
> * The caller of this function must already have shut down packet input
> * and PMD threads (which would have been quiesced). */
> @@ -1082,8 +1116,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> nc->parent_key = alg_exp->parent_key;
> }
>
> - ovs_mutex_init_adaptive(&nc->lock);
> - atomic_flag_clear(&nc->reclaimed);
> fwd_key_node->dir = CT_DIR_FWD;
> rev_key_node->dir = CT_DIR_REV;
>
> @@ -2716,6 +2748,16 @@ new_conn(struct conntrack *ct, struct dp_packet *pkt,
> struct conn_key *key,
> static void
> delete_conn__(struct conn *conn)
> {
> + uint32_t n;
> +
> + /* Invoke registered destructors for any non-NULL private slots. */
> + atomic_read_relaxed(&ct_private_next_id, &n);
A MIN(n, CT_CONN_PRIVATE_MAX) clamp would be nice here.
Just in case conn_private_id_alloc() is biting us.
> + for (uint32_t i = 0; i < n; i++) {
> + if (ct_private_slots[i].destructor && conn->private[i]) {
> + ct_private_slots[i].destructor(conn->private[i]);
> + }
> + }
> +
> free(conn->alg);
> free(conn);
> }
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index c3136e9554..0f791e75b4 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -91,6 +91,48 @@ struct nat_action_info_t {
> uint16_t nat_flags;
> };
>
> +/* Private per-connection storage slots.
> + *
> + * Modules (protocol handlers, offload interfaces, etc.) can reserve a slot
> + * at initialization time and use it to attach private data to every tracked
> + * connection. Slot IDs are small integers that index directly into a fixed-
> + * size array inside struct conn, so get/set operations are O(1) and branch-
> + * free, safe to call on the datapath fast path.
> + *
> + * Usage
> + * -----
> + * // At module initialization, allocate and store the returned id.
> + * static ct_private_id_t my_id = CT_PRIVATE_ID_INVALID;
> + * my_id = conn_private_id_alloc(my_conn_data_free);
> + * if (my_id == CT_PRIVATE_ID_INVALID) {
> + * VLOG_ERR("failed to allocate private storage slot");
return ENOSPC;
> + * }
> + *
> + * // On the fast path, caller must hold conn->lock.
> + * conn_private_set(conn, my_id, my_data);
> + * my_data = conn_private_get(conn, my_id);
> + *
> + * Thread-safety
> + * -------------
> + * The pointer slot itself is protected by conn->lock. The pointed-to data
> + * is the responsibility of the registering module.
> + */
> +
> +/* Maximum number of private storage slots available per connection. */
> +#define CT_CONN_PRIVATE_MAX 8
> +
> +typedef uint32_t ct_private_id_t;
> +
> +/* Returned by conn_private_id_alloc() when no slots remain. */
> +#define CT_PRIVATE_ID_INVALID UINT32_MAX
> +
> +/* Allocate a private storage slot. 'destructor' (may be NULL) is called
> with
> + * the stored pointer when a connection is freed; the destructor is only
> + * invoked for non-NULL slot values. Returns CT_PRIVATE_ID_INVALID on
> failure
> + * (all slots taken). Must be called before any connection is created that
> + * should carry this slot (i.e. at module initialization time). */
Should this say "conntrack module initialization time" to
be consistent with the earlier comment on the get function?
> +ct_private_id_t conn_private_id_alloc(void (*destructor)(void *));
> +
> struct conntrack *conntrack_init(void);
> void conntrack_destroy(struct conntrack *);
>
>
[...]
> --- a/tests/test-conntrack.c
> +++ b/tests/test-conntrack.c
> @@ -16,6 +16,7 @@
>
> #include <config.h>
> #include "conntrack.h"
> +#include "conntrack-private.h"
>
> #include "dp-packet.h"
> #include "fatal-signal.h"
> @@ -496,7 +497,7 @@ test_pcap(struct ovs_cmdl_context *ctx)
> ovs_pcap_close(pcap);
> }
>
> -/* ALG related testing. */
> +/* Conntrack functional testing. */
>
> /* FTP IPv4 PORT payload for testing. */
> #define FTP_PORT_CMD_STR "PORT 192,168,123,2,113,42\r\n"
> @@ -576,6 +577,169 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> conntrack_destroy(ct);
> }
>
> +/* Verify that conn_private_id_alloc() returns a valid slot ID and that the
> + * idiomatic "store the ID in a static variable at module init" pattern
> works.
> + */
The existing comments in this file place the closing */ on
the same line as the comment text. The new multi-line
comments should follow the same style.
[...]
> +/* Register a destructor, commit a real connection, attach a sentinel pointer
> + * as private data, then destroy the conntrack instance. After draining the
> + * RCU queue (ovsrcu_exit) the destructor must have been called exactly
> + * once with the sentinel value.
> + */
> +static uintptr_t ERRPTR;
> +
> +static void
> +test_private_destructor(struct ovs_cmdl_context *ctx OVS_UNUSED)
> +{
> + /* Sentinel: a non-NULL pointer value we can identify unambiguously.
> + * ERRPTR is defined above in case we want to use it in the future as
> + * a platform-agnostic and portable sentinel value rather than some
> + * hardcoded hex. */
> + void *sentinel = (void *)(uintptr_t)&ERRPTR;
The code here tries to mimic the kernel's ERR_PTR pattern, but
the cast chain is unnecessary. &ERRPTR is already a pointer
that converts to void * implicitly, so the (uintptr_t)
intermediate does a pointless round trip.
If the intent is a sentinel in the ERR_PTR style, this would
be clearer:
void *sentinel = (void *)(uintptr_t) EINVAL;
This also avoids the all uppercase variable, and the comments
can be removed.
> +
> + static ct_private_id_t dtor_id = CT_PRIVATE_ID_INVALID;
> + dtor_id = conn_private_id_alloc(record_destructor);
> + ovs_assert(dtor_id != CT_PRIVATE_ID_INVALID);
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
