On 30 Jul 2026, at 19:49, 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.
>
> Assisted-by: Claude Sonnet 4.6 <[email protected]>
> Signed-off-by: Aaron Conole <[email protected]>
See some comments below.
//Eelco
> ---
> lib/conntrack.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 90 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index d183d4920b..fbc7e43dc6 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,18 @@ conn_clean(struct conntrack *ct, struct conn *conn)
> atomic_count_dec(&zl->czl.count);
> }
>
> + struct ct_offload_ctx offload_ctx = {
> + .conn = conn,
> + .netdev_in = NULL,
> + .input_port_id = ODPP_NONE,
> + .key = &conn->key_node[CT_DIR_FWD].key,
> + };
> + ovs_mutex_lock(&offload_ctx.conn->lock);
> + if (ct_offload_conn_is_offloaded(offload_ctx.conn)) {
Should ct_offload_conn_is_offloaded() be part of the
ct_offload_conn_del() API? Every caller guards with this
check, so it makes more sense to do it internally.
> + ct_offload_conn_del(&offload_ctx);
> + }
> + ovs_mutex_unlock(&offload_ctx.conn->lock);
> +
> ovsrcu_postpone(delete_conn, conn);
> atomic_count_dec(&ct->n_conn);
> }
> @@ -1382,6 +1395,38 @@ 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.orig_in_port,
> + .key = &conn->key_node[CT_DIR_FWD].key,
Is key the right thing to pass here? Would passing the
direction enum (CT_DIR_FWD/CT_DIR_REV) be better? It can
then also easily figure out the other direction key if
needed.
> + };
> + ovs_mutex_lock(&offload_ctx.conn->lock);
> + ct_offload_conn_add(&offload_ctx);
> + ovs_mutex_unlock(&offload_ctx.conn->lock);
> + }
> + }
> +
> + if (!create_new_conn && conn && ctx->reply &&
> + (pkt->md.ct_state & CS_ESTABLISHED)) {
> + /* 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.orig_in_port,
> + .key = &conn->key_node[CT_DIR_FWD].key,
> + };
> + ovs_mutex_lock(&offload_ctx.conn->lock);
> + if (ct_offload_conn_is_offloaded(offload_ctx.conn) &&
> + !ct_offload_conn_is_established(offload_ctx.conn)) {
> + ct_offload_conn_established(&offload_ctx);
> + }
> + ovs_mutex_unlock(&offload_ctx.conn->lock);
> }
>
> write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
> @@ -1527,19 +1572,61 @@ 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)) {
Should this check is_established instead of is_offloaded?
A connection in ADDED state is not confirmed in hardware,
so querying it for a last-used timestamp will return 0 and
just delay expiration by one sweep interval.
> + conn_clean(ct, conn);
> + cleaned++;
> + } else {
> + struct ct_offload_ctx offload_ctx = {
> + .conn = conn,
> + .netdev_in = NULL,
> + .input_port_id = ODPP_NONE,
> + .key = &conn->key_node[CT_DIR_FWD].key,
> + };
> + ct_offload_op_batch_add(&batch, CT_OFFLOAD_OP_UPD,
> + &offload_ctx);
Is there documentation that whatever we add to the batch
should stay valid until the batch operation finishes? This
goes for netdev_in, conn, and key. The ctx is copied by
value but the pointers inside it are not referenced. We
should add something to the batch operation section in the
.h file.
> + }
> }
> -
> count++;
> }
>
> + /* Submit the batch to ask providers whether each offloaded connection
> + * is still active in hardware. The batch holds raw conn pointers;
> + * connections remain in the RCU-protected list for the duration of
> + * this sweep, so they are guaranteed live here. */
> + ct_offload_op_batch_submit(&batch);
> +
> + CT_OFFLOAD_BATCH_OP_FOR_EACH (idx, op, &batch) {
> + struct conn *c = op->ctx.conn;
> +
> + if (op->error) {
> + /* Provider reports the connection is no longer active. */
> + conn_clean(ct, c);
> + cleaned++;
> + } else {
> + /* Extend expiration by one sweep interval 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);
> + }
The provider's last-used timestamp is collapsed to a
boolean (non-zero = keep alive) and then discarded. If
hardware returns a timestamp older than the connection's
timeout, the connection should be expired, but here we
blindly extend it by one sweep interval. Should the
actual timestamp be compared against the timeout policy
to decide whether to extend or clean?
> + }
> + }
> +
> + ct_offload_op_batch_destroy(&batch);
> +
> if (cleaned_count) {
> *cleaned_count = cleaned;
> }
> --
> 2.51.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
