On 30 Jul 2026 at 01:49:22 PM, Aaron Conole <[email protected]> wrote:
> Add a private data slot allocated from the connection's private store > to track offload state per connection. Three states are defined: > none, added, and established. The state is set on a successful > conn_add(), cleared on conn_del(), and transitions from added to > established when conn_established() is called for the first time. > Two public helpers, ct_offload_conn_is_offloaded() and > ct_offload_conn_is_established(), expose the current state. > > Use ct_offload_conn_is_offloaded() in conntrack-tcp.c to bypass TCP > sequence-number checking for offloaded connections. > > Assisted-by: Claude Sonnet 4.6 <[email protected]> > Signed-off-by: Aaron Conole <[email protected]> > --- > lib/conntrack-tcp.c | 9 +++-- > lib/ct-offload.c | 89 +++++++++++++++++++++++++++++++++++++++++++-- > lib/ct-offload.h | 29 ++++++++++++--- > 3 files changed, 115 insertions(+), 12 deletions(-) > > diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c > index 5ab300b661..c6630223dd 100644 > --- a/lib/conntrack-tcp.c > +++ b/lib/conntrack-tcp.c > @@ -44,6 +44,7 @@ > #include "conntrack-tp.h" > #include "coverage.h" > #include "ct-dpif.h" > +#include "ct-offload.h" > #include "dp-packet.h" > #include "util.h" > > @@ -133,9 +134,11 @@ tcp_get_wscale(const struct tcp_header *tcp) > } > > static bool > -tcp_bypass_seq_chk(struct conntrack *ct) > +tcp_bypass_seq_chk(struct conntrack *ct, struct conn *conn) > + OVS_REQUIRES(conn->lock) > { > - if (!conntrack_get_tcp_seq_chk(ct)) { > + if (!conntrack_get_tcp_seq_chk(ct) || > + ct_offload_conn_is_offloaded(conn)) { > COVERAGE_INC(conntrack_tcp_seq_chk_bypass); > return true; > } > @@ -290,7 +293,7 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, > /* Acking not more than one window forward */ > && ((tcp_flags & TCP_RST) == 0 || orig_seq == src->seqlo > || (orig_seq == src->seqlo + 1) || (orig_seq + 1 == src->seqlo))) > - || tcp_bypass_seq_chk(ct)) { > + || tcp_bypass_seq_chk(ct, conn_)) { > /* Require an exact/+1 sequence match on resets when possible */ > > /* update max window */ > diff --git a/lib/ct-offload.c b/lib/ct-offload.c > index 7339b205b2..58afef7203 100644 > --- a/lib/ct-offload.c > +++ b/lib/ct-offload.c > @@ -20,6 +20,8 @@ > > #include <errno.h> > > +#include "conntrack.h" > +#include "conntrack-private.h" > #include "ovs-thread.h" > #include "util.h" > > @@ -28,6 +30,14 @@ > > VLOG_DEFINE_THIS_MODULE(ct_offload); > > +/* Private slot for storing per-connection offload state. */ > +static ct_private_id_t ct_offload_private_id = CT_PRIVATE_ID_INVALID; > + > +/* Values stored in the private slot. */ > +#define CT_OFFLOAD_STATE_NONE ((void *)(uintptr_t) 0) /* Not offloaded. */ > +#define CT_OFFLOAD_STATE_ADDED ((void *)(uintptr_t) 1) /* Offload pending. > */ > +#define CT_OFFLOAD_STATE_EST ((void *)(uintptr_t) 2) /* Established. */ > + > /* Node in the registered-provider list. */ > struct ct_offload_class_node { > const struct ct_offload_class *class; > @@ -116,16 +126,39 @@ out: > ovs_rwlock_unlock(&ct_offload_classes_rwlock); > } > > +/* ct_offload_alloc_private_slot() - allocate the per-connection private > slot. > + * > + * Called once at module init. Logs an error if the slot pool is exhausted. > */ > +static void > +ct_offload_alloc_private_slot(void) > +{ > + ct_offload_private_id = conn_private_id_alloc(NULL); > + if (ct_offload_private_id == CT_PRIVATE_ID_INVALID) { > + VLOG_ERR("failed to allocate private slot for ct offload"); > + } > +} > + > /* ct_offload_module_init() - register built-in CT offload providers. > * > * Must be called once before any connections are created. */ > void > ct_offload_module_init(void) > { > + ct_offload_alloc_private_slot(); > /* No built-in providers yet; third parties call ct_offload_register() > * directly from their own module-init routines. */ > } > > +/* ct_offload_init_for_tests() - allocate the internal private slot for > tests. > + * > + * Must not be called in production code; use ct_offload_module_init() > + * instead. */ > +void > +ct_offload_init_for_tests(void) > +{ > + ct_offload_alloc_private_slot(); > +} > + > /* Internal helpers -- callers must hold ct_offload_classes_rwlock (rdlock). > * > * When 'batched' is true the helper skips providers that implement > @@ -140,8 +173,10 @@ ct_offload_module_init(void) > static int > ct_offload_conn_add__(const struct ct_offload_ctx *ctx, bool batched) > OVS_REQ_RDLOCK(ct_offload_classes_rwlock) > + OVS_REQUIRES(ctx->conn->lock) > { > struct ct_offload_class_node *node; > + bool offloaded = false; > int ret = 0; > > LIST_FOR_EACH (node, list_node, &ct_offload_classes) { > @@ -157,16 +192,24 @@ ct_offload_conn_add__(const struct ct_offload_ctx *ctx, > bool batched) > > int error = class->conn_add(ctx); > > - if (error && !ret) { > + if (!error) { > + offloaded = true; > + } else if (!ret) { > ret = error; > } > } > > + if (offloaded) { > + conn_private_set(ctx->conn, ct_offload_private_id, > + CT_OFFLOAD_STATE_ADDED); doesn't this get skipped for batched conns? The same applies for STATE_EST > + } > + > return ret; > } > > int > ct_offload_conn_add(const struct ct_offload_ctx *ctx) > + OVS_REQUIRES(ctx->conn->lock) > { > int ret; > > @@ -184,6 +227,7 @@ ct_offload_conn_add(const struct ct_offload_ctx *ctx) > static void > ct_offload_conn_del__(const struct ct_offload_ctx *ctx, bool batched) > OVS_REQ_RDLOCK(ct_offload_classes_rwlock) > + OVS_REQUIRES(ctx->conn->lock) > { > struct ct_offload_class_node *node; > > @@ -196,10 +240,13 @@ ct_offload_conn_del__(const struct ct_offload_ctx *ctx, > bool batched) > > class->conn_del(ctx); > } > + > + conn_private_set(ctx->conn, ct_offload_private_id, > CT_OFFLOAD_STATE_NONE); > } > > void > ct_offload_conn_del(const struct ct_offload_ctx *ctx) > + OVS_REQUIRES(ctx->conn->lock) > { > ovs_rwlock_rdlock(&ct_offload_classes_rwlock); > ct_offload_conn_del__(ctx, false); > @@ -209,8 +256,15 @@ ct_offload_conn_del(const struct ct_offload_ctx *ctx) > static void > ct_offload_conn_established__(const struct ct_offload_ctx *ctx, bool batched) > OVS_REQ_RDLOCK(ct_offload_classes_rwlock) > + OVS_REQUIRES(ctx->conn->lock) > { > + if (conn_private_get(ctx->conn, ct_offload_private_id) > + != CT_OFFLOAD_STATE_ADDED) { > + return; > + } > + > struct ct_offload_class_node *node; > + bool established = false; > > LIST_FOR_EACH (node, list_node, &ct_offload_classes) { > const struct ct_offload_class *class = node->class; > @@ -219,20 +273,43 @@ ct_offload_conn_established__(const struct > ct_offload_ctx *ctx, bool batched) > continue; > } > > - if (class->conn_established) { > - class->conn_established(ctx); > + if (class->conn_established && class->conn_established(ctx)) { > + established = true; > } > } > + > + if (established) { > + conn_private_set(ctx->conn, ct_offload_private_id, > + CT_OFFLOAD_STATE_EST); > + } > } > > void > ct_offload_conn_established(const struct ct_offload_ctx *ctx) > + OVS_REQUIRES(ctx->conn->lock) > { > ovs_rwlock_rdlock(&ct_offload_classes_rwlock); > ct_offload_conn_established__(ctx, false); > ovs_rwlock_unlock(&ct_offload_classes_rwlock); > } > > +bool > +ct_offload_conn_is_offloaded(const struct conn *conn) > + OVS_REQUIRES(conn->lock) > +{ > + void *state = conn_private_get(conn, ct_offload_private_id); > + > + return state == CT_OFFLOAD_STATE_ADDED || state == CT_OFFLOAD_STATE_EST; > +} > + > +bool > +ct_offload_conn_is_established(const struct conn *conn) > + OVS_REQUIRES(conn->lock) > +{ > + return conn_private_get(conn, ct_offload_private_id) > + == CT_OFFLOAD_STATE_EST; > +} > + > /* ct_offload_conn_update__() - query the hardware last-used timestamp. > * > * Iterates over providers and returns the first non-zero timestamp returned > @@ -404,11 +481,15 @@ ct_offload_op_batch_submit(struct ct_offload_op_batch > *batch) > > switch (op->type) { > case CT_OFFLOAD_OP_ADD: > + ovs_mutex_lock(&op->ctx.conn->lock); > op->error = ct_offload_conn_add__(&op->ctx, true); > + ovs_mutex_unlock(&op->ctx.conn->lock); Here we acquire rdlock > conn->lock whereas in non batched APIs like ct_offload_conn_add() we aquire in the reverse order conn->lock > rdlock. This might not be a problem as long as only readers are involved, but with a writer (register/unregister) and new readers blocked, this has the potential to become deadlock. T1 T2(batch) T3 (reg/unreg) -- --------- -------------- lock(conn->lock); rdlock(classes_rwlock); lock(conn->lock); wrlock(classes_rwlock); rdlock(classes_rwlock); DEADLOCK: T1: holds conn->lock and waits for rdlock (T3, w/ wrlock no more allowing readers to acquire) T2: holds rdlock, waits conn->lock T3: waits readers to release (T2) Can this be a real problem? > break; > > case CT_OFFLOAD_OP_DEL: > + ovs_mutex_lock(&op->ctx.conn->lock); > ct_offload_conn_del__(&op->ctx, true); > + ovs_mutex_unlock(&op->ctx.conn->lock); > op->error = 0; > break; > > @@ -429,7 +510,9 @@ ct_offload_op_batch_submit(struct ct_offload_op_batch > *batch) > break; > > case CT_OFFLOAD_OP_EST: > + ovs_mutex_lock(&op->ctx.conn->lock); > ct_offload_conn_established__(&op->ctx, true); > + ovs_mutex_unlock(&op->ctx.conn->lock); > op->error = 0; > break; > > diff --git a/lib/ct-offload.h b/lib/ct-offload.h > index 754232730c..4971a61e53 100644 > --- a/lib/ct-offload.h > +++ b/lib/ct-offload.h > @@ -81,8 +81,9 @@ struct ct_offload_class { > * 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 *); > + * for an offloaded connection. Returns true if the connection is > + * confirmed established in hardware, false otherwise. */ > + bool (*conn_established)(const struct ct_offload_ctx *); > /* Check whether this provider can offload a connection. */ > bool (*can_offload)(const struct ct_offload_ctx *); > /* Flush all offloaded connections. */ > @@ -98,14 +99,30 @@ void ct_offload_unregister(const struct ct_offload_class > *); > /* Module initialization (register built-in providers). */ > void ct_offload_module_init(void); > > -/* Per-connection offload API that dispatches to all registered providers. */ > -int ct_offload_conn_add(const struct ct_offload_ctx *); > -void ct_offload_conn_del(const struct ct_offload_ctx *); > +/* Testing entry point: allocates the internal private slot. Not for > + * production use; call ct_offload_module_init() instead. */ > +void ct_offload_init_for_tests(void); > + > +/* Per-connection offload API that dispatches to all registered providers. > + * conn_add, conn_del, and conn_established require conn->lock to be held by > + * the caller; conn_update, can_offload, and flush do not. */ > +int ct_offload_conn_add(const struct ct_offload_ctx *ctx) > + OVS_REQUIRES(ctx->conn->lock); > +void ct_offload_conn_del(const struct ct_offload_ctx *ctx) > + OVS_REQUIRES(ctx->conn->lock); > long long ct_offload_conn_update(const struct ct_offload_ctx *); > -void ct_offload_conn_established(const struct ct_offload_ctx *); > +void ct_offload_conn_established(const struct ct_offload_ctx *ctx) > + OVS_REQUIRES(ctx->conn->lock); > bool ct_offload_can_offload(const struct ct_offload_ctx *); > void ct_offload_flush(void); > > +/* Connection state query helpers. Both require conn->lock to be held > + * by the caller. */ > +bool ct_offload_conn_is_offloaded(const struct conn *conn) > + OVS_REQUIRES(conn->lock); > +bool ct_offload_conn_is_established(const struct conn *conn) > + OVS_REQUIRES(conn->lock); > + > /* Batch offload API. > * > * The default implementation dispatches each operation individually using > the > -- > 2.51.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
