On Tue, 14 Jul 2026 at 01:29, Ilya Maximets <[email protected]> wrote: > > On 7/10/26 9:08 AM, David Marchand wrote: > > Next commit will introduce the possibility to grow dp_packet_batch. > > As some memory will be allocated when growing the batch, split > > dp_packet_batch handling with new helpers for initialising, resetting > > (before reuse) and destroying. > > > > Some special care must be taken to init/destroy xmemdup'd dp_packet_batch > > objects. > > > > Signed-off-by: David Marchand <[email protected]> > > --- > > Changes since v1: > > - renamed dp_packet_batch_reset_metadata as dp_packet_batch_reset, > > > > Changes since RFC: > > - rebased (dropped AVX512), > > - fixed one leak in dp_netdev_pmd_clear_ports, > > > > --- > > lib/dp-packet.h | 15 ++++++++-- > > lib/dpif-netdev.c | 67 +++++++++++++++++++++++++++++++++++++++--- > > lib/dpif.c | 1 + > > lib/netdev-afxdp.c | 5 ++-- > > lib/netdev-bsd.c | 3 +- > > lib/netdev-dummy.c | 3 +- > > lib/netdev-linux.c | 2 +- > > lib/netdev.c | 1 + > > lib/odp-execute.c | 3 ++ > > tests/test-conntrack.c | 6 +++- > > 10 files changed, 94 insertions(+), 12 deletions(-) > > > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h > > index 16b9afbc36..e144cf8d75 100644 > > --- a/lib/dp-packet.h > > +++ b/lib/dp-packet.h > > @@ -866,12 +866,18 @@ struct dp_packet_batch { > > }; > > > > static inline void > > -dp_packet_batch_init(struct dp_packet_batch *batch) > > +dp_packet_batch_reset(struct dp_packet_batch *batch) > > { > > batch->count = 0; > > batch->trunc = false; > > } > > > > +static inline void > > +dp_packet_batch_init(struct dp_packet_batch *batch) > > +{ > > + dp_packet_batch_reset(batch); > > +} > > + > > static inline void > > dp_packet_batch_add__(struct dp_packet_batch *batch, > > struct dp_packet *packet, size_t limit) > > @@ -987,6 +993,11 @@ dp_packet_batch_clone(struct dp_packet_batch *dst, > > dst->trunc = src->trunc; > > } > > > > +static inline void > > +dp_packet_batch_destroy(struct dp_packet_batch *batch OVS_UNUSED) > > +{ > > +} > > + > > static inline void > > dp_packet_delete_batch(struct dp_packet_batch *batch, bool should_steal) > > { > > @@ -996,7 +1007,7 @@ dp_packet_delete_batch(struct dp_packet_batch *batch, > > bool should_steal) > > DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > > dp_packet_delete(packet); > > } > > - dp_packet_batch_init(batch); > > + dp_packet_batch_reset(batch); > > } > > } > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index aa1f6c6f30..7d46d747fd 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -3619,6 +3619,7 @@ dpif_netdev_execute(struct dpif *dpif, struct > > dpif_execute *execute) > > * tunnel push. */ > > dp_packet_delete_batch(&pp, true); > > } > > + dp_packet_batch_destroy(&pp); > > > > return 0; > > } > > @@ -4375,7 +4376,7 @@ dp_netdev_pmd_flush_output_on_port(struct > > dp_netdev_pmd_thread *pmd, > > continue; > > } > > netdev_send(p->port->netdev, i, &p->txq_pkts[i], true); > > - dp_packet_batch_init(&p->txq_pkts[i]); > > + dp_packet_batch_reset(&p->txq_pkts[i]); > > } > > } else { > > if (p->port->txq_mode == TXQ_MODE_XPS) { > > @@ -4387,7 +4388,7 @@ dp_netdev_pmd_flush_output_on_port(struct > > dp_netdev_pmd_thread *pmd, > > } > > netdev_send(p->port->netdev, tx_qid, &p->output_pkts, > > concurrent_txqs); > > } > > - dp_packet_batch_init(&p->output_pkts); > > + dp_packet_batch_reset(&p->output_pkts); > > > > /* Update time of the next flush. */ > > atomic_read_relaxed(&pmd->dp->tx_flush_interval, &tx_flush_interval); > > @@ -4494,6 +4495,8 @@ dp_netdev_process_rxq_port(struct > > dp_netdev_pmd_thread *pmd, > > } > > } > > > > + dp_packet_batch_destroy(&batch); > > + > > pmd->ctx.last_rxq = NULL; > > > > return batch_cnt; > > @@ -5907,11 +5910,27 @@ pmd_free_cached_ports(struct dp_netdev_pmd_thread > > *pmd) > > dpif_netdev_xps_revalidate_pmd(pmd, true); > > > > HMAP_FOR_EACH_POP (tx_port_cached, node, &pmd->tnl_port_cache) { > > + if (tx_port_cached->txq_pkts) { > > + int n_txq = netdev_n_txq(tx_port_cached->port->netdev); > > + > > + for (int i = 0; i < n_txq; i++) { > > + dp_packet_batch_destroy(&tx_port_cached->txq_pkts[i]); > > + } > > + } > > free(tx_port_cached->txq_pkts); > > + dp_packet_batch_destroy(&tx_port_cached->output_pkts); > > free(tx_port_cached); > > } > > HMAP_FOR_EACH_POP (tx_port_cached, node, &pmd->send_port_cache) { > > + if (tx_port_cached->txq_pkts) { > > + int n_txq = netdev_n_txq(tx_port_cached->port->netdev); > > + > > + for (int i = 0; i < n_txq; i++) { > > + dp_packet_batch_destroy(&tx_port_cached->txq_pkts[i]); > > + } > > + } > > free(tx_port_cached->txq_pkts); > > + dp_packet_batch_destroy(&tx_port_cached->output_pkts); > > free(tx_port_cached); > > } > > } > > @@ -5936,10 +5955,14 @@ pmd_load_cached_ports(struct dp_netdev_pmd_thread > > *pmd) > > > > if (netdev_has_tunnel_push_pop(tx_port->port->netdev)) { > > tx_port_cached = xmemdup(tx_port, sizeof *tx_port_cached); > > + dp_packet_batch_init(&tx_port_cached->output_pkts); > > if (tx_port->txq_pkts) { > > txq_pkts_cached = xmemdup(tx_port->txq_pkts, > > n_txq * sizeof > > *tx_port->txq_pkts); > > tx_port_cached->txq_pkts = txq_pkts_cached; > > + for (int i = 0; i < n_txq; i++) { > > + dp_packet_batch_init(&tx_port_cached->txq_pkts[i]); > > + } > > } > > hmap_insert(&pmd->tnl_port_cache, &tx_port_cached->node, > > hash_port_no(tx_port_cached->port->port_no)); > > @@ -5947,10 +5970,14 @@ pmd_load_cached_ports(struct dp_netdev_pmd_thread > > *pmd) > > > > if (n_txq) { > > tx_port_cached = xmemdup(tx_port, sizeof *tx_port_cached); > > + dp_packet_batch_init(&tx_port_cached->output_pkts); > > if (tx_port->txq_pkts) { > > txq_pkts_cached = xmemdup(tx_port->txq_pkts, > > n_txq * sizeof > > *tx_port->txq_pkts); > > tx_port_cached->txq_pkts = txq_pkts_cached; > > + for (int i = 0; i < n_txq; i++) { > > + dp_packet_batch_init(&tx_port_cached->txq_pkts[i]); > > + } > > } > > hmap_insert(&pmd->send_port_cache, &tx_port_cached->node, > > hash_port_no(tx_port_cached->port->port_no)); > > @@ -6795,6 +6822,7 @@ dp_netdev_pmd_clear_ports(struct dp_netdev_pmd_thread > > *pmd) > > } > > HMAP_FOR_EACH_POP (port, node, &pmd->tx_ports) { > > free(port->txq_pkts); > > This doesn't cause any real issues, but we should technically destroy > all the individual batches here as well to be consistent. > > And since that became a problem, there is quite a bit of code duplication > going on that we can avoid and save us from this kind of issues with > missing part of the destruction in the future. > > If we move out the creation/clone/destruction of tx_port into functions, > we could save some space here and in the patch 11. > > We could do something like this (not fully tested, includes the allocation > of the output_pkts_rxqs from patch 11): > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 585bffda9..fc3b1678b 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4503,6 +4503,55 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread > *pmd, > return batch_cnt; > } > > +static struct tx_port * > +tx_port_create(struct dp_netdev_port *port) > +{ > + struct tx_port *tx = xzalloc(sizeof *tx); > + > + tx->port = port; > + tx->qid = -1; > + tx->flush_time = 0LL; > + dp_packet_batch_init(&tx->output_pkts); > + tx->output_pkts_rxqs = > xcalloc(dp_packet_batch_capacity(&tx->output_pkts), > + sizeof *tx->output_pkts_rxqs); > + > + if (tx->port->txq_mode == TXQ_MODE_XPS_HASH) { > + int i, n_txq = netdev_n_txq(tx->port->netdev); > + > + tx->txq_pkts = xzalloc(n_txq * sizeof *tx->txq_pkts); > + for (i = 0; i < n_txq; i++) { > + dp_packet_batch_init(&tx->txq_pkts[i]); > + } > + } > + > + return tx; > +} > + > +static struct tx_port * > +tx_port_clone(const struct tx_port *tx_port) > +{ > + struct tx_port *clone = tx_port_create(tx_port->port); > + > + clone->qid = tx_port->qid; > + return clone; > +} > + > +static void > +tx_port_destroy(struct tx_port *tx_port) > +{ > + if (tx_port->txq_pkts) { > + int n_txq = netdev_n_txq(tx_port->port->netdev); > + > + for (int i = 0; i < n_txq; i++) { > + dp_packet_batch_destroy(&tx_port->txq_pkts[i]); > + } > + } > + free(tx_port->txq_pkts); > + dp_packet_batch_destroy(&tx_port->output_pkts); > + free(tx_port->output_pkts_rxqs); > + free(tx_port); > +} > + > static struct tx_port * > tx_port_lookup(const struct hmap *hmap, odp_port_t port_no) > { > --- > > And use these functions in 4 places that destruct the ports: > pmd_free_cached_ports() x2 > dp_netdev_pmd_clear_ports() > dp_netdev_del_port_tx_from_pmd() > > Clone: > pmd_load_cached_ports() x2 > > Create: > dp_netdev_add_port_tx_to_pmd() > > Note: The clone() above is implemented on top of create(), as we do > not really care about duplicating most of the memory. It is re-set > back to default values right away in the correct code anyways. So, > it seems like we could just allocate the new tx port and only set > two fields that are actually important to copy.
I did not like all those memdups, it was hard to understand why we were copying memory. And I had already missed one location in a previous revision, so this refactoring makes a lot of sense. For next revision then. -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
