Re: [PATCH 20/34] xen: convert put_page() to put_user_page*()
On 8/2/19 9:09 AM, Weiny, Ira wrote: On 02.08.19 07:48, John Hubbard wrote: On 8/1/19 9:36 PM, Juergen Gross wrote: On 02.08.19 04:19, john.hubb...@gmail.com wrote: From: John Hubbard ... If that's not the case (both here, and in 3 or 4 other patches in this series, then as you said, I should add NULL checks to put_user_pages() and put_user_pages_dirty_lock(). In this case it is not correct, but can easily be handled. The NULL case can occur only in an error case with the pages array filled partially or not at all. I'd prefer something like the attached patch here. I'm not an expert in this code and have not looked at it carefully but that patch does seem to be the better fix than forcing NULL checks on everyone. OK, I'll use Juergen's approach, and also check for that pattern in the other patches. thanks, -- John Hubbard NVIDIA ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH 20/34] xen: convert put_page() to put_user_page*()
>
> On 02.08.19 07:48, John Hubbard wrote:
> > On 8/1/19 9:36 PM, Juergen Gross wrote:
> >> On 02.08.19 04:19, john.hubb...@gmail.com wrote:
> >>> From: John Hubbard
> > ...
> >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index
> >>> 2f5ce7230a43..29e461dbee2d 100644
> >>> --- a/drivers/xen/privcmd.c
> >>> +++ b/drivers/xen/privcmd.c
> >>> @@ -611,15 +611,10 @@ static int lock_pages(
> >>> static void unlock_pages(struct page *pages[], unsigned int
> >>> nr_pages)
> >>> {
> >>> - unsigned int i;
> >>> -
> >>> if (!pages)
> >>> return;
> >>> - for (i = 0; i < nr_pages; i++) {
> >>> - if (pages[i])
> >>> - put_page(pages[i]);
> >>> - }
> >>> + put_user_pages(pages, nr_pages);
> >>
> >> You are not handling the case where pages[i] is NULL here. Or am I
> >> missing a pending patch to put_user_pages() here?
> >>
> >
> > Hi Juergen,
> >
> > You are correct--this no longer handles the cases where pages[i] is
> > NULL. It's intentional, though possibly wrong. :)
> >
> > I see that I should have added my standard blurb to this commit
> > description. I missed this one, but some of the other patches have it.
> > It makes the following, possibly incorrect claim:
> >
> > "This changes the release code slightly, because each page slot in the
> > page_list[] array is no longer checked for NULL. However, that check
> > was wrong anyway, because the get_user_pages() pattern of usage here
> > never allowed for NULL entries within a range of pinned pages."
> >
> > The way I've seen these page arrays used with get_user_pages(), things
> > are either done single page, or with a contiguous range. So unless I'm
> > missing a case where someone is either
> >
> > a) releasing individual pages within a range (and thus likely messing
> > up their count of pages they have), or
> >
> > b) allocating two gup ranges within the same pages[] array, with a gap
> > between the allocations,
> >
> > ...then it should be correct. If so, then I'll add the above blurb to
> > this patch's commit description.
> >
> > If that's not the case (both here, and in 3 or 4 other patches in this
> > series, then as you said, I should add NULL checks to put_user_pages()
> > and put_user_pages_dirty_lock().
>
> In this case it is not correct, but can easily be handled. The NULL case can
> occur only in an error case with the pages array filled partially or not at
> all.
>
> I'd prefer something like the attached patch here.
I'm not an expert in this code and have not looked at it carefully but that
patch does seem to be the better fix than forcing NULL checks on everyone.
Ira
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 20/34] xen: convert put_page() to put_user_page*()
On 02.08.19 07:48, John Hubbard wrote:
On 8/1/19 9:36 PM, Juergen Gross wrote:
On 02.08.19 04:19, john.hubb...@gmail.com wrote:
From: John Hubbard
...
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..29e461dbee2d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,15 +611,10 @@ static int lock_pages(
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
- unsigned int i;
-
if (!pages)
return;
- for (i = 0; i < nr_pages; i++) {
- if (pages[i])
- put_page(pages[i]);
- }
+ put_user_pages(pages, nr_pages);
You are not handling the case where pages[i] is NULL here. Or am I
missing a pending patch to put_user_pages() here?
Hi Juergen,
You are correct--this no longer handles the cases where pages[i]
is NULL. It's intentional, though possibly wrong. :)
I see that I should have added my standard blurb to this
commit description. I missed this one, but some of the other patches
have it. It makes the following, possibly incorrect claim:
"This changes the release code slightly, because each page slot in the
page_list[] array is no longer checked for NULL. However, that check
was wrong anyway, because the get_user_pages() pattern of usage here
never allowed for NULL entries within a range of pinned pages."
The way I've seen these page arrays used with get_user_pages(),
things are either done single page, or with a contiguous range. So
unless I'm missing a case where someone is either
a) releasing individual pages within a range (and thus likely messing
up their count of pages they have), or
b) allocating two gup ranges within the same pages[] array, with a
gap between the allocations,
...then it should be correct. If so, then I'll add the above blurb
to this patch's commit description.
If that's not the case (both here, and in 3 or 4 other patches in this
series, then as you said, I should add NULL checks to put_user_pages()
and put_user_pages_dirty_lock().
In this case it is not correct, but can easily be handled. The NULL case
can occur only in an error case with the pages array filled partially or
not at all.
I'd prefer something like the attached patch here.
Juergen
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..12bd3154126d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -582,10 +582,11 @@ static long privcmd_ioctl_mmap_batch(
static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
- struct page *pages[], unsigned int nr_pages)
+ struct page *pages[], unsigned int *nr_pages)
{
- unsigned int i;
+ unsigned int i, free = *nr_pages;
+ *nr_pages = 0;
for (i = 0; i < num; i++) {
unsigned int requested;
int pinned;
@@ -593,35 +594,22 @@ static int lock_pages(
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
PAGE_SIZE);
- if (requested > nr_pages)
+ if (requested > free)
return -ENOSPC;
pinned = get_user_pages_fast(
(unsigned long) kbufs[i].uptr,
- requested, FOLL_WRITE, pages);
+ requested, FOLL_WRITE, pages + *nr_pages);
if (pinned < 0)
return pinned;
- nr_pages -= pinned;
- pages += pinned;
+ free -= pinned;
+ *nr_pages += pinned;
}
return 0;
}
-static void unlock_pages(struct page *pages[], unsigned int nr_pages)
-{
- unsigned int i;
-
- if (!pages)
- return;
-
- for (i = 0; i < nr_pages; i++) {
- if (pages[i])
- put_page(pages[i]);
- }
-}
-
static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
{
struct privcmd_data *data = file->private_data;
@@ -681,11 +669,12 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
xbufs = kcalloc(kdata.num, sizeof(*xbufs), GFP_KERNEL);
if (!xbufs) {
+ nr_pages = 0;
rc = -ENOMEM;
goto out;
}
- rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
+ rc = lock_pages(kbufs, kdata.num, pages, _pages);
if (rc)
goto out;
@@ -699,7 +688,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
xen_preemptible_hcall_end();
out:
- unlock_pages(pages, nr_pages);
+ if (pages)
+ put_user_pages(pages, nr_pages);
kfree(xbufs);
kfree(pages);
kfree(kbufs);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 20/34] xen: convert put_page() to put_user_page*()
On 8/1/19 9:36 PM, Juergen Gross wrote:
On 02.08.19 04:19, john.hubb...@gmail.com wrote:
From: John Hubbard
...
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..29e461dbee2d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,15 +611,10 @@ static int lock_pages(
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
- unsigned int i;
-
if (!pages)
return;
- for (i = 0; i < nr_pages; i++) {
- if (pages[i])
- put_page(pages[i]);
- }
+ put_user_pages(pages, nr_pages);
You are not handling the case where pages[i] is NULL here. Or am I
missing a pending patch to put_user_pages() here?
Hi Juergen,
You are correct--this no longer handles the cases where pages[i]
is NULL. It's intentional, though possibly wrong. :)
I see that I should have added my standard blurb to this
commit description. I missed this one, but some of the other patches
have it. It makes the following, possibly incorrect claim:
"This changes the release code slightly, because each page slot in the
page_list[] array is no longer checked for NULL. However, that check
was wrong anyway, because the get_user_pages() pattern of usage here
never allowed for NULL entries within a range of pinned pages."
The way I've seen these page arrays used with get_user_pages(),
things are either done single page, or with a contiguous range. So
unless I'm missing a case where someone is either
a) releasing individual pages within a range (and thus likely messing
up their count of pages they have), or
b) allocating two gup ranges within the same pages[] array, with a
gap between the allocations,
...then it should be correct. If so, then I'll add the above blurb
to this patch's commit description.
If that's not the case (both here, and in 3 or 4 other patches in this
series, then as you said, I should add NULL checks to put_user_pages()
and put_user_pages_dirty_lock().
thanks,
--
John Hubbard
NVIDIA
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 20/34] xen: convert put_page() to put_user_page*()
On 02.08.19 04:19, john.hubb...@gmail.com wrote:
From: John Hubbard
For pages that were retained via get_user_pages*(), release those pages
via the new put_user_page*() routines, instead of via put_page() or
release_pages().
This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
("mm: introduce put_user_page*(), placeholder versions").
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: xen-de...@lists.xenproject.org
Signed-off-by: John Hubbard
---
drivers/xen/gntdev.c | 5 +
drivers/xen/privcmd.c | 7 +--
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 4c339c7e66e5..2586b3df2bb6 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -864,10 +864,7 @@ static int gntdev_get_page(struct gntdev_copy_batch
*batch, void __user *virt,
static void gntdev_put_pages(struct gntdev_copy_batch *batch)
{
- unsigned int i;
-
- for (i = 0; i < batch->nr_pages; i++)
- put_page(batch->pages[i]);
+ put_user_pages(batch->pages, batch->nr_pages);
batch->nr_pages = 0;
}
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..29e461dbee2d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,15 +611,10 @@ static int lock_pages(
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
- unsigned int i;
-
if (!pages)
return;
- for (i = 0; i < nr_pages; i++) {
- if (pages[i])
- put_page(pages[i]);
- }
+ put_user_pages(pages, nr_pages);
You are not handling the case where pages[i] is NULL here. Or am I
missing a pending patch to put_user_pages() here?
Juergen
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 20/34] xen: convert put_page() to put_user_page*()
From: John Hubbard
For pages that were retained via get_user_pages*(), release those pages
via the new put_user_page*() routines, instead of via put_page() or
release_pages().
This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
("mm: introduce put_user_page*(), placeholder versions").
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: xen-de...@lists.xenproject.org
Signed-off-by: John Hubbard
---
drivers/xen/gntdev.c | 5 +
drivers/xen/privcmd.c | 7 +--
2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 4c339c7e66e5..2586b3df2bb6 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -864,10 +864,7 @@ static int gntdev_get_page(struct gntdev_copy_batch
*batch, void __user *virt,
static void gntdev_put_pages(struct gntdev_copy_batch *batch)
{
- unsigned int i;
-
- for (i = 0; i < batch->nr_pages; i++)
- put_page(batch->pages[i]);
+ put_user_pages(batch->pages, batch->nr_pages);
batch->nr_pages = 0;
}
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 2f5ce7230a43..29e461dbee2d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,15 +611,10 @@ static int lock_pages(
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
- unsigned int i;
-
if (!pages)
return;
- for (i = 0; i < nr_pages; i++) {
- if (pages[i])
- put_page(pages[i]);
- }
+ put_user_pages(pages, nr_pages);
}
static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
--
2.22.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
