[1/2] net/mana: use a MR variable on the stack instead of allocating it
Checks
Commit Message
From: Long Li <longli@microsoft.com>
The content of the MR is copied to the cache trees, it's not necessary to
allocate a MR to do this. Use a variable on the stack instead.
This also fixes the memory leak in the code where a MR is allocated but
never freed.
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/net/mana/mr.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
Comments
On 1/30/2024 1:24 AM, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
>
> The content of the MR is copied to the cache trees, it's not necessary to
> allocate a MR to do this. Use a variable on the stack instead.
>
> This also fixes the memory leak in the code where a MR is allocated but
> never freed.
>
patch title describes what is done, but not gives information about
reasoning and impact.
Is this a performance improvement (if so how much), or is this a fix for
the memory leak (if so we need fixes tag for backport), or just a
refactoring?
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
> drivers/net/mana/mr.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/mana/mr.c b/drivers/net/mana/mr.c
> index d6a5ad1460..c9d0f7ef5a 100644
> --- a/drivers/net/mana/mr.c
> +++ b/drivers/net/mana/mr.c
> @@ -40,7 +40,7 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct mana_priv *priv,
> struct ibv_mr *ibv_mr;
> struct mana_range ranges[pool->nb_mem_chunks];
> uint32_t i;
> - struct mana_mr_cache *mr;
> + struct mana_mr_cache mr;
> int ret;
>
> rte_mempool_mem_iter(pool, mana_mempool_chunk_cb, ranges);
> @@ -75,14 +75,13 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct mana_priv *priv,
> DP_LOG(DEBUG, "MR lkey %u addr %p len %zu",
> ibv_mr->lkey, ibv_mr->addr, ibv_mr->length);
>
> - mr = rte_calloc("MANA MR", 1, sizeof(*mr), 0);
> - mr->lkey = ibv_mr->lkey;
> - mr->addr = (uintptr_t)ibv_mr->addr;
> - mr->len = ibv_mr->length;
> - mr->verb_obj = ibv_mr;
> + mr.lkey = ibv_mr->lkey;
> + mr.addr = (uintptr_t)ibv_mr->addr;
> + mr.len = ibv_mr->length;
> + mr.verb_obj = ibv_mr;
>
> rte_spinlock_lock(&priv->mr_btree_lock);
> - ret = mana_mr_btree_insert(&priv->mr_btree, mr);
> + ret = mana_mr_btree_insert(&priv->mr_btree, &mr);
> rte_spinlock_unlock(&priv->mr_btree_lock);
> if (ret) {
> ibv_dereg_mr(ibv_mr);
> @@ -90,7 +89,7 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct mana_priv *priv,
> return ret;
> }
>
> - ret = mana_mr_btree_insert(local_tree, mr);
> + ret = mana_mr_btree_insert(local_tree, &mr);
> if (ret) {
> /* Don't need to clean up MR as it's already
> * in the global tree
> > From: Long Li <longli@microsoft.com>
> >
> > The content of the MR is copied to the cache trees, it's not necessary
> > to allocate a MR to do this. Use a variable on the stack instead.
> >
> > This also fixes the memory leak in the code where a MR is allocated
> > but never freed.
> >
>
> patch title describes what is done, but not gives information about reasoning and
> impact.
>
> Is this a performance improvement (if so how much), or is this a fix for the
> memory leak (if so we need fixes tag for backport), or just a refactoring?
It's for fixing memory leak. I'll send v2 for better wording.
>
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> > drivers/net/mana/mr.c | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/mana/mr.c b/drivers/net/mana/mr.c index
> > d6a5ad1460..c9d0f7ef5a 100644
> > --- a/drivers/net/mana/mr.c
> > +++ b/drivers/net/mana/mr.c
> > @@ -40,7 +40,7 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree,
> struct mana_priv *priv,
> > struct ibv_mr *ibv_mr;
> > struct mana_range ranges[pool->nb_mem_chunks];
> > uint32_t i;
> > - struct mana_mr_cache *mr;
> > + struct mana_mr_cache mr;
> > int ret;
> >
> > rte_mempool_mem_iter(pool, mana_mempool_chunk_cb, ranges); @@
> -75,14
> > +75,13 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct
> mana_priv *priv,
> > DP_LOG(DEBUG, "MR lkey %u addr %p len %zu",
> > ibv_mr->lkey, ibv_mr->addr, ibv_mr->length);
> >
> > - mr = rte_calloc("MANA MR", 1, sizeof(*mr), 0);
> > - mr->lkey = ibv_mr->lkey;
> > - mr->addr = (uintptr_t)ibv_mr->addr;
> > - mr->len = ibv_mr->length;
> > - mr->verb_obj = ibv_mr;
> > + mr.lkey = ibv_mr->lkey;
> > + mr.addr = (uintptr_t)ibv_mr->addr;
> > + mr.len = ibv_mr->length;
> > + mr.verb_obj = ibv_mr;
> >
> > rte_spinlock_lock(&priv->mr_btree_lock);
> > - ret = mana_mr_btree_insert(&priv->mr_btree, mr);
> > + ret = mana_mr_btree_insert(&priv->mr_btree, &mr);
> > rte_spinlock_unlock(&priv->mr_btree_lock);
> > if (ret) {
> > ibv_dereg_mr(ibv_mr);
> > @@ -90,7 +89,7 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree,
> struct mana_priv *priv,
> > return ret;
> > }
> >
> > - ret = mana_mr_btree_insert(local_tree, mr);
> > + ret = mana_mr_btree_insert(local_tree, &mr);
> > if (ret) {
> > /* Don't need to clean up MR as it's already
> > * in the global tree
@@ -40,7 +40,7 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct mana_priv *priv,
struct ibv_mr *ibv_mr;
struct mana_range ranges[pool->nb_mem_chunks];
uint32_t i;
- struct mana_mr_cache *mr;
+ struct mana_mr_cache mr;
int ret;
rte_mempool_mem_iter(pool, mana_mempool_chunk_cb, ranges);
@@ -75,14 +75,13 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct mana_priv *priv,
DP_LOG(DEBUG, "MR lkey %u addr %p len %zu",
ibv_mr->lkey, ibv_mr->addr, ibv_mr->length);
- mr = rte_calloc("MANA MR", 1, sizeof(*mr), 0);
- mr->lkey = ibv_mr->lkey;
- mr->addr = (uintptr_t)ibv_mr->addr;
- mr->len = ibv_mr->length;
- mr->verb_obj = ibv_mr;
+ mr.lkey = ibv_mr->lkey;
+ mr.addr = (uintptr_t)ibv_mr->addr;
+ mr.len = ibv_mr->length;
+ mr.verb_obj = ibv_mr;
rte_spinlock_lock(&priv->mr_btree_lock);
- ret = mana_mr_btree_insert(&priv->mr_btree, mr);
+ ret = mana_mr_btree_insert(&priv->mr_btree, &mr);
rte_spinlock_unlock(&priv->mr_btree_lock);
if (ret) {
ibv_dereg_mr(ibv_mr);
@@ -90,7 +89,7 @@ mana_new_pmd_mr(struct mana_mr_btree *local_tree, struct mana_priv *priv,
return ret;
}
- ret = mana_mr_btree_insert(local_tree, mr);
+ ret = mana_mr_btree_insert(local_tree, &mr);
if (ret) {
/* Don't need to clean up MR as it's already
* in the global tree