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@xxxxxxxxxx>
> ---
> 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
