RE: [EXT] Re: [PATCH v8 rdma-next 2/7] RDMA/core: Create mmap database and cookie helper functions

[Michal Kalderon <mkalderon@marvell.com>]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, August 29, 2019 3:41 PM
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Aug 29, 2019 at 02:35:45PM +0300, Gal Pressman wrote:
> > On 27/08/2019 16:28, Michal Kalderon wrote:
> > > +/**
> > > + * rdma_user_mmap_entry_get() - Get an entry from the mmap_xa.
> > > + *
> > > + * @ucontext: associated user context.
> > > + * @key: the key received from rdma_user_mmap_entry_insert which
> > > + *     is provided by user as the address to map.
> > > + * @len: the length the user wants to map.
> > > + * @vma: the vma related to the current mmap call.
> > > + *
> > > + * This function is called when a user tries to mmap a key it
> > > + * initially received from the driver. The key was created by
> > > + * the function rdma_user_mmap_entry_insert. The function should
> > > + * be called only once per mmap. It initializes the vma and
> > > + * increases the entries ref-count. Once the memory is unmapped
> > > + * the ref-count will decrease. When the refcount reaches zero
> > > + * the entry will be deleted.
> > > + *
> > > + * Return an entry if exists or NULL if there is no match.
> > > + */
> > > +struct rdma_user_mmap_entry *
> > > +rdma_user_mmap_entry_get(struct ib_ucontext *ucontext, u64 key,
> u64 len,
> > > +			 struct vm_area_struct *vma)
> > > +{
> > > +	struct rdma_user_mmap_entry *entry;
> > > +	u64 mmap_page;
> > > +
> > > +	mmap_page = key >> PAGE_SHIFT;
> > > +	if (mmap_page > U32_MAX)
> > > +		return NULL;
> > > +
> > > +	entry = xa_load(&ucontext->mmap_xa, mmap_page);
> > > +	if (!entry)
> > > +		return NULL;
> >
> > I'm probably missing something, what happens if an insertion is done,
> > a get is called and right at this point (before kref_get) the entry is
> > being removed (and freed by the driver)?
> 
> Yes, things are wrong here.. It should hold xa_lock to protect entry until the
> kref is obtained and this must use kref_get_unless_zero() as the kref could
> be 0 while still in the xarray.
> 
> > > +	for (i = 0; i < entry->npages; i++) {
> > > +		xa_erase(&ucontext->mmap_xa, entry->mmap_page + i);
> 
> This is better to use __xa_erase and hold the xa_lock outside the loop
Ok, will fix

> 
> > > +	/* We want the whole allocation to be done without interruption
> > > +	 * from a different thread. The allocation requires finding a
> > > +	 * free range and storing. During the xa_insert the lock could be
> > > +	 * released, we don't want another thread taking the gap.
> > > +	 */
> > > +	mutex_lock(&ufile->umap_lock);
> > > +
> > > +	xa_lock(&ucontext->mmap_xa);
> >
> > Doesn't the mutex replace the xa_lock?
> 
> No, absolutely not. xarray must hold its internal lock when required. The
> external lock is only about protecting the contents
> 
> I'm not sure why this needs to hold this mutex, the spinlock looks OK.
> 
You pointed this out in "v7" xa_insert can release the lock while allocating memory leading
To a race that another thread could squeeze into the gap in the meantime. 

> > > +
> > > +	/* We want to find an empty range */
> > > +	npages = (u32)DIV_ROUND_UP(length, PAGE_SIZE);
> > > +	entry->npages = npages;
> > > +	do {
> > > +		/* First find an empty index */
> > > +		xas_find_marked(&xas, U32_MAX, XA_FREE_MARK);
> > > +		if (xas.xa_node == XAS_RESTART)
> > > +			goto err_unlock;
> > > +
> > > +		xa_first = xas.xa_index;
> > > +
> > > +		/* Is there enough room to have the range? */
> > > +		if (check_add_overflow(xa_first, npages, &xa_last))
> > > +			goto err_unlock;
> > > +
> > > +		/* Now look for the next present entry. If such doesn't
> > > +		 * exist, we found an empty range and can proceed
> > > +		 */
> > > +		xas_next_entry(&xas, xa_last - 1);
> > > +		if (xas.xa_node == XAS_BOUNDS || xas.xa_index >= xa_last)
> > > +			break;
> > > +		/* o/w look for the next free entry */
> > > +	} while (true);
> 
> while(true) not do/while is the usual convention
ok
> 
> Jason