[dpdk-stable] patch 'common/mlx5: fix memory region leak' has been queued to stable release 20.11.3
luca.boccassi at gmail.com
luca.boccassi at gmail.com
Mon Jul 12 15:05:28 CEST 2021
Hi,
FYI, your patch has been queued to stable release 20.11.3
Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 07/14/21. So please
shout if anyone has objections.
Also note that after the patch there's a diff of the upstream commit vs the
patch applied to the branch. This will indicate if there was any rebasing
needed to apply to the stable branch. If there were code changes for rebasing
(ie: not only metadata diffs), please double check that the rebase was
correctly done.
Queued patches are on a temporary branch at:
https://github.com/bluca/dpdk-stable
This queued commit can be viewed at:
https://github.com/bluca/dpdk-stable/commit/d7ecda57fbbd8a715be4bcac9bdefacb7a12db3a
Thanks.
Luca Boccassi
---
>From d7ecda57fbbd8a715be4bcac9bdefacb7a12db3a Mon Sep 17 00:00:00 2001
From: Michael Baum <michaelba at nvidia.com>
Date: Mon, 28 Jun 2021 18:06:14 +0300
Subject: [PATCH] common/mlx5: fix memory region leak
[ upstream commit 2f6c2adbe550ea95a0f73c4f9a9cc5da890b9bf2 ]
All the mlx5 drivers using MRs for data-path must unregister the mapped
memory when it is freed by the dpdk process.
Currently, only the net/eth driver unregisters MRs in free event.
Move the net callback handler from net driver to common.
Signed-off-by: Michael Baum <michaelba at nvidia.com>
Acked-by: Matan Azrad <matan at nvidia.com>
---
drivers/common/mlx5/mlx5_common_mr.c | 89 ++++++++++++++++++++++++++
drivers/common/mlx5/mlx5_common_mr.h | 3 +
drivers/common/mlx5/version.map | 1 +
drivers/net/mlx5/mlx5_mr.c | 95 +---------------------------
4 files changed, 95 insertions(+), 93 deletions(-)
diff --git a/drivers/common/mlx5/mlx5_common_mr.c b/drivers/common/mlx5/mlx5_common_mr.c
index 7c25541dc4..d01f86837d 100644
--- a/drivers/common/mlx5/mlx5_common_mr.c
+++ b/drivers/common/mlx5/mlx5_common_mr.c
@@ -1060,6 +1060,95 @@ mlx5_create_mr_ext(void *pd, uintptr_t addr, size_t len, int socket_id,
return mr;
}
+/**
+ * Callback for memory free event. Iterate freed memsegs and check whether it
+ * belongs to an existing MR. If found, clear the bit from bitmap of MR. As a
+ * result, the MR would be fragmented. If it becomes empty, the MR will be freed
+ * later by mlx5_mr_garbage_collect(). Even if this callback is called from a
+ * secondary process, the garbage collector will be called in primary process
+ * as the secondary process can't call mlx5_mr_create().
+ *
+ * The global cache must be rebuilt if there's any change and this event has to
+ * be propagated to dataplane threads to flush the local caches.
+ *
+ * @param share_cache
+ * Pointer to a global shared MR cache.
+ * @param ibdev_name
+ * Name of ibv device.
+ * @param addr
+ * Address of freed memory.
+ * @param len
+ * Size of freed memory.
+ */
+void
+mlx5_free_mr_by_addr(struct mlx5_mr_share_cache *share_cache,
+ const char *ibdev_name, const void *addr, size_t len)
+{
+ const struct rte_memseg_list *msl;
+ struct mlx5_mr *mr;
+ int ms_n;
+ int i;
+ int rebuild = 0;
+
+ DRV_LOG(DEBUG, "device %s free callback: addr=%p, len=%zu",
+ ibdev_name, addr, len);
+ msl = rte_mem_virt2memseg_list(addr);
+ /* addr and len must be page-aligned. */
+ MLX5_ASSERT((uintptr_t)addr ==
+ RTE_ALIGN((uintptr_t)addr, msl->page_sz));
+ MLX5_ASSERT(len == RTE_ALIGN(len, msl->page_sz));
+ ms_n = len / msl->page_sz;
+ rte_rwlock_write_lock(&share_cache->rwlock);
+ /* Clear bits of freed memsegs from MR. */
+ for (i = 0; i < ms_n; ++i) {
+ const struct rte_memseg *ms;
+ struct mr_cache_entry entry;
+ uintptr_t start;
+ int ms_idx;
+ uint32_t pos;
+
+ /* Find MR having this memseg. */
+ start = (uintptr_t)addr + i * msl->page_sz;
+ mr = mlx5_mr_lookup_list(share_cache, &entry, start);
+ if (mr == NULL)
+ continue;
+ MLX5_ASSERT(mr->msl); /* Can't be external memory. */
+ ms = rte_mem_virt2memseg((void *)start, msl);
+ MLX5_ASSERT(ms != NULL);
+ MLX5_ASSERT(msl->page_sz == ms->hugepage_sz);
+ ms_idx = rte_fbarray_find_idx(&msl->memseg_arr, ms);
+ pos = ms_idx - mr->ms_base_idx;
+ MLX5_ASSERT(rte_bitmap_get(mr->ms_bmp, pos));
+ MLX5_ASSERT(pos < mr->ms_bmp_n);
+ DRV_LOG(DEBUG, "device %s MR(%p): clear bitmap[%u] for addr %p",
+ ibdev_name, (void *)mr, pos, (void *)start);
+ rte_bitmap_clear(mr->ms_bmp, pos);
+ if (--mr->ms_n == 0) {
+ LIST_REMOVE(mr, mr);
+ LIST_INSERT_HEAD(&share_cache->mr_free_list, mr, mr);
+ DRV_LOG(DEBUG, "device %s remove MR(%p) from list",
+ ibdev_name, (void *)mr);
+ }
+ /*
+ * MR is fragmented or will be freed. the global cache must be
+ * rebuilt.
+ */
+ rebuild = 1;
+ }
+ if (rebuild) {
+ mlx5_mr_rebuild_cache(share_cache);
+ /*
+ * No explicit wmb is needed after updating dev_gen due to
+ * store-release ordering in unlock that provides the
+ * implicit barrier at the software visible level.
+ */
+ ++share_cache->dev_gen;
+ DRV_LOG(DEBUG, "broadcasting local cache flush, gen=%d",
+ share_cache->dev_gen);
+ }
+ rte_rwlock_write_unlock(&share_cache->rwlock);
+}
+
/**
* Dump all the created MRs and the global cache entries.
*
diff --git a/drivers/common/mlx5/mlx5_common_mr.h b/drivers/common/mlx5/mlx5_common_mr.h
index da0a0f0c79..09d39ddb5b 100644
--- a/drivers/common/mlx5/mlx5_common_mr.h
+++ b/drivers/common/mlx5/mlx5_common_mr.h
@@ -143,6 +143,9 @@ void mlx5_mr_rebuild_cache(struct mlx5_mr_share_cache *share_cache);
__rte_internal
void mlx5_mr_flush_local_cache(struct mlx5_mr_ctrl *mr_ctrl);
__rte_internal
+void mlx5_free_mr_by_addr(struct mlx5_mr_share_cache *share_cache,
+ const char *ibdev_name, const void *addr, size_t len);
+__rte_internal
int
mlx5_mr_insert_cache(struct mlx5_mr_share_cache *share_cache,
struct mlx5_mr *mr);
diff --git a/drivers/common/mlx5/version.map b/drivers/common/mlx5/version.map
index fd6019bd2b..02aedbc431 100644
--- a/drivers/common/mlx5/version.map
+++ b/drivers/common/mlx5/version.map
@@ -69,6 +69,7 @@ INTERNAL {
mlx5_mr_create_primary;
mlx5_mr_flush_local_cache;
mlx5_mr_free;
+ mlx5_free_mr_by_addr;
mlx5_nl_allmulti;
mlx5_nl_devlink_family_id_get;
diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c
index da4e91fc24..2cce3b9fe8 100644
--- a/drivers/net/mlx5/mlx5_mr.c
+++ b/drivers/net/mlx5/mlx5_mr.c
@@ -29,98 +29,6 @@ struct mr_update_mp_data {
int ret;
};
-/**
- * Callback for memory free event. Iterate freed memsegs and check whether it
- * belongs to an existing MR. If found, clear the bit from bitmap of MR. As a
- * result, the MR would be fragmented. If it becomes empty, the MR will be freed
- * later by mlx5_mr_garbage_collect(). Even if this callback is called from a
- * secondary process, the garbage collector will be called in primary process
- * as the secondary process can't call mlx5_mr_create().
- *
- * The global cache must be rebuilt if there's any change and this event has to
- * be propagated to dataplane threads to flush the local caches.
- *
- * @param sh
- * Pointer to the Ethernet device shared context.
- * @param addr
- * Address of freed memory.
- * @param len
- * Size of freed memory.
- */
-static void
-mlx5_mr_mem_event_free_cb(struct mlx5_dev_ctx_shared *sh,
- const void *addr, size_t len)
-{
- const struct rte_memseg_list *msl;
- struct mlx5_mr *mr;
- int ms_n;
- int i;
- int rebuild = 0;
-
- DEBUG("device %s free callback: addr=%p, len=%zu",
- sh->ibdev_name, addr, len);
- msl = rte_mem_virt2memseg_list(addr);
- /* addr and len must be page-aligned. */
- MLX5_ASSERT((uintptr_t)addr ==
- RTE_ALIGN((uintptr_t)addr, msl->page_sz));
- MLX5_ASSERT(len == RTE_ALIGN(len, msl->page_sz));
- ms_n = len / msl->page_sz;
- rte_rwlock_write_lock(&sh->share_cache.rwlock);
- /* Clear bits of freed memsegs from MR. */
- for (i = 0; i < ms_n; ++i) {
- const struct rte_memseg *ms;
- struct mr_cache_entry entry;
- uintptr_t start;
- int ms_idx;
- uint32_t pos;
-
- /* Find MR having this memseg. */
- start = (uintptr_t)addr + i * msl->page_sz;
- mr = mlx5_mr_lookup_list(&sh->share_cache, &entry, start);
- if (mr == NULL)
- continue;
- MLX5_ASSERT(mr->msl); /* Can't be external memory. */
- ms = rte_mem_virt2memseg((void *)start, msl);
- MLX5_ASSERT(ms != NULL);
- MLX5_ASSERT(msl->page_sz == ms->hugepage_sz);
- ms_idx = rte_fbarray_find_idx(&msl->memseg_arr, ms);
- pos = ms_idx - mr->ms_base_idx;
- MLX5_ASSERT(rte_bitmap_get(mr->ms_bmp, pos));
- MLX5_ASSERT(pos < mr->ms_bmp_n);
- DEBUG("device %s MR(%p): clear bitmap[%u] for addr %p",
- sh->ibdev_name, (void *)mr, pos, (void *)start);
- rte_bitmap_clear(mr->ms_bmp, pos);
- if (--mr->ms_n == 0) {
- LIST_REMOVE(mr, mr);
- LIST_INSERT_HEAD(&sh->share_cache.mr_free_list, mr, mr);
- DEBUG("device %s remove MR(%p) from list",
- sh->ibdev_name, (void *)mr);
- }
- /*
- * MR is fragmented or will be freed. the global cache must be
- * rebuilt.
- */
- rebuild = 1;
- }
- if (rebuild) {
- mlx5_mr_rebuild_cache(&sh->share_cache);
- /*
- * Flush local caches by propagating invalidation across cores.
- * rte_smp_wmb() is enough to synchronize this event. If one of
- * freed memsegs is seen by other core, that means the memseg
- * has been allocated by allocator, which will come after this
- * free call. Therefore, this store instruction (incrementing
- * generation below) will be guaranteed to be seen by other core
- * before the core sees the newly allocated memory.
- */
- ++sh->share_cache.dev_gen;
- DEBUG("broadcasting local cache flush, gen=%d",
- sh->share_cache.dev_gen);
- rte_smp_wmb();
- }
- rte_rwlock_write_unlock(&sh->share_cache.rwlock);
-}
-
/**
* Callback for memory event. This can be called from both primary and secondary
* process.
@@ -146,7 +54,8 @@ mlx5_mr_mem_event_cb(enum rte_mem_event event_type, const void *addr,
rte_rwlock_write_lock(&mlx5_shared_data->mem_event_rwlock);
/* Iterate all the existing mlx5 devices. */
LIST_FOREACH(sh, dev_list, mem_event_cb)
- mlx5_mr_mem_event_free_cb(sh, addr, len);
+ mlx5_free_mr_by_addr(&sh->share_cache,
+ sh->ibdev_name, addr, len);
rte_rwlock_write_unlock(&mlx5_shared_data->mem_event_rwlock);
break;
case RTE_MEM_EVENT_ALLOC:
--
2.30.2
---
Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- - 2021-07-12 13:41:41.388316195 +0100
+++ 0093-common-mlx5-fix-memory-region-leak.patch 2021-07-12 13:41:36.782128693 +0100
@@ -1 +1 @@
-From 2f6c2adbe550ea95a0f73c4f9a9cc5da890b9bf2 Mon Sep 17 00:00:00 2001
+From d7ecda57fbbd8a715be4bcac9bdefacb7a12db3a Mon Sep 17 00:00:00 2001
@@ -5,0 +6,2 @@
+[ upstream commit 2f6c2adbe550ea95a0f73c4f9a9cc5da890b9bf2 ]
+
@@ -13,2 +14,0 @@
-Cc: stable at dpdk.org
-
@@ -18 +18 @@
- drivers/common/mlx5/mlx5_common_mr.c | 89 +++++++++++++++++++++++++++
+ drivers/common/mlx5/mlx5_common_mr.c | 89 ++++++++++++++++++++++++++
@@ -21,2 +21,2 @@
- drivers/net/mlx5/mlx5_mr.c | 90 +---------------------------
- 4 files changed, 95 insertions(+), 88 deletions(-)
+ drivers/net/mlx5/mlx5_mr.c | 95 +---------------------------
+ 4 files changed, 95 insertions(+), 93 deletions(-)
@@ -25 +25 @@
-index afb5b3d0a7..98fe8698e2 100644
+index 7c25541dc4..d01f86837d 100644
@@ -28 +28 @@
-@@ -1062,6 +1062,95 @@ mlx5_create_mr_ext(void *pd, uintptr_t addr, size_t len, int socket_id,
+@@ -1060,6 +1060,95 @@ mlx5_create_mr_ext(void *pd, uintptr_t addr, size_t len, int socket_id,
@@ -125 +125 @@
-index 5cc3f097c2..6e465a05e9 100644
+index da0a0f0c79..09d39ddb5b 100644
@@ -128 +128 @@
-@@ -144,6 +144,9 @@ void mlx5_mr_rebuild_cache(struct mlx5_mr_share_cache *share_cache);
+@@ -143,6 +143,9 @@ void mlx5_mr_rebuild_cache(struct mlx5_mr_share_cache *share_cache);
@@ -139 +139 @@
-index db4f13f1f7..b8be73a77b 100644
+index fd6019bd2b..02aedbc431 100644
@@ -142,4 +142,4 @@
-@@ -103,6 +103,7 @@ INTERNAL {
- mlx5_mr_insert_cache;
- mlx5_mr_lookup_cache;
- mlx5_mr_lookup_list;
+@@ -69,6 +69,7 @@ INTERNAL {
+ mlx5_mr_create_primary;
+ mlx5_mr_flush_local_cache;
+ mlx5_mr_free;
@@ -147,2 +146,0 @@
- mlx5_mr_rebuild_cache;
- mlx5_mr_release_cache;
@@ -149,0 +148,2 @@
+ mlx5_nl_allmulti;
+ mlx5_nl_devlink_family_id_get;
@@ -151 +151 @@
-index 0c5403e493..0b6cfc8cb9 100644
+index da4e91fc24..2cce3b9fe8 100644
@@ -154 +154 @@
-@@ -31,93 +31,6 @@ struct mr_update_mp_data {
+@@ -29,98 +29,6 @@ struct mr_update_mp_data {
@@ -186 +186 @@
-- DRV_LOG(DEBUG, "device %s free callback: addr=%p, len=%zu",
+- DEBUG("device %s free callback: addr=%p, len=%zu",
@@ -216 +216 @@
-- DRV_LOG(DEBUG, "device %s MR(%p): clear bitmap[%u] for addr %p",
+- DEBUG("device %s MR(%p): clear bitmap[%u] for addr %p",
@@ -222 +222 @@
-- DRV_LOG(DEBUG, "device %s remove MR(%p) from list",
+- DEBUG("device %s remove MR(%p) from list",
@@ -234,3 +234,7 @@
-- * No explicit wmb is needed after updating dev_gen due to
-- * store-release ordering in unlock that provides the
-- * implicit barrier at the software visible level.
+- * Flush local caches by propagating invalidation across cores.
+- * rte_smp_wmb() is enough to synchronize this event. If one of
+- * freed memsegs is seen by other core, that means the memseg
+- * has been allocated by allocator, which will come after this
+- * free call. Therefore, this store instruction (incrementing
+- * generation below) will be guaranteed to be seen by other core
+- * before the core sees the newly allocated memory.
@@ -239 +243 @@
-- DRV_LOG(DEBUG, "broadcasting local cache flush, gen=%d",
+- DEBUG("broadcasting local cache flush, gen=%d",
@@ -240,0 +245 @@
+- rte_smp_wmb();
@@ -248 +253 @@
-@@ -143,7 +56,8 @@ mlx5_mr_mem_event_cb(enum rte_mem_event event_type, const void *addr,
+@@ -146,7 +54,8 @@ mlx5_mr_mem_event_cb(enum rte_mem_event event_type, const void *addr,
More information about the stable
mailing list