Re: [PATCH rdma-next] RDMA/mlx5: Enable ATS when allocating MRs

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Sep 02, 2024 at 01:37:03PM +0300, Leon Romanovsky wrote:
> From: Maher Sanalla <msanalla@nvidia.com>
> 
> When allocating MRs, it is not definitive whether they will be used for
> peer-to-peer transactions or for other usecases.
> 
> Since peer-to-peer transactions benefit significantly from ATS
> performance-wise, enable ATS on newly-allocated MRs when supported.

?

On the upstream kernel there is no way for a peer-to-peer page to get
into the normal MRs, is there? So why do this?

This also makes all CPU memory run slower.

> Signed-off-by: Maher Sanalla <msanalla@nvidia.com>
> Reviewed-by: Gal Shalom <galshalom@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/mlx5/mr.c | 5 +++++
>  1 file changed, 5 insertions(+)

shoulnd't g
Are yo sur? ATS has a big negative impact if it isn't required..

> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index 3320238a0eb8..7d8c58f803ac 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
> @@ -1080,6 +1080,7 @@ struct ib_mr *mlx5_ib_get_dma_mr(struct ib_pd *pd, int acc)
>  	MLX5_SET(mkc, mkc, length64, 1);
>  	set_mkc_access_pd_addr_fields(mkc, acc | IB_ACCESS_RELAXED_ORDERING, 0,
>  				      pd);
> +	MLX5_SET(mkc, mkc, ma_translation_mode, MLX5_CAP_GEN(dev->mdev, ats));
>  
>  	err = mlx5_ib_create_mkey(dev, &mr->mmkey, in, inlen);
>  	if (err)
> @@ -2218,6 +2219,7 @@ int mlx5_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
>  static void mlx5_set_umr_free_mkey(struct ib_pd *pd, u32 *in, int ndescs,
>  				   int access_mode, int page_shift)
>  {
> +	struct mlx5_ib_dev *dev = to_mdev(pd->device);
>  	void *mkc;
>  
>  	mkc = MLX5_ADDR_OF(create_mkey_in, in, memory_key_mkey_entry);
> @@ -2230,6 +2232,9 @@ static void mlx5_set_umr_free_mkey(struct ib_pd *pd, u32 *in, int ndescs,
>  	MLX5_SET(mkc, mkc, access_mode_4_2, (access_mode >> 2) & 0x7);
>  	MLX5_SET(mkc, mkc, umr_en, 1);
>  	MLX5_SET(mkc, mkc, log_page_size, page_shift);
> +	if (access_mode == MLX5_MKC_ACCESS_MODE_PA ||
> +	    access_mode == MLX5_MKC_ACCESS_MODE_MTT)
> +		MLX5_SET(mkc, mkc, ma_translation_mode, MLX5_CAP_GEN(dev->mdev, ats));
>  }

This doesn't look right, isn't it re-introducing the bug I fixed about
mis-caching the ATS property?

drivers/infiniband/hw/mlx5/mr.c:        MLX5_SET(mkc, mkc, ma_translation_mode, !!ent->rb_key.ats);

?

Or at least shouldn't all the old ATS stuff be ripped out if it is
being forced on for every mkey?

Jasan