Re: [PATCH 1/2] net/mana: use a MR variable on the stack instead of allocating it

[Ferruh Yigit <ferruh.yigit@amd.com>]

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