| Message ID | 1593054160-12628-2-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] xen/privcmd: Corrected error handling path and mark pages dirty | expand |
On 2020-06-24 20:02, Souptick Joarder wrote: > In 2019, we introduced pin_user_pages*() and now we are converting > get_user_pages*() to the new API as appropriate. [1] & [2] could > be referred for more information. This is case 5 as per document [1]. > > [1] Documentation/core-api/pin_user_pages.rst > > [2] "Explicit pinning of user-space pages": > https://lwn.net/Articles/807108/ > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Paul Durrant <xadimgnik@gmail.com> > --- > Hi, > > I'm compile tested this, but unable to run-time test, so any testing > help is much appriciated. > > drivers/xen/privcmd.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index 0da417c..eb05254 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -595,7 +595,7 @@ static int lock_pages( > if (requested > nr_pages) > return -ENOSPC; > > - page_count = get_user_pages_fast( > + page_count = pin_user_pages_fast( > (unsigned long) kbufs[i].uptr, > requested, FOLL_WRITE, pages); > if (page_count < 0) { > @@ -612,13 +612,7 @@ static int lock_pages( > > static void unlock_pages(struct page *pages[], unsigned int nr_pages) > { > - unsigned int i; > - > - for (i = 0; i < nr_pages; i++) { > - if (!PageDirty(page)) > - set_page_dirty_lock(page); > - put_page(pages[i]); > - } > + unpin_user_pages_dirty_lock(pages, nr_pages, 1); "true", not "1", is the correct way to call that function. Also, this approach changes the behavior slightly, but I think it's reasonable to just set_page_dirty_lock() on the whole range--hard to see much benefit in checking PageDirty first. > } > > static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > thanks,
On Thu, Jun 25, 2020 at 11:19 AM John Hubbard <jhubbard@nvidia.com> wrote: > > On 2020-06-24 20:02, Souptick Joarder wrote: > > In 2019, we introduced pin_user_pages*() and now we are converting > > get_user_pages*() to the new API as appropriate. [1] & [2] could > > be referred for more information. This is case 5 as per document [1]. > > > > [1] Documentation/core-api/pin_user_pages.rst > > > > [2] "Explicit pinning of user-space pages": > > https://lwn.net/Articles/807108/ > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Cc: John Hubbard <jhubbard@nvidia.com> > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Cc: Paul Durrant <xadimgnik@gmail.com> > > --- > > Hi, > > > > I'm compile tested this, but unable to run-time test, so any testing > > help is much appriciated. > > > > drivers/xen/privcmd.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > > index 0da417c..eb05254 100644 > > --- a/drivers/xen/privcmd.c > > +++ b/drivers/xen/privcmd.c > > @@ -595,7 +595,7 @@ static int lock_pages( > > if (requested > nr_pages) > > return -ENOSPC; > > > > - page_count = get_user_pages_fast( > > + page_count = pin_user_pages_fast( > > (unsigned long) kbufs[i].uptr, > > requested, FOLL_WRITE, pages); > > if (page_count < 0) { > > @@ -612,13 +612,7 @@ static int lock_pages( > > > > static void unlock_pages(struct page *pages[], unsigned int nr_pages) > > { > > - unsigned int i; > > - > > - for (i = 0; i < nr_pages; i++) { > > - if (!PageDirty(page)) > > - set_page_dirty_lock(page); > > - put_page(pages[i]); > > - } > > + unpin_user_pages_dirty_lock(pages, nr_pages, 1); > > "true", not "1", is the correct way to call that function. Ok. > > Also, this approach changes the behavior slightly, but I think it's > reasonable to just set_page_dirty_lock() on the whole range--hard to > see much benefit in checking PageDirty first. unpin_user_pages_dirty_lock() internally will do the same check after patch [2/2] So I thought to keep old and new code in sync. Shall we avoid this check ? > > > > } > > > > static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > > > > thanks, > -- > John Hubbard > NVIDIA
On 2020-06-25 22:26, Souptick Joarder wrote: > On Thu, Jun 25, 2020 at 11:19 AM John Hubbard <jhubbard@nvidia.com> wrote: >> On 2020-06-24 20:02, Souptick Joarder wrote: ... >>> @@ -612,13 +612,7 @@ static int lock_pages( >>> >>> static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>> { >>> - unsigned int i; >>> - >>> - for (i = 0; i < nr_pages; i++) { >>> - if (!PageDirty(page)) >>> - set_page_dirty_lock(page); >>> - put_page(pages[i]); >>> - } >>> + unpin_user_pages_dirty_lock(pages, nr_pages, 1); >> >> "true", not "1", is the correct way to call that function. > > Ok. > >> >> Also, this approach changes the behavior slightly, but I think it's Correction, I forgot that I put that same if(!PageDirty(page)) check into unpin_user_pages_dirty_lock(). So it doesn't change behavior. That's good. >> reasonable to just set_page_dirty_lock() on the whole range--hard to >> see much benefit in checking PageDirty first. > > unpin_user_pages_dirty_lock() internally will do the same check after > patch [2/2] > So I thought to keep old and new code in sync. Shall we avoid this check ? > Just leave it as you have it, but of course use "true" instead of 1, please. thanks,
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 0da417c..eb05254 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -595,7 +595,7 @@ static int lock_pages( if (requested > nr_pages) return -ENOSPC; - page_count = get_user_pages_fast( + page_count = pin_user_pages_fast( (unsigned long) kbufs[i].uptr, requested, FOLL_WRITE, pages); if (page_count < 0) { @@ -612,13 +612,7 @@ static int lock_pages( static void unlock_pages(struct page *pages[], unsigned int nr_pages) { - unsigned int i; - - for (i = 0; i < nr_pages; i++) { - if (!PageDirty(page)) - set_page_dirty_lock(page); - put_page(pages[i]); - } + unpin_user_pages_dirty_lock(pages, nr_pages, 1); } static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
In 2019, we introduced pin_user_pages*() and now we are converting get_user_pages*() to the new API as appropriate. [1] & [2] could be referred for more information. This is case 5 as per document [1]. [1] Documentation/core-api/pin_user_pages.rst [2] "Explicit pinning of user-space pages": https://lwn.net/Articles/807108/ Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Paul Durrant <xadimgnik@gmail.com> --- Hi, I'm compile tested this, but unable to run-time test, so any testing help is much appriciated. drivers/xen/privcmd.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)