Eelco Chaudron <[email protected]> writes: > 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.
That makes sense. > 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. ACK >> + 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. Okay, that makes sense. >> + }; >> + 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[]. ACK >> + /* 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. I think it makes sense to document it. > We should also add a similar note to the ct_offload_class > documentation for all the conn references (batched and > non-batches). Yes. >> + 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
