> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Monday, July 29, 2019 5:05 PM
>
> External Email
>
> ----------------------------------------------------------------------
> On Mon, Jul 29, 2019 at 04:53:38PM +0300, Gal Pressman wrote:
> > On 29/07/2019 15:58, Michal Kalderon wrote:
> > >> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> > >> owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
> > >>
> > >>> + xa_lock(&ucontext->mmap_xa);
> > >>> + if (check_add_overflow(ucontext->mmap_xa_page,
> > >>> + (u32)(length >> PAGE_SHIFT),
> > >>> + &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.
> > > Gal & Jason,
> > >
> > > Coming back to the mmap_xa_page algorithm. I couldn't find some
> background on this.
> > > Why do you need the length to be represented in the mmap_xa_page ?
> > > Why not simply use xa_alloc_cyclic ( like in siw )
>
> I think siw is dealing with only PAGE_SIZE objects, efa had variable sized
> ones.
>
> > > This is simply a key to a mmap object...
> >
> > The intention was that the entry would "occupy" number of xarray
> > elements according to its size (in pages). It wasn't initially like
> > this, but IIRC this was preferred by Jason.
>
> It is not so critical, maybe we could drop it if it is really simplifiying. But it
> doesn't look so hard to make an xa algorithm that will be OK.
>
> The offset/length is shown in things like lsof and what not, and from a
> debugging perspective it makes a lot more sense if the offset/length are
> sensible, ie they should not overlap.
>
Thanks for the clarification
> Jason
