Re: [PATCH v2 rdma-next 4/6] RDMA/mlx5: Introduce mlx5r_cache_rb_key

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Dec 07, 2022 at 10:57:50AM +0200, Michael Guralnik wrote:
> Switch from using the mkey order to using the new struct as the key to
> the RB tree of cache entries.
> The key is all the mkey properties that UMR operations can't modify.
> Using this key to define the cache entries and to search and create
> cache mkeys.
> 
> Signed-off-by: Michael Guralnik <michaelgur@nvidia.com>
> ---
>  drivers/infiniband/hw/mlx5/mlx5_ib.h |  32 +++--
>  drivers/infiniband/hw/mlx5/mr.c      | 196 +++++++++++++++++++--------
>  drivers/infiniband/hw/mlx5/odp.c     |  26 ++--
>  3 files changed, 180 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 10e22fb01e1b..d795e9fc2c2f 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -731,17 +731,26 @@ struct umr_common {
>  	unsigned int state;
>  };
>  
> +struct mlx5r_cache_rb_key {
> +	u8 ats:1;
> +	unsigned int access_mode;
> +	unsigned int access_flags;
> +	/*
> +	 * keep ndescs as the last member so entries with about the same ndescs
> +	 * will be close in the tree
> +	 */

? How does this happen? The compare function doesn't use memcmp..

I think this comment should go in the compare function because the
search function does this:

> -	return smallest;
> +	return (smallest &&
> +		smallest->rb_key.access_mode == rb_key.access_mode &&
> +		smallest->rb_key.access_flags == rb_key.access_flags &&
> +		smallest->rb_key.ats == rb_key.ats) ?
> +		       smallest :
> +		       NULL;

So it isn't that they have to be close in the tree, it is that
"smallest" has to find a matching mode/flags/ats before finding the
smallest ndescs of the matching list. Thus ndescs must always by the
last thing in the compare ladder.

> +struct mlx5_ib_mr *mlx5_mr_cache_alloc(struct mlx5_ib_dev *dev,
> +				       int access_flags, int access_mode,
> +				       int ndescs)
> +{
> +	struct mlx5r_cache_rb_key rb_key = {
> +		.ndescs = ndescs,
> +		.access_mode = access_mode,
> +		.access_flags = get_unchangeable_access_flags(dev, access_flags)
> +	};
> +	struct mlx5_cache_ent *ent = mkey_cache_ent_from_rb_key(dev, rb_key);
> +	if (!ent)

Missing newline

>  struct mlx5_cache_ent *mlx5r_cache_create_ent(struct mlx5_ib_dev *dev,
> -					      int order)
> +					      struct mlx5r_cache_rb_key rb_key,
> +					      bool debugfs)
>  {
>  	struct mlx5_cache_ent *ent;
>  	int ret;
> @@ -808,7 +873,10 @@ struct mlx5_cache_ent *mlx5r_cache_create_ent(struct mlx5_ib_dev *dev,
>  		return ERR_PTR(-ENOMEM);
>  
>  	xa_init_flags(&ent->mkeys, XA_FLAGS_LOCK_IRQ);
> -	ent->order = order;
> +	ent->rb_key.ats = rb_key.ats;
> +	ent->rb_key.access_mode = rb_key.access_mode;
> +	ent->rb_key.access_flags = rb_key.access_flags;
> +	ent->rb_key.ndescs = rb_key.ndescs;

ent->rb_key = rb_key

>  int mlx5_mkey_cache_init(struct mlx5_ib_dev *dev)
>  {
> +	struct mlx5r_cache_rb_key rb_key = {
> +		.access_mode = MLX5_MKC_ACCESS_MODE_MTT,
> +	};
>  	struct mlx5_mkey_cache *cache = &dev->cache;
>  	struct mlx5_cache_ent *ent;
>  	int i;
> @@ -838,19 +913,26 @@ int mlx5_mkey_cache_init(struct mlx5_ib_dev *dev)
>  
>  	mlx5_cmd_init_async_ctx(dev->mdev, &dev->async_ctx);
>  	timer_setup(&dev->delay_timer, delay_time_func, 0);
> +	mlx5_mkey_cache_debugfs_init(dev);
>  	for (i = 0; i < MAX_MKEY_CACHE_ENTRIES; i++) {
> -		ent = mlx5r_cache_create_ent(dev, i);
> -
> -		if (i > MKEY_CACHE_LAST_STD_ENTRY) {
> -			mlx5_odp_init_mkey_cache_entry(ent);
> +		if (i > mkey_cache_max_order(dev))
>  			continue;

This is goofy, just make the for loop go from 2 to
mkey_cache_max_order() + 2 (and probably have the function do the + 2
internally)

Get rid of MAX_MKEY_CACHE_ENTRIES
> +
> +		if (i == MLX5_IMR_KSM_CACHE_ENTRY) {
> +			ent = mlx5_odp_init_mkey_cache_entry(dev);
> +			if (!ent)
> +				continue;

This too, just call mlx5_odp_init_mkey_cache_entry() outside the loop

And rename it to something like mlx5_odp_init_mkey_cache(), and don't
return ent.

Set ent->limit inside mlx5r_cache_create_ent()

And run over the whole rbtree in a final loop to do the final
queue_adjust_cache_locked() step.

> -void mlx5_odp_init_mkey_cache_entry(struct mlx5_cache_ent *ent)
> +struct mlx5_cache_ent *mlx5_odp_init_mkey_cache_entry(struct mlx5_ib_dev *dev)
>  {
> -	if (!(ent->dev->odp_caps.general_caps & IB_ODP_SUPPORT_IMPLICIT))
> -		return;
> -	ent->ndescs = mlx5_imr_ksm_entries;
> -	ent->access_mode = MLX5_MKC_ACCESS_MODE_KSM;
> +	struct mlx5r_cache_rb_key rb_key = {
> +		.ats = 0,
> +		.access_mode = MLX5_MKC_ACCESS_MODE_KSM,
> +		.access_flags = 0,
> +		.ndescs = mlx5_imr_ksm_entries,

Don't need to zero init things here

Jason