On 30 Jul 2026, at 19:49, Aaron Conole wrote:
> This adds the basic primitives, initialization, and operations that
> conntrack offload providers will need to implement in order to
> offer a path to offloading.
>
> Assisted-by: Claude Sonnet 4.6 <[email protected]>
> Signed-off-by: Aaron Conole <[email protected]>
Hi Aaron, thanks for the v2. See some comment below.
In addition maybe add 'ctx' and 'rwlock' to the checkpatch
dictionary?
//Eelco
[...]
> +
> +/* Global list of registered CT offload classes and a rwlock to protect it.
> + * Write lock is held only during register/unregister; fast-path operations
> + * hold the read lock so multiple PMD threads can iterate concurrently. */
> +static struct ovs_rwlock ct_offload_rwlock = OVS_RWLOCK_INITIALIZER;
> +static struct ovs_list ct_offload_classes
> + OVS_GUARDED_BY(ct_offload_rwlock)
> + = OVS_LIST_INITIALIZER(&ct_offload_classes);
> +
> +
Single newline?
> +/* ct_offload_register() - register a CT offload provider class.
> + *
> + * Calls class->init() if provided. Returns 0 on success or a positive
> + * errno value on failure. Attempting to register the same class twice
> + * returns EEXIST. */
> +int
> +ct_offload_register(const struct ct_offload_class *class)
[...]
> +
> +/* ct_offload_conn_add() - notify all eligible providers of a new connection.
> + *
> + * Iterates over registered providers and calls conn_add() on each one that
> + * reports can_offload() == true for this context. Returns the first
> non-zero
> + * error encountered, but continues notifying remaining providers. This
> allows
> + * the underlying hardware conntrack details across providers function. */
Is the return value needed here? The only caller in
conntrack.c discards it, and the tests do too. Would it
be enough to log the error at DBG level inside the
function instead?
> +int
> +ct_offload_conn_add(const struct ct_offload_ctx *ctx)
> +{
> + struct ct_offload_class_node *node;
> + int ret = 0;
> +
> + ovs_rwlock_rdlock(&ct_offload_rwlock);
> + LIST_FOR_EACH (node, list_node, &ct_offload_classes) {
> + const struct ct_offload_class *class = node->class;
> +
> + if (!class->can_offload(ctx)) {
> + continue;
> + }
> +
> + int error = class->conn_add(ctx);
> +
> + if (error && !ret) {
> + ret = error;
> + }
> + }
> + ovs_rwlock_unlock(&ct_offload_rwlock);
> +
> + return ret;
> +}
[...]
> +/* Context for offload as part of the callbacks that all connection
> + * offload APIs receive.
> + */
Close comment should go on the previous line.
> +struct ct_offload_ctx {
> + struct conn *conn; /* Connection object being offloaded. */
> + struct netdev *netdev_in; /* Input netdev (may be NULL). */
> + odp_port_t input_port_id; /* ODP port number. */
> + const struct conn_key *key; /* Forward-direction 5-tuple. */
> +};
> +
> +/* CT offload class describes a conntrack offload provider implementation. */
> +struct ct_offload_class {
> + const char *name;
> +
> + /* Optional initialization routine for the provider. */
> + int (*init)(void);
> +
> + /* Per-connection operation callbacks get called for individual
> operations
> + * on the fast path or when batching is not in use.
> + * conn_add, conn_del, and can_offload are mandatory (non-NULL). */
Can we add a bit more explanation on what these callbacks are for.
> + int (*conn_add)(const struct ct_offload_ctx *);
> + void (*conn_del)(const struct ct_offload_ctx *);
> +
> + /* Populate the last-used timestamp for the connection. Returns the
> + * last-used time in milliseconds since epoch, or 0 if the connection
> + * is not offloaded or the timestamp is not available. The caller only
> + * updates the connection expiration if the returned value is newer than
> + * the current expiration. */
How do we get the current expiration?
> + long long (*conn_update)(const struct ct_offload_ctx *);
> + /* Called exactly once when the first reply-direction packet is seen
> + * for an offloaded connection. */
> + void (*conn_established)(const struct ct_offload_ctx *);
Is conn_established intentionally optional? If all
providers are expected to implement it, making it mandatory
removes a NULL check on the fast path. Are there providers
that would not need to know about the established
transition?
> + /* Check whether this provider can offload a connection. */
What does 'can offload' mean exactly? At this point the
provider may not have all the details it needs to make a
definitive decision (e.g., the egress interface is not yet
known). Should we document what the expected behavior is
when the provider is uncertain?
> + bool (*can_offload)(const struct ct_offload_ctx *);
> + /* Flush all offloaded connections. */
> + void (*flush)(void);
I mentioned this before, but we should add some granularity
to the flush. A full flush would cause problems, as it
would probably result in a flush of all offloads across
NICs.
Since this has no callers, should we remove it for now and
re-add it with proper granularity when it is needed?
> +};
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
