Re: [PATCH for-next] RDMA/core: Fix best page size finding when it can cross SG entries

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Feb 13, 2025 at 04:30:11PM +0200, Margolin, Michael wrote:
> 
> On 2/13/2025 4:04 PM, Jason Gunthorpe wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Thu, Feb 13, 2025 at 02:51:26PM +0200, Leon Romanovsky wrote:
> > > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> > > index e7e428369159..63a92d6cfbc2 100644
> > > --- a/drivers/infiniband/core/umem.c
> > > +++ b/drivers/infiniband/core/umem.c
> > > @@ -112,8 +112,7 @@ unsigned long ib_umem_find_best_pgsz(struct ib_umem *umem,
> > >                  /* If the current entry is physically contiguous with the previous
> > >                   * one, no need to take its start addresses into consideration.
> > >                   */
> > > -               if (curr_base + curr_len != sg_dma_address(sg)) {
> > > -
> > > +               if (curr_base != sg_dma_address(sg) - curr_len) {
> > >                          curr_base = sg_dma_address(sg);
> > >                          curr_len = 0;
> > I'm not sure about this, what ensures sg_dma_address() > curr_len?
> > 
> > curr_base + curr_len could also overflow, we've seen that AMD IOMMU
> > sometimes uses the very high addresess already
> 
> I think the only case we care about where curr_base + curr_len can overflow
> is when next sg_dma_address() == 0.
> 
> But maybe we should just add an explicit check:
> 
> -               if (curr_base + curr_len != sg_dma_address(sg)) {
> +               if (curr_base + curr_len < curr_base ||
> +                   curr_base + curr_len != sg_dma_address(sg)) {
>                         curr_base = sg_dma_address(sg);
>                         curr_len = 0;

Ugh

I wonder if we should try to make a overflow.h helper for these kinds
of problems.

/* Check if a + n == b, failing if a+n overflows */
check_consecutive(a, n, b)

?

It is a fairly common problem

I suggest to take the patch as it originally was and try to propose
the above helper?

Jason