Re: [PATCH v6 rdma-next 1/6] RDMA/core: Create mmap database and cookie helper functions

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jul 25, 2019 at 07:34:15PM +0000, Michal Kalderon wrote:
> > > +	ibdev_dbg(ucontext->device,
> > > +		  "mmap: obj[0x%p] key[%#llx] addr[%#llx] len[%#llx]
> > removed\n",
> > > +		  entry->obj, key, entry->address, entry->length);
> > > +
> > > +	return entry;
> > > +}
> > > +EXPORT_SYMBOL(rdma_user_mmap_entry_get);
> > 
> > It is a mistake we keep making, and maybe the war is hopelessly lost now,
> > but functions called from a driver should not be part of the ib_uverbs module
> > - ideally uverbs is an optional module. They should be in ib_core.
> > 
> > Maybe put this in ib_core_uverbs.c ?

> But if there isn't ib_uverbs user apps can't be run right ? and then
> these functions Won't get called anyway ?

Right, but, we don't want loading the driver to force creating
/dev/infiniband/uverbs - so the driver support component of uverbs
should live in ib_core, and the /dev/ component should be in ib_uverbs

> > > +	xa_lock(&ucontext->mmap_xa);
> > > +	if (check_add_overflow(ucontext->mmap_xa_page,
> > > +			       (u32)(length >> PAGE_SHIFT),
> > 
> > Should this be divide round up ?

> For cases that length is not rounded to PAGE_SHIFT? 

It should never happen, but yes
 
> > 
> > > +			       &next_mmap_page))
> > > +		goto err_unlock;
> > 
> > I still don't like that this algorithm latches into a permanent failure when the
> > xa_page wraps.
> > 
> > It seems worth spending a bit more time here to tidy this.. Keep using the
> > mmap_xa_page scheme, but instead do something like
> > 
> > alloc_cyclic_range():
> > 
> > while () {
> >    // Find first empty element in a cyclic way
> >    xa_page_first = mmap_xa_page;
> >    xa_find(xa, &xa_page_first, U32_MAX, XA_FREE_MARK)
> > 
> >    // Is there a enough room to have the range?
> >    if (check_add_overflow(xa_page_first, npages, &xa_page_end)) {
> >       mmap_xa_page = 0;
> >       continue;
> >    }
> > 
> >    // See if the element before intersects
> >    elm = xa_find(xa, &zero, xa_page_end, 0);
> >    if (elm && intersects(xa_page_first, xa_page_last, elm->first, elm->last)) {
> >       mmap_xa_page = elm->last + 1;
> >       continue
> >    }
> > 
> >    // xa_page_first -> xa_page_end should now be free
> >    xa_insert(xa, xa_page_start, entry);
> >    mmap_xa_page = xa_page_end + 1;
> >    return xa_page_start;
> > }
> > 
> > Approximately, please check it.

> But we don't free entires from the xa_array ( only when ucontext is destroyed) so how will 
> There be an empty element after we wrap ?  

Oh!

That should be fixed up too, in the general case if a user is
creating/destroying driver objects in loop we don't want memory usage
to be unbounded.

The rdma_user_mmap stuff has VMA ops that can refcount the xa entry
and now that this is core code it is easy enough to harmonize the two
things and track the xa side from the struct rdma_umap_priv

The question is, does EFA or qedr have a use model for this that
allows a userspace verb to create/destroy in a loop? ie do we need to
fix this right now?

Jason