On 18 May 2026, at 19:13, Aaron Conole wrote: > Offloading providers will need input port details in order to > correctly map the packet movement. They will also need the output > port mapping for the batch, but that will come in the future.
Hi Aaron, How will we handle a change in the ingress port? This might change which offload provider is actually tracking this flow. There was also earlier discussion that NVIDIA needed to track the egress port. Will this be added later, or will it be the responsibility of the conntrack offload provider? See some more comments below. //Eelco > Signed-off-by: Aaron Conole <[email protected]> > --- > lib/conntrack.c | 10 +++++----- > lib/conntrack.h | 5 ++++- > lib/dpif-netdev.c | 14 +++++++++++++- > tests/test-conntrack.c | 18 +++++++++--------- > 4 files changed, 31 insertions(+), 16 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 83dac97b89..4d302eeb5d 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1322,7 +1322,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, > bool force, bool commit, long long now, const uint32_t *setmark, > const struct ovs_key_ct_labels *setlabel, > const struct nat_action_info_t *nat_action_info, > - const char *helper, uint32_t tp_id) > + const char *helper, uint32_t tp_id, struct netdev *in_netdev) > { > /* Reset ct_state whenever entering a new zone. */ > if (pkt->md.ct_state && pkt->md.ct_zone != zone) { > @@ -1397,7 +1397,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, > if (conn && ct_offload_enabled()) { > struct ct_offload_ctx offload_ctx = { > .conn = conn, > - .netdev_in = NULL, > + .netdev_in = in_netdev, > .input_port_id = pkt->md.in_port.odp_port, > }; > ct_offload_conn_add(&offload_ctx); > @@ -1415,7 +1415,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, > * reverse direction port. */ > struct ct_offload_ctx offload_ctx = { > .conn = conn, > - .netdev_in = NULL, > + .netdev_in = in_netdev, > .input_port_id = pkt->md.in_port.odp_port, > }; > ct_offload_conn_established(&offload_ctx); > @@ -1452,7 +1452,7 @@ conntrack_execute(struct conntrack *ct, struct > dp_packet_batch *pkt_batch, > const struct ovs_key_ct_labels *setlabel, > const char *helper, > const struct nat_action_info_t *nat_action_info, > - long long now, uint32_t tp_id) > + long long now, uint32_t tp_id, struct netdev *in_netdev) > { > odp_port_t in_port = ODPP_LOCAL; > struct conn_lookup_ctx ctx; > @@ -1489,7 +1489,7 @@ conntrack_execute(struct conntrack *ct, struct > dp_packet_batch *pkt_batch, > write_ct_md(packet, zone, NULL, NULL, NULL); > } else { > process_one(ct, packet, &ctx, zone, force, commit, now, setmark, > - setlabel, nat_action_info, helper, tp_id); > + setlabel, nat_action_info, helper, tp_id, in_netdev); > } > } > > diff --git a/lib/conntrack.h b/lib/conntrack.h > index e5ca1528bf..fc4a529e2a 100644 > --- a/lib/conntrack.h > +++ b/lib/conntrack.h > @@ -130,6 +130,8 @@ typedef unsigned int ct_private_id_t; > * this slot (i.e. at module initialization time). */ > ct_private_id_t conn_private_id_alloc(void (*destructor)(void *)); > > +struct netdev; > + > struct conntrack *conntrack_init(void); > void conntrack_destroy(struct conntrack *); > > @@ -139,7 +141,8 @@ int conntrack_execute(struct conntrack *ct, struct > dp_packet_batch *pkt_batch, > const struct ovs_key_ct_labels *setlabel, > const char *helper, > const struct nat_action_info_t *nat_action_info, > - long long now, uint32_t tp_id); > + long long now, uint32_t tp_id, > + struct netdev *in_netdev); > void conntrack_clear(struct dp_packet *packet); > > struct conntrack_dump { > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 49f4fa2ac6..2d05063dcd 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -8636,6 +8636,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch > *packets_, > break; > > case OVS_ACTION_ATTR_CT: { > + struct dp_netdev_port *in_port_p = NULL;; Double ;; > + struct netdev *in_netdev = NULL; > const struct nlattr *b; > bool force = false; > bool commit = false; > @@ -8768,9 +8770,19 @@ dp_execute_cb(void *aux_, struct dp_packet_batch > *packets_, > VLOG_WARN_RL(&rl, "NAT specified without commit."); > } > > + if (!dp_packet_batch_is_empty(packets_)) { > + odp_port_t query_port = > packets_->packets[0]->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. This matters for tunneled packets where in_port may have been rewritten. Eli or Gaetan can confirm. If true the variable names in this patch should be adjusted too. > + in_port_p = dp_netdev_lookup_port(dp, query_port); > + } > + > + if (in_port_p) { > + in_netdev = in_port_p->netdev; No netdev_ref() is taken here. This is safe in the PMD fast path because port deletion waits for PMD quiescence, but if conntrack_execute() is ever called outside the PMD context the netdev could be freed during the call. Should we document that callers of conntrack_execute() should hold a reference to the in_netdev? > + } The second 'if' could be nested inside the first one. The in_port_p definition could move inside the block too. > conntrack_execute(dp->conntrack, packets_, aux->flow->dl_type, force, > commit, zone, setmark, setlabel, helper, > - nat_action_info_ref, pmd->ctx.now / 1000, tp_id); > + nat_action_info_ref, pmd->ctx.now / 1000, tp_id, > + in_netdev); > break; > } > > diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c > index 7f42adbb55..3c409b373b 100644 > --- a/tests/test-conntrack.c > +++ b/tests/test-conntrack.c > @@ -198,7 +198,7 @@ ct_thread_main(void *aux_) > ovs_barrier_block(&barrier); > for (i = 0; i < n_pkts; i += batch_size) { > conntrack_execute(ct, pkt_batch, dl_type, false, true, 0, NULL, NULL, > - NULL, NULL, now, 0); > + NULL, NULL, now, 0, NULL); > DP_PACKET_BATCH_FOR_EACH (j, pkt, pkt_batch) { > pkt_metadata_init_conn(&pkt->md); > } > @@ -311,7 +311,7 @@ test_benchmark_zones(struct ovs_cmdl_context *ctx) > for (i = 0; i < n_zones; i++) { > for (j = 0; j < n_conns; j++) { > conntrack_execute(ct, pkt_batch[j], dl_type, false, true, i, > - NULL, NULL, NULL, NULL, now, 0); > + NULL, NULL, NULL, NULL, now, 0, NULL); > pkt_metadata_init_conn(&pkt_batch[j]->packets[0]->md); > } > } > @@ -334,7 +334,7 @@ test_benchmark_zones(struct ovs_cmdl_context *ctx) > stopwatch_start(STOPWATCH_CT_EXECUTE_COMMIT, time_usec()); > for (j = 0; j < n_conns; j++) { > conntrack_execute(ct, pkt_batch[j], dl_type, false, true, zone, > - NULL, NULL, NULL, NULL, now, 0); > + NULL, NULL, NULL, NULL, now, 0, NULL); > pkt_metadata_init_conn(&pkt_batch[j]->packets[0]->md); > } > stopwatch_stop(STOPWATCH_CT_EXECUTE_COMMIT, time_usec()); > @@ -343,7 +343,7 @@ test_benchmark_zones(struct ovs_cmdl_context *ctx) > stopwatch_start(STOPWATCH_CT_EXECUTE_NO_COMMIT, time_usec()); > for (j = 0; j < n_conns; j++) { > conntrack_execute(ct, pkt_batch[j], dl_type, false, false, zone, > - NULL, NULL, NULL, NULL, now, 0); > + NULL, NULL, NULL, NULL, now, 0, NULL); > pkt_metadata_init_conn(&pkt_batch[j]->packets[0]->md); > } > stopwatch_stop(STOPWATCH_CT_EXECUTE_NO_COMMIT, time_usec()); > @@ -419,7 +419,7 @@ pcap_batch_execute_conntrack(struct conntrack *ct_, > > if (flow.dl_type != dl_type) { > conntrack_execute(ct_, &new_batch, dl_type, false, true, 0, > - NULL, NULL, NULL, NULL, now, 0); > + NULL, NULL, NULL, NULL, now, 0, NULL); > dp_packet_batch_init(&new_batch); > } > dp_packet_batch_add(&new_batch, packet); > @@ -427,7 +427,7 @@ pcap_batch_execute_conntrack(struct conntrack *ct_, > > if (!dp_packet_batch_is_empty(&new_batch)) { > conntrack_execute(ct_, &new_batch, dl_type, false, true, 0, NULL, > NULL, > - NULL, NULL, now, 0); > + NULL, NULL, now, 0, NULL); > } > > } > @@ -540,7 +540,7 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx > OVS_UNUSED) > struct dp_packet_batch syn_batch; > dp_packet_batch_init_packet(&syn_batch, syn); > conntrack_execute(ct, &syn_batch, htons(ETH_TYPE_IP), false, true, 0, > - NULL, NULL, "ftp", &nat_info, now, 0); > + NULL, NULL, "ftp", &nat_info, now, 0, NULL); > dp_packet_delete_batch(&syn_batch, true); > > /* We get to skip some of the processing because the conntrack execute > @@ -563,7 +563,7 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx > OVS_UNUSED) > struct dp_packet_batch port_batch; > dp_packet_batch_init_packet(&port_batch, port_pkt); > conntrack_execute(ct, &port_batch, htons(ETH_TYPE_IP), false, true, 0, > - NULL, NULL, "ftp", &nat_info, now, 0); > + NULL, NULL, "ftp", &nat_info, now, 0, NULL); > > struct tcp_header *th = dp_packet_l4(port_pkt); > size_t tcp_hdr_len = TCP_OFFSET(th->tcp_ctl) * 4; > @@ -660,7 +660,7 @@ test_private_destructor(struct ovs_cmdl_context *ctx > OVS_UNUSED) > > long long now = time_msec(); > conntrack_execute(lct, &batch, dl_type, false, true, 0, > - NULL, NULL, NULL, NULL, now, 0); > + NULL, NULL, NULL, NULL, now, 0, NULL); > > /* After a committed execute the packet carries a cached conn pointer. */ > struct conn *conn = pkt->md.conn; > -- > 2.53.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
