>Subject: Re: [rdma-next 10/12] RDMA/nes: Use for_each_sg_dma_page iterator
>on umem SGL
>
>On Mon, Feb 11, 2019 at 09:25:06AM -0600, Shiraz Saleem wrote:
>> From: "Shiraz, Saleem" <shiraz.saleem@xxxxxxxxx>
>>
>> Use the for_each_sg_dma_page iterator variant to walk the umem
>> DMA-mapped SGL and get the page DMA address. This avoids the extra
>> loop to iterate pages in the SGE when for_each_sg iterator is used.
>>
>> Additionally, purge umem->page_shift usage in the driver as its only
>> relevant for ODP MRs. Use system page size and shift instead.
>>
[.....]
>> - for_each_sg(region->sg_head.sgl, sg, region->nmap,
>entry) {
>> - if (sg_dma_address(sg) & ~PAGE_MASK) {
>> + for_each_sg_dma_page(region->sg_head.sgl,
>&dma_iter, region->nmap, 0) {
>> + struct sg_page_iter *piter = &dma_iter.base;
>> +
>> + if (sg_dma_address(piter->sg) & ~PAGE_MASK) {
>
>Why add a piter variable?
>
>Isn't this just sg_page_iter_dma_address(&dma_iter); ?
In retrospect, I think so.
I was trying to mirror what the original code was doing which is
checking if the dma addr of the sge is page aligned and piter variable is used
to get the sg holding the page. But the original code assumed a 1:1 mapping
between sge and system page in umem sgl. So probably was really checking
if the dma addr of pages in the SGL are page aligned.
>
>
>> ib_umem_release(region);
>> nes_free_resource(nesadapter,
>nesadapter->allocated_mrs, stag_index);
>> nes_debug(NES_DBG_MR, "Unaligned
>Memory Buffer: 0x%x\n",
>> - (unsigned int)
>sg_dma_address(sg));
>> + (unsigned
>int)sg_dma_address(piter->sg));
>> ibmr = ERR_PTR(-EINVAL);
>> kfree(nesmr);
>> goto reg_user_mr_err;
>> }
>>
>> - if (!sg_dma_len(sg)) {
>> + if (!sg_dma_len(piter->sg)) {
>
>It makes no sense to mix sg_dma_len with the iterator. I think this dma_len can't
>be zero so this protective if should just be deleted.
Makes sense.
>> ib_umem_release(region);
>> nes_free_resource(nesadapter,
>nesadapter->allocated_mrs,
>> stag_index);
>> @@ -2204,103 +2204,94 @@ static struct ib_mr *nes_reg_user_mr(struct
>ib_pd *pd, u64 start, u64 length,
>> goto reg_user_mr_err;
>> }
>>
>> - region_length += sg_dma_len(sg);
>> - chunk_pages = sg_dma_len(sg) >> 12;
>> + region_length += PAGE_SIZE;
>
>And this region_length thing not be OK... sg_dma_len is actually smaller than
>PAGE_SIZE in some cases - but this only seems fed into debuging, so let us
>ignore.
OK.
