On 30 Jul 2026, at 19:49, Aaron Conole wrote: > The CT offload operations API currently considers operating on a > single connection at a time. However, there may be reason to > accumulate offload API operations and execute them as a single large > batch of operations. Provide a basic batch abstraction that allows > for accumulating operations and then executing them all at once. This > will be used in an upcoming commit, especially with the ct expiration > logic. The provider also may have a batched abstraction that lets it > do a better provider based optimization. > > As part of this extension, move the lock management up a level for the > batching system to have a single bulk operations lock.
Hi Aaron, Thanks for the patch. Thinking about it again, I feel like having two APIs into the provider doing sort of the same does not sound like a good approach. What about having only a single backend API, i.e., only batch_submit()? From the library we can still have the individual calls, but it makes the code way simpler. We might need some "static" batch we can use for these single actions. WDYT? Some more comments below. //Eelco > Assisted-by: Claude Sonnet 4.6 <[email protected]> > Signed-off-by: Aaron Conole <[email protected]> > --- > lib/ct-offload.c | 268 ++++++++++++++++++++++++++++++++++++++++------- > lib/ct-offload.h | 96 ++++++++++++++++- > 2 files changed, 322 insertions(+), 42 deletions(-) > > diff --git a/lib/ct-offload.c b/lib/ct-offload.c > index e92d8f9508..7339b205b2 100644 > --- a/lib/ct-offload.c > +++ b/lib/ct-offload.c > @@ -34,12 +34,12 @@ struct ct_offload_class_node { > struct ovs_list list_node; > }; > > -/* 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; > +/* Global list of registered CT offload classes. 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_classes_rwlock = OVS_RWLOCK_INITIALIZER; Should this rename and comment update be squashed into the patch that introduced the rwlock? [...] > +/* ct_offload_op_batch_submit() - execute every operation in the batch. > + * > + * Each op's 'error' field is set to the result of the corresponding > + * per-connection dispatch. The rwlock is held for the duration of the > + * batch; providers are invoked directly rather than through the public > + * single-op wrappers to avoid repeated lock/unlock cycles. */ > +void > +ct_offload_op_batch_submit(struct ct_offload_op_batch *batch) > { > struct ct_offload_class_node *node; > + struct ct_offload_op *op; > + > + ovs_rwlock_rdlock(&ct_offload_classes_rwlock); > > - 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->flush) { > - class->flush(); > + if (class->batch_submit) { > + class->batch_submit(batch); The classes do not have a specific order, but are executed based on order of registration. If some do not support batching, the execution order might be unexpected: all batch providers run first, then non-batch providers run second. In addition, some of the non-batch functions stop further execution based on a response, but for batching we do not. Do we need to document that the batch operation should skip the operation in specific cases? Also, below we re-execute them for providers that do not support batching (ones that should now be skipped). > } > } > - ovs_rwlock_unlock(&ct_offload_rwlock); > + > + CT_OFFLOAD_BATCH_OP_FOR_EACH (idx, op, batch) { > + No need for a new line here. > + switch (op->type) { > + case CT_OFFLOAD_OP_ADD: > + op->error = ct_offload_conn_add__(&op->ctx, true); > + break; > + > + case CT_OFFLOAD_OP_DEL: > + ct_offload_conn_del__(&op->ctx, true); > + op->error = 0; > + break; > + > + case CT_OFFLOAD_OP_UPD: { > + long long ts = ct_offload_conn_update__(&op->ctx, true); > + > + op->error = ts ? 0 : EIO; > + break; > + } > + > + case CT_OFFLOAD_OP_POLICY: > + op->error = ct_offload_can_offload__(&op->ctx, true) ? 0 : EPERM; > + break; > + > + case CT_OFFLOAD_OP_FLUSH: > + ct_offload_flush__(true); > + op->error = 0; > + break; > + > + case CT_OFFLOAD_OP_EST: > + ct_offload_conn_established__(&op->ctx, true); > + op->error = 0; > + break; > + > + default: > + op->error = EINVAL; OVS_NOT_REACHED()? > + break; > + } > + } > + > + ovs_rwlock_unlock(&ct_offload_classes_rwlock); > } > diff --git a/lib/ct-offload.h b/lib/ct-offload.h > index f44bfee217..754232730c 100644 > --- a/lib/ct-offload.h > +++ b/lib/ct-offload.h > @@ -20,12 +20,12 @@ > #include "conntrack.h" > #include "conntrack-private.h" > #include "openvswitch/types.h" > +#include "util.h" > > struct netdev; > > /* Context for offload as part of the callbacks that all connection > - * offload APIs receive. > - */ > + * offload APIs receive. */ Should be fixed earlier. > struct ct_offload_ctx { > struct conn *conn; /* Connection object being offloaded. */ > struct netdev *netdev_in; /* Input netdev (may be NULL). */ > @@ -33,6 +33,29 @@ struct ct_offload_ctx { > const struct conn_key *key; /* Forward-direction 5-tuple. */ > }; > > +enum ct_offload_op_type { > + CT_OFFLOAD_OP_ADD, /* Add operation. */ > + CT_OFFLOAD_OP_DEL, /* Del operation. */ > + CT_OFFLOAD_OP_UPD, /* Update operation. */ > + CT_OFFLOAD_OP_POLICY, /* Policy check operation. */ > + CT_OFFLOAD_OP_FLUSH, /* Flush. */ The flush does not use the ctx since it is a global operation. Should we document each operation type and which ctx fields it uses, similar to the callback descriptions in struct ct_offload_class? > + CT_OFFLOAD_OP_EST, /* Established - notify that a connection > + * has a reply seen. */ > +}; > + > +struct ct_offload_op { > + enum ct_offload_op_type type; > + struct ct_offload_ctx ctx; The batch API documentation should mention that the conn and netdev_in pointers stored in each ctx must remain valid until ct_offload_op_batch_submit() returns, since the ctx is copied by value but no references are taken. > + int error; For CT_OFFLOAD_OP_UPD, the conn_update__() helper returns a last-used timestamp, but this value is discarded. Callers that need the actual timestamp cannot use the batch API. > +}; > + > +/* Batched set of offload contexts and operations. */ > +struct ct_offload_op_batch { > + struct ct_offload_op *ops; > + size_t n_ops; > + size_t allocated; > +}; > + > /* CT offload class describes a conntrack offload provider implementation. */ > struct ct_offload_class { > const char *name; > @@ -40,6 +63,11 @@ struct ct_offload_class { > /* Optional initialization routine for the provider. */ > int (*init)(void); > > + /* Interface to allow offload providers to operate in bulk. If a > provider > + * does not implement this, the fallback is to dispatch each operation > + * individually. */ > + void (*batch_submit)(struct ct_offload_op_batch *); The batch_submit callback documentation should mention that the provider is responsible for acquiring conn->lock when accessing conn->private[]. The fallback path takes the lock per-op, but batch_submit receives the raw batch with no locks held (other than the provider list rwlock). Or should the API hold all conn locks and then call the batch callback? This avoids changes to conn when executing batches, and avoids multiple lock/unlock cycles. > + > /* 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). */ > @@ -78,4 +106,68 @@ void ct_offload_conn_established(const struct > ct_offload_ctx *); > bool ct_offload_can_offload(const struct ct_offload_ctx *); > void ct_offload_flush(void); > > +/* Batch offload API. > + * > + * The default implementation dispatches each operation individually using > the > + * per-connection API above. Providers that can handle a native batch may do > + * so by implementing a batch_submit callback in struct ct_offload_class. > + * > + * Typical usage: > + * > + * struct ct_offload_op_batch batch; > + * ct_offload_op_batch_init(&batch); > + * > + * ct_offload_op_batch_add(&batch, CT_OFFLOAD_OP_ADD, &ctx_a); > + * ct_offload_op_batch_add(&batch, CT_OFFLOAD_OP_ADD, &ctx_b); > + * > + * ct_offload_op_batch_submit(&batch); > + * for_each_op inspect batch.ops[i].error > + * > + * ct_offload_op_batch_destroy(&batch); > + * > + * For CT_OFFLOAD_OP_UPD, op->error is set to 0 when the hardware returned a > + * valid last-used timestamp (expiration was refreshed by the provider), or > to > + * EIO when no hardware record was found. > + * > + * For CT_OFFLOAD_OP_POLICY, op->error is set to 0 when the connection is > + * eligible for offload, or EPERM when no provider will accept it. > + */ > +static inline void > +ct_offload_op_batch_init(struct ct_offload_op_batch *batch) > +{ > + batch->ops = NULL; > + batch->n_ops = 0; > + batch->allocated = 0; > +} Looking at the batching process, this will be a hot path, so allocating memory for every batch might be expensive. What about having some static entries defined within the object, similar to how dp_packet_batch embeds up to NETDEV_MAX_BURST packets inline? I know it increases the object size, but there might be a sweet spot between size and avoiding allocation in most cases. Maybe Gaetan has some statistics on this? [...] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
