Re: [PATCH 1/1] RDMA/odp: convert to use HMM for ODP

[Jason Gunthorpe]

On Tue, Feb 12, 2019 at 11:11:24AM -0500, Jerome Glisse wrote:
> This is what serialize programming the hw and any concurrent CPU page
> table invalidation. This is also one of the thing i want to improve
> long term as mlx5_ib_update_xlt() can do memory allocation and i would
> like to avoid that ie make mlx5_ib_update_xlt() and its sub-functions
> as small and to the points as possible so that they could only fail if
> the hardware is in bad state not because of memory allocation issues.

How can the translation table memory consumption be dynamic (ie use
tables sized huge pages until the OS breaks into 4k pages) if the
tables are pre-allocated?
> > 
> > > +
> > > +static uint64_t odp_hmm_flags[HMM_PFN_FLAG_MAX] = {
> > > +	ODP_READ_BIT,	/* HMM_PFN_VALID */
> > > +	ODP_WRITE_BIT,	/* HMM_PFN_WRITE */
> > > +	ODP_DEVICE_BIT,	/* HMM_PFN_DEVICE_PRIVATE */
> > It seems that the mlx5_ib code in this patch currently ignores the 
> > ODP_DEVICE_BIT (e.g., in umem_dma_to_mtt). Is that okay? Or is it 
> > handled implicitly by the HMM_PFN_SPECIAL case?
> 
> This is because HMM except a bit for device memory as same API is
> use for GPU which have device memory. I can add a comment explaining
> that it is not use for ODP but there just to comply with HMM API.
> 
> > 
> > > @@ -327,9 +287,10 @@ void put_per_mm(struct ib_umem_odp *umem_odp)
> > >  	up_write(&per_mm->umem_rwsem);
> > >  
> > >  	WARN_ON(!RB_EMPTY_ROOT(&per_mm->umem_tree.rb_root));
> > > -	mmu_notifier_unregister_no_release(&per_mm->mn, per_mm->mm);
> > > +	hmm_mirror_unregister(&per_mm->mirror);
> > >  	put_pid(per_mm->tgid);
> > > -	mmu_notifier_call_srcu(&per_mm->rcu, free_per_mm);
> > > +
> > > +	kfree(per_mm);
> > >  }
> > Previously the per_mm struct was released through call srcu, but now it 
> > is released immediately. Is it safe? I saw that hmm_mirror_unregister 
> > calls mmu_notifier_unregister_no_release, so I don't understand what 
> > prevents concurrently running invalidations from accessing the released 
> > per_mm struct.
> 
> Yes it is safe, the hmm struct has its own refcount and mirror holds a
> reference on it, the mm struct itself has a reference on the mm
> struct.

The issue here is that that hmm_mirror_unregister() must be a strong
fence that guarentees no callback is running or will run after
return. mmu_notifier_unregister did not provide that.

I think I saw locking in hmm that was doing this..

Jason