From: Jason Gunthorpe <jgg@nvidia.com>
To: John Hubbard <jhubbard@nvidia.com>
Cc: Joao Martins <joao.m.martins@oracle.com>, <linux-mm@kvack.org>,
<linux-kernel@vger.kernel.org>, <linux-rdma@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Doug Ledford <dledford@redhat.com>,
Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release()
Date: Thu, 4 Feb 2021 16:00:26 -0400 [thread overview]
Message-ID: <20210204200026.GP4247@nvidia.com> (raw)
In-Reply-To: <4ed92932-8cf2-97ab-7296-6efee51fc555@nvidia.com>
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
next prev parent reply other threads:[~2021-02-04 20:02 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-03 22:00 [PATCH 0/4] mm/gup: page unpining improvements Joao Martins
2021-02-03 22:00 ` [PATCH 1/4] mm/gup: add compound page list iterator Joao Martins
2021-02-03 23:00 ` John Hubbard
2021-02-04 11:27 ` Joao Martins
2021-02-04 16:09 ` Joao Martins
2021-02-04 19:53 ` Jason Gunthorpe
2021-02-04 23:37 ` John Hubbard
2021-02-03 22:00 ` [PATCH 2/4] mm/gup: decrement head page once for group of subpages Joao Martins
2021-02-03 23:28 ` John Hubbard
2021-02-04 11:27 ` Joao Martins
2021-02-03 22:00 ` [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock() Joao Martins
2021-02-03 23:37 ` John Hubbard
2021-02-04 11:35 ` Joao Martins
2021-02-04 16:30 ` Joao Martins
2021-02-04 0:11 ` John Hubbard
2021-02-04 11:47 ` Joao Martins
2021-02-03 22:00 ` [PATCH 4/4] RDMA/umem: batch page unpin in __ib_mem_release() Joao Martins
2021-02-04 0:15 ` John Hubbard
2021-02-04 12:29 ` Joao Martins
2021-02-04 20:00 ` Jason Gunthorpe [this message]
2021-02-05 17:00 ` Joao Martins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210204200026.GP4247@nvidia.com \
--to=jgg@nvidia.com \
--cc=akpm@linux-foundation.org \
--cc=dledford@redhat.com \
--cc=jhubbard@nvidia.com \
--cc=joao.m.martins@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-rdma@vger.kernel.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).