Re: [PATCH rdma-next v1 1/1] RDMA/mana_ib: Fix bug in creation of dma regions

[Jason Gunthorpe]

> > > > > +
> > > > > + if (force_zero_offset) {
> > > > > +         while (ib_umem_dma_offset(umem, page_sz) && page_sz >
> > > > > PAGE_SIZE)
> > > > > +                 page_sz /= 2;
> > > > > +         if (ib_umem_dma_offset(umem, page_sz) != 0) {
> > > > > +                 ibdev_dbg(&dev->ib_dev, "failed to find page
> > > > > + size to
> > > > > force zero offset.\n");
> > > > > +                 return -ENOMEM;
> > > > > +         }
> > > > > + }
> > > > > +
> > 
> > Yes this doesn't look quite right..
> > 
> > It should flow from the HW capability, the helper you call should be tightly
> > linked to what the HW can do.
> > 
> > ib_umem_find_best_pgsz() is used for MRs that have the usual
> >   offset = IOVA % pgsz
> > 
> > We've always created other helpers for other restrictions.
> > 
> > So you should move your "force_zero_offset" into another helper and
> > describe exactly how the HW works to support the calculation
> > 
> > It is odd to have the offset loop and be using
> > ib_umem_find_best_pgsz() with some iova, usually you'd use
> > ib_umem_find_best_pgoff() in those cases, see the other callers.
> 
> Hi Jason,
> Thanks for the comments.
> 
> To be honest, I do not understand how I could employ ib_umem_find_best_pgoff
> for my purpose. As well as I do not see any mistake in the patch, and I think you neither.

It does exactly the same thing, it is just intended to be used by
things that are not doing the IOVA calculation. It is a matter of documentation.

> I can make a special helper, but I do not think that it will be useful to anyone. Plus,
> there is no better approach then halving the page size, so the helper will end up with that
> loop under the hood. As I see mlnx also uses a loop with halving page_sz, but for a different
> purpose, I do not see why our code cannot do the same without a special helper.

Are you sure you don't need the length check too? You have a granular
size but not a granular offset?

In that case yes, a helper does not seem necessary

However, you should still be calling ib_umem_find_best_pgoff() for
the initialize sizing as a matter of clarity since this is not a MR
and does not use IOVA addressing.

Jason