On 30 Jul 2026, at 19:49, Aaron Conole 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]>
Thanks Aaron for the changes. Some more comment below on top of
the ones from Paolo.
Cheers,
Eelco
[...]
> /* 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;
The name 'offloaded' is misleading since conn_add
returning success does not mean the connection is offloaded
in hardware. Maybe just 'conn_added'?
> int ret = 0;
>
> LIST_FOR_EACH (node, list_node, &ct_offload_classes) {
[...]
> 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;
> + }
> +
I would move the if() below the definitions, inline with other functions.
> 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;
The name 'ct_offload_conn_is_offloaded' is misleading.
CT_OFFLOAD_STATE_ADDED only means a provider accepted the
conn_add call, not that a hardware flow is installed.
CT_OFFLOAD_STATE_EST is the state that means hardware has
confirmed the connection (conn_established returned true).
This matters for tcp_bypass_seq_chk(), which skips TCP
sequence number validation based on this flag. If the
provider accepted the connection but has not actually
installed a hardware flow, skipping sequence checks is
unsafe. Should the TCP seq bypass check
ct_offload_conn_is_established() instead, since only
EST means hardware has confirmed?
Should this function be renamed to something like
'ct_offload_conn_is_tracked' to reflect what it actually
checks?
> +}
> +
> +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);
Guess these locks should have been added in the previous patch?
> op->error = ct_offload_conn_add__(&op->ctx, true);
> + ovs_mutex_unlock(&op->ctx.conn->lock);
> break;
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
