[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240209003115.GA31743@ziepe.ca>
Date: Thu, 8 Feb 2024 20:31:15 -0400
From: Jason Gunthorpe <jgg@...pe.ca>
To: Konstantin Taranov <kotaranov@...rosoft.com>
Cc: Long Li <longli@...rosoft.com>,
Konstantin Taranov <kotaranov@...ux.microsoft.com>,
"leon@...nel.org" <leon@...nel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH rdma-next v1 1/1] RDMA/mana_ib: Fix bug in creation of
dma regions
> > > > > +
> > > > > + 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
Powered by blists - more mailing lists