On 18 May 2026, at 19:13, Aaron Conole wrote: > This connects the offload provider infrastructure with the > various places in conntrack where connection status changes > take place. > > Batches are only used in one place at the moment - during the expiration > list traversal in ct_sweep. Future batch uses may be when processing > the packet batch in the main conntrack_execute loop, but that isn't part > of this series.
Hi Aaron, I think this is the main point for having the batch API. Having batch support from conntrack_execute() down should be part of this series, maybe as a separate patch. The two call sites in process_one() (conn_add and conn_established) are already inside a per-packet loop, so accumulating them into a batch and submitting once after the loop is straightforward. This would also let us drop the individual public APIs entirely and only maintain the batch API. See more comments below. //Eelco > Signed-off-by: Aaron Conole <[email protected]> > --- > lib/conntrack.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 76 insertions(+), 3 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 7eca4abf6b..6ebae26411 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -27,6 +27,7 @@ > #include "conntrack-tcp.h" > #include "conntrack-tp.h" > #include "coverage.h" > +#include "ct-offload.h" > #include "crc32c.h" > #include "csum.h" > #include "ct-dpif.h" > @@ -533,6 +534,13 @@ conn_clean(struct conntrack *ct, struct conn *conn) > atomic_count_dec(&zl->czl.count); > } > I guess the 'if (ct_offload_conn_is_offloaded(conn)) {' should be part of this patch. > + struct ct_offload_ctx offload_ctx = { > + .conn = conn, > + .netdev_in = NULL, > + .input_port_id = ODPP_NONE, > + }; > + ct_offload_conn_del(&offload_ctx); > + > ovsrcu_postpone(delete_conn, conn); > atomic_count_dec(&ct->n_conn); > } > @@ -1383,6 +1391,31 @@ process_one(struct conntrack *ct, struct dp_packet > *pkt, > helper, alg_exp, ct_alg_ctl, tp_id); > } > ovs_mutex_unlock(&ct->ct_lock); > + > + if (conn) { > + struct ct_offload_ctx offload_ctx = { > + .conn = conn, > + .netdev_in = NULL, > + .input_port_id = pkt->md.in_port.odp_port, I think for hardware offload the original input port is needed, so md.orig_in_port rather than md.in_port.odp_port. Same for other places this information is stored. > + }; > + ct_offload_conn_add(&offload_ctx); > + } > + } > + > + if (!create_new_conn && conn && ctx->reply && > + (pkt->md.ct_state & CS_ESTABLISHED) && > + ct_offload_conn_is_offloaded(conn) && > + !ct_offload_conn_is_established(conn)) { We need to hold conn->lock here, as both functions read conn->private[]. > + /* Notify offload providers that the connection is established. > + * We use the reply bit to detect that the connection has > + * transitioned and give us the input port, which should be the > + * reverse direction port. */ > + struct ct_offload_ctx offload_ctx = { > + .conn = conn, > + .netdev_in = NULL, > + .input_port_id = pkt->md.in_port.odp_port, > + }; > + ct_offload_conn_established(&offload_ctx); > } > > write_ct_md(pkt, zone, conn, &ctx->key, alg_exp); > @@ -1528,19 +1561,59 @@ ct_sweep(struct conntrack *ct, struct rculist *list, > long long now, > size_t *cleaned_count) > OVS_NO_THREAD_SAFETY_ANALYSIS > { > + struct ct_offload_op_batch batch; > + struct ct_offload_op *op; > + > struct conn *conn; > size_t cleaned = 0; > size_t count = 0; > > + > + ct_offload_op_batch_init(&batch); > + > RCULIST_FOR_EACH (conn, node, list) { > if (conn_expired(conn, now)) { > - conn_clean(ct, conn); > - cleaned++; > + if (!ct_offload_conn_is_offloaded(conn)) { > + conn_clean(ct, conn); > + cleaned++; > + } else { > + struct ct_offload_ctx offload_ctx = { > + .conn = conn, > + .netdev_in = NULL, > + .input_port_id = ODPP_NONE, > + }; > + > + ct_offload_op_batch_add(&batch, CT_OFFLOAD_OP_UPD, > + &offload_ctx); > + } > } > - > count++; > } > > + /* Run the batch. */ > + ct_offload_op_batch_submit(&batch); > + > + CT_OFFLOAD_BATCH_OP_FOR_EACH (idx, op, &batch) { > + struct conn *c = CONST_CAST(struct conn *, op->ctx.conn); > + Should we document that it is the caller's responsibility to verify the conn object is still active when processing batch results? The batch stores raw pointers and does not check liveness. We should also add a similar note to the ct_offload_class documentation for all the conn references (batched and non-batches). > + if (op->error) { > + conn_clean(ct, c); > + cleaned++; > + } else { > + /* Extend expiration by one sweep interval from now so the > + * connection survives until the next pass. */ > + long long new_exp = now + conntrack_get_sweep_interval(ct); > + long long cur; > + > + atomic_read_relaxed(&c->expiration, &cur); > + if (new_exp > cur) { > + atomic_store_relaxed(&c->expiration, new_exp); > + } > + } > + } > + > + ct_offload_op_batch_destroy(&batch); > + > if (cleaned_count) { > *cleaned_count = cleaned; > } > -- > 2.53.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
