From: John Hubbard <jhubbard@nvidia.com> To: Bharath Vedartham <linux.bhar@gmail.com>, <gregkh@linuxfoundation.org>, <Matt.Sickler@daktronics.com> Cc: "Ira Weiny" <ira.weiny@intel.com>, "Jérôme Glisse" <jglisse@redhat.com>, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v4] staging: kpc2000: Convert put_page() to put_user_page*() Date: Fri, 2 Aug 2019 17:13:42 -0700 [thread overview] Message-ID: <4467d671-d011-0ebc-e2de-48a9745d4fe6@nvidia.com> (raw) In-Reply-To: <1564058658-3551-1-git-send-email-linux.bhar@gmail.com> On 7/25/19 5:44 AM, Bharath Vedartham wrote: > For pages that were retained via get_user_pages*(), release those pages > via the new put_user_page*() routines, instead of via put_page(). > > This is part a tree-wide conversion, as described in commit fc1d8e7cca2d > ("mm: introduce put_user_page*(), placeholder versions"). > Hi Bharath, If you like, I could re-post your patch here, modified slightly, as part of the next version of the miscellaneous call site conversion series [1]. As part of that, we should change this to use put_user_pages_dirty_lock() (see below). > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Matt Sickler <Matt.Sickler@daktronics.com> > Cc: devel@driverdev.osuosl.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mm@kvack.org > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com> > --- > Changes since v1 > - Improved changelog by John's suggestion. > - Moved logic to dirty pages below sg_dma_unmap > and removed PageReserved check. > Changes since v2 > - Added back PageResevered check as > suggested by John Hubbard. > Changes since v3 > - Changed the changelog as suggested by John. > - Added John's Reviewed-By tag. > Changes since v4 > - Rebased the patch on the staging tree. > - Improved commit log by fixing a line wrap. > --- > drivers/staging/kpc2000/kpc_dma/fileops.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c > index 48ca88b..f15e292 100644 > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c > @@ -190,9 +190,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv, > sg_free_table(&acd->sgt); > err_dma_map_sg: > err_alloc_sg_table: > - for (i = 0 ; i < acd->page_count ; i++) { > - put_page(acd->user_pages[i]); > - } > + put_user_pages(acd->user_pages, acd->page_count); > err_get_user_pages: > kfree(acd->user_pages); > err_alloc_userpages: > @@ -211,16 +209,13 @@ void transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags) > BUG_ON(acd->ldev == NULL); > BUG_ON(acd->ldev->pldev == NULL); > > - for (i = 0 ; i < acd->page_count ; i++) { > - if (!PageReserved(acd->user_pages[i])) { > - set_page_dirty(acd->user_pages[i]); > - } > - } > - > dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir); > > - for (i = 0 ; i < acd->page_count ; i++) { > - put_page(acd->user_pages[i]); > + for (i = 0; i < acd->page_count; i++) { > + if (!PageReserved(acd->user_pages[i])) > + put_user_pages_dirty(&acd->user_pages[i], 1); This would change to: put_user_pages_dirty_lock(&acd->user_pages[i], 1, true); ...and we'd add this blurb (this time with CH's name spelled properly) to the commit description: Note that this effectively changes the code's behavior in qp_release_pages(): it now ultimately calls set_page_dirty_lock(), instead of set_page_dirty(). This is probably more accurate. As Christoph Hellwig put it, "set_page_dirty() is only safe if we are dealing with a file backed page where we have reference on the inode it hangs off." [1] [1] https://lore.kernel.org/r/20190723153640.GB720@lst.de Also, future: I don't know the driver well enough to say, but maybe "true" could be replaced by "acd->ldev->dir == DMA_FROM_DEVICE", there, but that would be a separate patch. thanks, -- John Hubbard NVIDIA > + else > + put_user_page(acd->user_pages[i]); > } > > sg_free_table(&acd->sgt); >
WARNING: multiple messages have this Message-ID (diff)
From: jhubbard@nvidia.com (John Hubbard) Subject: [PATCH v4] staging: kpc2000: Convert put_page() to put_user_page*() Date: Fri, 2 Aug 2019 17:13:42 -0700 [thread overview] Message-ID: <4467d671-d011-0ebc-e2de-48a9745d4fe6@nvidia.com> (raw) In-Reply-To: <1564058658-3551-1-git-send-email-linux.bhar@gmail.com> On 7/25/19 5:44 AM, Bharath Vedartham wrote: > For pages that were retained via get_user_pages*(), release those pages > via the new put_user_page*() routines, instead of via put_page(). > > This is part a tree-wide conversion, as described in commit fc1d8e7cca2d > ("mm: introduce put_user_page*(), placeholder versions"). > Hi Bharath, If you like, I could re-post your patch here, modified slightly, as part of the next version of the miscellaneous call site conversion series [1]. As part of that, we should change this to use put_user_pages_dirty_lock() (see below). > Cc: Ira Weiny <ira.weiny at intel.com> > Cc: John Hubbard <jhubbard at nvidia.com> > Cc: J?r?me Glisse <jglisse at redhat.com> > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org> > Cc: Matt Sickler <Matt.Sickler at daktronics.com> > Cc: devel at driverdev.osuosl.org > Cc: linux-kernel at vger.kernel.org > Cc: linux-mm at kvack.org > Reviewed-by: John Hubbard <jhubbard at nvidia.com> > Signed-off-by: Bharath Vedartham <linux.bhar at gmail.com> > --- > Changes since v1 > - Improved changelog by John's suggestion. > - Moved logic to dirty pages below sg_dma_unmap > and removed PageReserved check. > Changes since v2 > - Added back PageResevered check as > suggested by John Hubbard. > Changes since v3 > - Changed the changelog as suggested by John. > - Added John's Reviewed-By tag. > Changes since v4 > - Rebased the patch on the staging tree. > - Improved commit log by fixing a line wrap. > --- > drivers/staging/kpc2000/kpc_dma/fileops.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c > index 48ca88b..f15e292 100644 > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c > @@ -190,9 +190,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv, > sg_free_table(&acd->sgt); > err_dma_map_sg: > err_alloc_sg_table: > - for (i = 0 ; i < acd->page_count ; i++) { > - put_page(acd->user_pages[i]); > - } > + put_user_pages(acd->user_pages, acd->page_count); > err_get_user_pages: > kfree(acd->user_pages); > err_alloc_userpages: > @@ -211,16 +209,13 @@ void transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags) > BUG_ON(acd->ldev == NULL); > BUG_ON(acd->ldev->pldev == NULL); > > - for (i = 0 ; i < acd->page_count ; i++) { > - if (!PageReserved(acd->user_pages[i])) { > - set_page_dirty(acd->user_pages[i]); > - } > - } > - > dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir); > > - for (i = 0 ; i < acd->page_count ; i++) { > - put_page(acd->user_pages[i]); > + for (i = 0; i < acd->page_count; i++) { > + if (!PageReserved(acd->user_pages[i])) > + put_user_pages_dirty(&acd->user_pages[i], 1); This would change to: put_user_pages_dirty_lock(&acd->user_pages[i], 1, true); ...and we'd add this blurb (this time with CH's name spelled properly) to the commit description: Note that this effectively changes the code's behavior in qp_release_pages(): it now ultimately calls set_page_dirty_lock(), instead of set_page_dirty(). This is probably more accurate. As Christoph Hellwig put it, "set_page_dirty() is only safe if we are dealing with a file backed page where we have reference on the inode it hangs off." [1] [1] https://lore.kernel.org/r/20190723153640.GB720 at lst.de Also, future: I don't know the driver well enough to say, but maybe "true" could be replaced by "acd->ldev->dir == DMA_FROM_DEVICE", there, but that would be a separate patch. thanks, -- John Hubbard NVIDIA > + else > + put_user_page(acd->user_pages[i]); > } > > sg_free_table(&acd->sgt); >
next prev parent reply other threads:[~2019-08-03 0:14 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-25 12:44 [PATCH v4] staging: kpc2000: Convert put_page() to put_user_page*() Bharath Vedartham 2019-08-03 0:13 ` John Hubbard [this message] 2019-08-03 0:13 ` John Hubbard -- strict thread matches above, loose matches on Subject: below -- 2019-07-25 12:44 Bharath Vedartham 2019-07-20 17:32 [PATCH v4] staging: kpc2000: Convert put_page " Bharath Vedartham 2019-07-20 17:32 ` Bharath Vedartham 2019-07-25 7:46 ` Greg KH 2019-07-25 7:46 ` Greg KH 2019-07-25 11:18 ` Bharath Vedartham
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=4467d671-d011-0ebc-e2de-48a9745d4fe6@nvidia.com \ --to=jhubbard@nvidia.com \ --cc=Matt.Sickler@daktronics.com \ --cc=devel@driverdev.osuosl.org \ --cc=gregkh@linuxfoundation.org \ --cc=ira.weiny@intel.com \ --cc=jglisse@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux.bhar@gmail.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.