On Wed, Jan 31, 2024 at 04:35:17PM +0200, Michael Guralnik wrote: > > On 31/01/2024 16:18, Jason Gunthorpe wrote: > > On Wed, Jan 31, 2024 at 02:50:03PM +0200, Michael Guralnik wrote: > > > On 29/01/2024 19:52, Jason Gunthorpe wrote: > > > > On Sun, Jan 28, 2024 at 11:29:15AM +0200, Leon Romanovsky wrote: > > > > > From: Or Har-Toov <ohartoov@xxxxxxxxxx> > > > > > > > > > > In the dereg flow, UMEM is not a good enough indication whether an MR > > > > > is from userspace since in mlx5_ib_rereg_user_mr there are some cases > > > > > when a new MR is created and the UMEM of the old MR is set to NULL. > > > > Why is this a problem though? The only thing the umem has to do is to > > > > trigger the UMR optimization. If UMR is not triggered then the mkey is > > > > destroyed and it shouldn't be part of the cache at all. > > > The problem is that it doesn't trigger the UMR on mkeys that are dereged > > > from the rereg flow. > > > Optimally, we'd want them to return to the cache, if possible. > > Right, so you suggest changing the umem and umr_can_load into > > is_cacheable_mkey() and carefully ensuring the rb_key.ndescs is > > zero for non-umrable? > > Yes. The code is already written trying to ensure this and we've rephrased > a comment in the previous patch to describe this more accurately. But then I wonder why does cache_ent become NULL but the rb_key.ndesc is set? That seems pretty confusing. > > > We can keep relying on the UMEM to decide whether we want to try to return > > > them to cache, as you suggested in the revoke_mr() below, but that way those > > > mkeys will not return to the cache and we have to deal with the in_use in > > > the revoke flow. > > I don't know what this in_use means? in_use should be only an issue if > > the cache_ent is set? Are we really having in_use be set and cache_ent > > bet NULL? That seems like a different bug that should be fixed by > > keeping cache_ent and in_use consistent. > > in_use should be handled only if mkey has a cache_ent. > > I take back what I wrote previously, in_use should be handled in revoke_mr > no matter how we choose to implement this, since we're not guaranteed to > succeed in UMR and might end up dereging mkeys from the cache. That makes the most sense, yes. Jason
