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.
WDYT?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
