Re: [PATCH rdma-next 2/8] RDMA/mlx5: Generalize mlx5_cache_cache_mr() to fit all cacheable mkeys

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Sep 08, 2022 at 11:54:15PM +0300, Michael Guralnik wrote:
> From: Aharon Landau <aharonl@nvidia.com>
> 
> All the mkeys that can use UMR, can be cached.
> Instead of using reg_create() for mkeys that are not available in the
> cache, generalize mlx5_cache_cache_mr() to fit all cacheable mkeys.
> When a specific mkey request can not be satisfied by the available cache
> mkeys, synchronously create an mkey with the requested properties.

This doesn't seem right to me.

The point of the cache mkeys is this:

> -	if (!ent || ent->limit == 0 ||
> -	    !mlx5r_umr_can_reconfig(dev, 0, access_flags)) {
                                        ^^^^^

Cached mkeys are created with 0 access_flags, so any access_flags
asking for a change away that requires UMR cannot be in the cache.

> -		mutex_lock(&dev->slow_path_mutex);
> -		mr = reg_create(pd, umem, iova, access_flags, page_size, false);
> -		mutex_unlock(&dev->slow_path_mutex);
> -		return mr;
> -	}
>  
> -	mr = mlx5_mr_cache_alloc(dev, ent, access_flags);
> +	ndescs = ib_umem_num_dma_blocks(umem, page_size);
> +
> +	mr = mlx5_mr_cache_alloc(dev, MLX5_MKC_ACCESS_MODE_MTT,
> +				 access_flags, ndescs);

So passing non-zero access_flags to a cache function that has no way
to record the access_flags in the cache is quite illogical.

Before any of this can be reworked the cache infrastructure has to be
fixed to record the exact details of the Mkeys it is holding,
specifically access flags.

But even if that is done, I don't think it makes sense to eliminate
the reg_create. Arguably once all mkeys are cachable we should
eliminate the mr_cache_alloc in the synchronous path instead.

Simply call reg_create always and on destruction put the mkey into the
proper place based on its size and access_flags.

Jason