* [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()
@ 2020-10-30 16:32 Jason Gunthorpe
2020-10-30 20:50 ` Ira Weiny
0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2020-10-30 16:32 UTC (permalink / raw)
To: Andrew Morton, linux-mm
Cc: Aneesh Kumar K.V, Claudio Imbrenda, Ira Weiny, Jan Kara, John Hubbard
When FOLL_PIN is passed to __get_user_pages() the page list must be put
back using unpin_user_pages() otherwise the page pin reference persists in
a corrupted state.
Cc: <stable@kernel.org>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
mm/gup.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
I don't have any way to test CMA stuff, this was noticed by inspection.
diff --git a/mm/gup.c b/mm/gup.c
index 1bb349e5ed212a..2e26757f3c9276 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
/*
* drop the above get_user_pages reference.
*/
- for (i = 0; i < nr_pages; i++)
- put_page(pages[i]);
+ if (gup_flags & FOLL_PIN)
+ unpin_user_pages(pages, nr_pages);
+ else
+ for (i = 0; i < nr_pages; i++)
+ put_page(pages[i]);
if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
(unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()
2020-10-30 16:32 [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages() Jason Gunthorpe
@ 2020-10-30 20:50 ` Ira Weiny
2020-10-30 21:39 ` John Hubbard
2020-10-30 23:58 ` Jason Gunthorpe
0 siblings, 2 replies; 5+ messages in thread
From: Ira Weiny @ 2020-10-30 20:50 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrew Morton, linux-mm, Aneesh Kumar K.V, Claudio Imbrenda,
Jan Kara, John Hubbard
On Fri, Oct 30, 2020 at 01:32:28PM -0300, Jason Gunthorpe wrote:
> When FOLL_PIN is passed to __get_user_pages() the page list must be put
> back using unpin_user_pages() otherwise the page pin reference persists in
> a corrupted state.
>
> Cc: <stable@kernel.org>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> mm/gup.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> I don't have any way to test CMA stuff, this was noticed by inspection.
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 1bb349e5ed212a..2e26757f3c9276 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
> /*
> * drop the above get_user_pages reference.
> */
I wonder if that comment should be deleted/modified? It does not seem to apply
any longer.
Also, looks like there is another place this occurs right before the call to
check_and_migrate_cma_pages() in __gup_longterm_locked()
1730 if (check_dax_vmas(vmas_tmp, rc)) {
1731 for (i = 0; i < rc; i++)
1732 put_page(pages[i]);
1733 rc = -EOPNOTSUPP;
1734 goto out;
1735 }
We should change that site as well.
And since we now have 2 places should this be a helper?
Ira
> - for (i = 0; i < nr_pages; i++)
> - put_page(pages[i]);
> + if (gup_flags & FOLL_PIN)
> + unpin_user_pages(pages, nr_pages);
> + else
> + for (i = 0; i < nr_pages; i++)
> + put_page(pages[i]);
>
> if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
> (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()
2020-10-30 20:50 ` Ira Weiny
@ 2020-10-30 21:39 ` John Hubbard
2020-10-30 23:58 ` Jason Gunthorpe
1 sibling, 0 replies; 5+ messages in thread
From: John Hubbard @ 2020-10-30 21:39 UTC (permalink / raw)
To: Ira Weiny, Jason Gunthorpe
Cc: Andrew Morton, linux-mm, Aneesh Kumar K.V, Claudio Imbrenda, Jan Kara
On 10/30/20 1:50 PM, Ira Weiny wrote:
> On Fri, Oct 30, 2020 at 01:32:28PM -0300, Jason Gunthorpe wrote:
>> When FOLL_PIN is passed to __get_user_pages() the page list must be put
>> back using unpin_user_pages() otherwise the page pin reference persists in
>> a corrupted state.
>>
>> Cc: <stable@kernel.org>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>> mm/gup.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> I don't have any way to test CMA stuff, this was noticed by inspection.
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 1bb349e5ed212a..2e26757f3c9276 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
>> /*
>> * drop the above get_user_pages reference.
>> */
>
> I wonder if that comment should be deleted/modified? It does not seem to apply
> any longer.
Modify, maybe. But it's still relevant: the pages came from either gup or pup.
>
> Also, looks like there is another place this occurs right before the call to
> check_and_migrate_cma_pages() in __gup_longterm_locked()
>
> 1730 if (check_dax_vmas(vmas_tmp, rc)) {
> 1731 for (i = 0; i < rc; i++)
> 1732 put_page(pages[i]);
> 1733 rc = -EOPNOTSUPP;
> 1734 goto out;
> 1735 }
>
> We should change that site as well.
>
> And since we now have 2 places should this be a helper?
>
+1
thanks,
--
John Hubbard
NVIDIA
> Ira
>
>> - for (i = 0; i < nr_pages; i++)
>> - put_page(pages[i]);
>> + if (gup_flags & FOLL_PIN)
>> + unpin_user_pages(pages, nr_pages);
>> + else
>> + for (i = 0; i < nr_pages; i++)
>> + put_page(pages[i]);
>>
>> if (migrate_pages(&cma_page_list, alloc_migration_target, NULL,
>> (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) {
>> --
>> 2.28.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()
2020-10-30 20:50 ` Ira Weiny
2020-10-30 21:39 ` John Hubbard
@ 2020-10-30 23:58 ` Jason Gunthorpe
2020-11-02 18:54 ` Ira Weiny
1 sibling, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2020-10-30 23:58 UTC (permalink / raw)
To: Ira Weiny
Cc: Andrew Morton, linux-mm, Aneesh Kumar K.V, Claudio Imbrenda,
Jan Kara, John Hubbard
On Fri, Oct 30, 2020 at 01:50:26PM -0700, Ira Weiny wrote:
> On Fri, Oct 30, 2020 at 01:32:28PM -0300, Jason Gunthorpe wrote:
> > When FOLL_PIN is passed to __get_user_pages() the page list must be put
> > back using unpin_user_pages() otherwise the page pin reference persists in
> > a corrupted state.
> >
> > Cc: <stable@kernel.org>
> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > mm/gup.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > I don't have any way to test CMA stuff, this was noticed by inspection.
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 1bb349e5ed212a..2e26757f3c9276 100644
> > +++ b/mm/gup.c
> > @@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
> > /*
> > * drop the above get_user_pages reference.
> > */
>
> I wonder if that comment should be deleted/modified? It does not seem to apply
> any longer.
It is still basically right, the 'above' is just a bit vauge.. Don't
want to touch extra stuff for a stable patch.
> Also, looks like there is another place this occurs right before the call to
> check_and_migrate_cma_pages() in __gup_longterm_locked()
>
> 1730 if (check_dax_vmas(vmas_tmp, rc)) {
> 1731 for (i = 0; i < rc; i++)
> 1732 put_page(pages[i]);
> 1733 rc = -EOPNOTSUPP;
> 1734 goto out;
> 1735 }
Oh, yes! I will update this in v2 - good eyes
> And since we now have 2 places should this be a helper?
I have another patch that deletes check_dax_vmas() and this code
https://github.com/jgunthorpe/linux/commit/48ee608271e124e4a89353b9694502372c1b2df0
Does it look OK to you? I was going to check it again and post next
week
Thus for this stable patch let's just leave it simple?
Thanks,
Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages()
2020-10-30 23:58 ` Jason Gunthorpe
@ 2020-11-02 18:54 ` Ira Weiny
0 siblings, 0 replies; 5+ messages in thread
From: Ira Weiny @ 2020-11-02 18:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrew Morton, linux-mm, Aneesh Kumar K.V, Claudio Imbrenda,
Jan Kara, John Hubbard
On Fri, Oct 30, 2020 at 08:58:25PM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 30, 2020 at 01:50:26PM -0700, Ira Weiny wrote:
> > On Fri, Oct 30, 2020 at 01:32:28PM -0300, Jason Gunthorpe wrote:
> > > When FOLL_PIN is passed to __get_user_pages() the page list must be put
> > > back using unpin_user_pages() otherwise the page pin reference persists in
> > > a corrupted state.
> > >
> > > Cc: <stable@kernel.org>
> > > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > > Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > mm/gup.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > I don't have any way to test CMA stuff, this was noticed by inspection.
> > >
> > > diff --git a/mm/gup.c b/mm/gup.c
> > > index 1bb349e5ed212a..2e26757f3c9276 100644
> > > +++ b/mm/gup.c
> > > @@ -1630,8 +1630,11 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm,
> > > /*
> > > * drop the above get_user_pages reference.
> > > */
> >
> > I wonder if that comment should be deleted/modified? It does not seem to apply
> > any longer.
>
> It is still basically right, the 'above' is just a bit vauge.. Don't
> want to touch extra stuff for a stable patch.
>
> > Also, looks like there is another place this occurs right before the call to
> > check_and_migrate_cma_pages() in __gup_longterm_locked()
> >
> > 1730 if (check_dax_vmas(vmas_tmp, rc)) {
> > 1731 for (i = 0; i < rc; i++)
> > 1732 put_page(pages[i]);
> > 1733 rc = -EOPNOTSUPP;
> > 1734 goto out;
> > 1735 }
>
> Oh, yes! I will update this in v2 - good eyes
>
> > And since we now have 2 places should this be a helper?
>
> I have another patch that deletes check_dax_vmas() and this code
>
> https://github.com/jgunthorpe/linux/commit/48ee608271e124e4a89353b9694502372c1b2df0
>
> Does it look OK to you? I was going to check it again and post next
> week
I think it is ok... This code has gotten twisted a bit. I wonder if
__get_user_pages_remote() could also be cleaned up after this patch?
Also, __gup_longterm_locked() seems a bit of an odd name after that change.
However, I don't have a better name so I think it is fine to leave it for now.
It seems like the CMA 'longterm' checks are more about pinning but I don't
recall the details ATM.
>
> Thus for this stable patch let's just leave it simple?
Yea good for stable!
Ira
>
> Thanks,
> Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-02 18:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-30 16:32 [PATCH rc] mm/gup: use unpin_user_pages() in check_and_migrate_cma_pages() Jason Gunthorpe
2020-10-30 20:50 ` Ira Weiny
2020-10-30 21:39 ` John Hubbard
2020-10-30 23:58 ` Jason Gunthorpe
2020-11-02 18:54 ` Ira Weiny
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).