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

[Jason Gunthorpe <jgg@ziepe.ca>]

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

> > +	/* 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.

> > +
> > +	/* 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

Jason