> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
>
> 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
ok
>
> > >
> > > > + &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?
The mapping occurs for every qp and cq creation. So yes.
So do you mean add a ref-cnt to the xarray entry and from umap decrease the refcnt and free?
>
> Jason
