Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()

[Jason Gunthorpe]

On Wed, Feb 03, 2021 at 04:15:53PM -0800, John Hubbard wrote:
> > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> > index 2dde99a9ba07..ea4ebb3261d9 100644
> > +++ b/drivers/infiniband/core/umem.c
> > @@ -47,17 +47,17 @@
> >   static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
> >   {
> > -	struct sg_page_iter sg_iter;
> > -	struct page *page;
> > +	bool make_dirty = umem->writable && dirty;
> > +	struct scatterlist *sg;
> > +	int i;
> 
> Maybe unsigned int is better, so as to perfectly match the scatterlist.length.

Yes please

> >   	if (umem->nmap > 0)
> >   		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
> >   				DMA_BIDIRECTIONAL);
> > -	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
> > -		page = sg_page_iter_page(&sg_iter);
> > -		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
> > -	}
> > +	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)
> 
> The change from umem->sg_nents to umem->nmap looks OK, although we should get
> IB people to verify that there is not some odd bug or reason to leave it as is.

No, nmap wouldn't be right here. nmap is the number of dma mapped SGLs
in the list and should only be used by things doing sg_dma* stuff.

umem->sg_nents is the number of CPU SGL entries and is the correct
thing here.

> > +		unpin_user_page_range_dirty_lock(sg_page(sg),
> > +			DIV_ROUND_UP(sg->length, PAGE_SIZE), make_dirty);
> 
> Is it really OK to refer directly to sg->length? The scatterlist library goes
> to some effort to avoid having callers directly access the struct member variables.

Yes, only the dma length has acessors

Jason