On Fri, May 03, 2019 at 04:01:27PM +0000, Saleem, Shiraz wrote: > >Subject: Re: [PATCH v2 rdma-next 1/5] RDMA/umem: Add API to find best driver > >supported page size in an MR > > > >On Fri, May 03, 2019 at 03:22:59PM +0000, Saleem, Shiraz wrote: > >> >This is because mask shouldn't start as zero - the highest possible > >> >mask is something like log2_ru(umem length) > >> > > >> >ie absent other considerations the page size at the NIC should be the > >> >size of the umem. > >> > > >> >Then we scan the sgl and reduce that value based on the physical > >> >address layout > >> > > >> >Then we reduce it again based on the uvirt vs address difference > >> > > >> >Oring a '0' into the mask means that step contributes no restriction. > >> > > >> >.. > >> > > >> >So I think the algorithm is just off as is, it should be more like > >> > > >> > // Page size can't be larger than the length of the MR mask = > >> >log2_ru(umem length); > >> > > >> > // offset into the first SGL for umem->addr pgoff = umem->address & > >> >PAGE_MASK; va = uvirt_addr; > >> > > >> > >> Did you mean pgoff = umem->address & ~PAGE_MASK? > > > >Yes... > > OK. Don't we need something like this for zero based VA? > va = uvirt_addr ? uvirt_addr : umem->addr; Every MR is created with an IOVA (here called VA). Before we get here the caller should figure out the IOVA and it should either be 0 or umem->address in the cases we implement today *however* it can really be anything and this code shouldn't care.. > Also can we do this for all HW? All hardware has to support an arbitary IOVA. > Or do we keep the IB_UMEM_VA_BASED_OFFSET flag and OR > in the dma_addr_end (for first SGL) and dma_addr_start (for last SGL) > to the mask when the flag is not set? I wasn't totally sure what that flag did.. If we don't have any drivers that need something different today then I would focus entirely on the: PA = MR_PAGE_TABLE[IOVA/PAGE_SIZE] | (IOVA & PAGE_MASK) Case, which is what I outlined. The ^ is checking that the (IOVA & PAGE_MASK) scheme will work. > > for_each_sgl() > > mask |= (sgl->dma_addr + pgoff) ^ va > > if (not first or last) > > // Interior SGLs limit by the length as well > > mask |= sgl->length; > > va += sgl->length - pgoff; > > pgoff = 0; > > > > > > >But really even that is not what it should be, the 'pgoff' should simply be the > >'offset' member of the first sgl and we should set it correctly based on the umem- > >>address when building the sgl in the core code. > > > > I can look to update it post this series. > And the core code changes to not over allocate scatter table. Sure Lets try and get this sorted out right away so it can go in this merge window, which might start on Monday. Jason
