Re: [PATCH v4 rdma-next 2/6] RDMA/mlx5: Remove explicit ODP cache entry

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 17, 2023 at 02:08:35AM +0200, Michael Guralnik wrote:
> 
> On 1/17/2023 1:45 AM, Jason Gunthorpe wrote:
> > On Tue, Jan 17, 2023 at 01:24:34AM +0200, Michael Guralnik wrote:
> > > On 1/16/2023 6:59 PM, Jason Gunthorpe wrote:
> > > > On Sun, Jan 15, 2023 at 03:34:50PM +0200, Michael Guralnik wrote:
> > > > > From: Aharon Landau <aharonl@nvidia.com>
> > > > > 
> > > > > Explicit ODP mkey doesn't have unique properties. It shares the same
> > > > > properties as the order 18 cache entry. There is no need to devote a special
> > > > > entry for that.
> > > > IMR is "implicit mr" for implicit ODP, the commit message is wrong
> > > Yes. I'll change to: "IMR MTT mkeys don't have unique properties..."
> > > 
> > > > > @@ -1591,20 +1593,8 @@ void mlx5_odp_init_mkey_cache_entry(struct mlx5_cache_ent *ent)
> > > > >    {
> > > > >    	if (!(ent->dev->odp_caps.general_caps & IB_ODP_SUPPORT_IMPLICIT))
> > > > >    		return;
> > > > > -
> > > > > -	switch (ent->order - 2) {
> > > > > -	case MLX5_IMR_MTT_CACHE_ENTRY:
> > > > > -		ent->ndescs = MLX5_IMR_MTT_ENTRIES;
> > > > > -		ent->access_mode = MLX5_MKC_ACCESS_MODE_MTT;
> > > > > -		ent->limit = 0;
> > > > > -		break;
> > > > > -
> > > > > -	case MLX5_IMR_KSM_CACHE_ENTRY:
> > > > > -		ent->ndescs = mlx5_imr_ksm_entries;
> > > > > -		ent->access_mode = MLX5_MKC_ACCESS_MODE_KSM;
> > > > > -		ent->limit = 0;
> > > > > -		break;
> > > > > -	}
> > > > > +	ent->ndescs = mlx5_imr_ksm_entries;
> > > > > +	ent->access_mode = MLX5_MKC_ACCESS_MODE_KSM;
> > > > And you didn't answer my question, is this URMable?
> > > Yes, we can UMR between access modes.
> > > > Because I don't quite understand how this can work at this point, for
> > > > lower orders the access_mode is assumed to be MTT, a KLM cannot be put
> > > > in a low order entry at this point.
> > > In our current code, the only non-MTT mkeys using the cache are the IMR KSM
> > > that this patch doesn't change.
> > It does change it, the isolation between the special IMR and the
> > normal MTT order is removed right here.
> > 
> > Now it is broken
> 
> How do IMR MTT mkeys sharing a cache entry with other MTT mkeys break
> anything?

Oh, I read it wrong, this is still keeping the high order
MLX5_IMR_KSM_CACHE_ENTRY

> > > > Ideally you'd teach UMR to switch between MTT/KSM and then the cache
> > > > is fine, size the amount of space required based on the number of
> > > > bytes in the memory.
> > > Agreed, access_mode and ndescs can be dropped from the rb_key that this
> > > series introduces and instead we'll add the size of the descriptors as a
> > > cache entry property.
> > > Doing this will reduce number of entries in the RB tree but will add
> > > complexity to the dereg and rereg flows .
> > Not really, you just always set the access mode in the UMR like
> > everything else.
> > 
> > Jason
> 
> ok, I'll give this a second look. if it's really only this, I can probably
> push this quickly.
> BTW, this will mean that IMR KSM mkeys will also share an entry with other
> MTT mkeys

That would be perfect, you should definately do it

But it seems there is not an issue here, so a followup is OK

Jason