Re: [PATCH rdma-next 5/6] RDMA/mlx5: Change check for cacheable user mkeys

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2024/1/25 20:52, Junxian Huang 写道:
> 
> 
> On 2024/1/25 20:30, Leon Romanovsky wrote:
>> From: Or Har-Toov <ohartoov@nvidia.com>
>>
>> In the dereg flow, UMEM is not a good enough indication whether an MR
>> is from userspace since in mlx5_ib_rereg_user_mr there are some cases
>> when a new MR is created and the UMEM of the old MR is set to NULL.
>> Currently when mlx5_ib_dereg_mr is called on the old MR, UMEM is NULL
>> but cache_ent can be different than NULL. So, the mkey will not be
>> destroyed.
>> Therefore checking if mkey is from user application and cacheable
>> should be done by checking if rb_key or cache_ent exist and all other kind of
>> mkeys should be destroyed.
>>
>> Fixes: dd1b913fb0d0 ("RDMA/mlx5: Cache all user cacheable mkeys on dereg MR flow")
>> Signed-off-by: Or Har-Toov <ohartoov@nvidia.com>
>> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>> ---
>>   drivers/infiniband/hw/mlx5/mr.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
>> index 12bca6ca4760..3c241898e064 100644
>> --- a/drivers/infiniband/hw/mlx5/mr.c
>> +++ b/drivers/infiniband/hw/mlx5/mr.c
>> @@ -1857,6 +1857,11 @@ static int cache_ent_find_and_store(struct mlx5_ib_dev *dev,
>>   	return ret;
>>   }
>>   
>> +static bool is_cacheable_mkey(struct mlx5_ib_mkey mkey)
> 
> I think it's better using a pointer as the parameter instead of the struct itself.

Why do you think that a pointer is better that the struct itself? In 
kernel doc, is there any rule about this?

Thanks.

Zhu Yanjun

> 
> Junxian
> 
>> +{
>> +	return mkey.cache_ent || mkey.rb_key.ndescs;
>> +}
>> +
>>   int mlx5_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
>>   {
>>   	struct mlx5_ib_mr *mr = to_mmr(ibmr);
>> @@ -1901,12 +1906,6 @@ int mlx5_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
>>   		mr->sig = NULL;
>>   	}
>>   
>> -	/* Stop DMA */
>> -	if (mr->umem && mlx5r_umr_can_load_pas(dev, mr->umem->length))
>> -		if (mlx5r_umr_revoke_mr(mr) ||
>> -		    cache_ent_find_and_store(dev, mr))
>> -			mr->mmkey.cache_ent = NULL;
>> -
>>   	if (mr->umem && mr->umem->is_peer) {
>>   		rc = mlx5r_umr_revoke_mr(mr);
>>   		if (rc)
>> @@ -1914,7 +1913,9 @@ int mlx5_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
>>   		ib_umem_stop_invalidation_notifier(mr->umem);
>>   	}
>>   
>> -	if (!mr->mmkey.cache_ent) {
>> +	/* Stop DMA */
>> +	if (!is_cacheable_mkey(mr->mmkey) || mlx5r_umr_revoke_mr(mr) ||
>> +	    cache_ent_find_and_store(dev, mr)) {
>>   		rc = destroy_mkey(to_mdev(mr->ibmr.device), mr);
>>   		if (rc)
>>   			return rc;