[RFC,v2,02/16] mm,madvise: call soft_offline_page() without MF_COUNT_INCREASED
diff mbox series

Message ID 20191017142123.24245-3-osalvador@suse.de
State New
Headers show
Series
  • Hwpoison rework {hard,soft}-offline
Related show

Commit Message

Oscar Salvador Oct. 17, 2019, 2:21 p.m. UTC
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

The call to get_user_pages_fast is only to get the pointer to a struct
page of a given address, pinning it is memory-poisoning handler's job,
so drop the refcount grabbed by get_user_pages_fast

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 mm/madvise.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

Michal Hocko Oct. 18, 2019, 11:52 a.m. UTC | #1
On Thu 17-10-19 16:21:09, Oscar Salvador wrote:
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> 
> The call to get_user_pages_fast is only to get the pointer to a struct
> page of a given address, pinning it is memory-poisoning handler's job,
> so drop the refcount grabbed by get_user_pages_fast
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  mm/madvise.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 2be9f3fdb05e..89ed9a22ff4f 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -878,16 +878,24 @@ static int madvise_inject_error(int behavior,
>  		 */
>  		order = compound_order(compound_head(page));
>  
> -		if (PageHWPoison(page)) {
> -			put_page(page);
> +		/*
> +		 * The get_user_pages_fast() is just to get the pfn of the
> +		 * given address, and the refcount has nothing to do with
> +		 * what we try to test, so it should be released immediately.
> +		 * This is racy but it's intended because the real hardware
> +		 * errors could happen at any moment and memory error handlers
> +		 * must properly handle the race.
> +		 */
> +		put_page(page);
> +
> +		if (PageHWPoison(page))
>  			continue;
> -		}
>  
>  		if (behavior == MADV_SOFT_OFFLINE) {
>  			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
>  					pfn, start);
>  
> -			ret = soft_offline_page(page, MF_COUNT_INCREASED);
> +			ret = soft_offline_page(page, 0);

What does prevent this struct page to go away completely?

>  			if (ret)
>  				return ret;
>  			continue;
> @@ -895,14 +903,6 @@ static int madvise_inject_error(int behavior,
>  
>  		pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
>  				pfn, start);
> -
> -		/*
> -		 * Drop the page reference taken by get_user_pages_fast(). In
> -		 * the absence of MF_COUNT_INCREASED the memory_failure()
> -		 * routine is responsible for pinning the page to prevent it
> -		 * from being released back to the page allocator.
> -		 */
> -		put_page(page);
>  		ret = memory_failure(pfn, 0);
>  		if (ret)
>  			return ret;
> -- 
> 2.12.3
>
Naoya Horiguchi Oct. 21, 2019, 7:02 a.m. UTC | #2
On Fri, Oct 18, 2019 at 01:52:27PM +0200, Michal Hocko wrote:
> On Thu 17-10-19 16:21:09, Oscar Salvador wrote:
> > From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > 
> > The call to get_user_pages_fast is only to get the pointer to a struct
> > page of a given address, pinning it is memory-poisoning handler's job,
> > so drop the refcount grabbed by get_user_pages_fast
> > 
> > Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > Signed-off-by: Oscar Salvador <osalvador@suse.de>
> > ---
> >  mm/madvise.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 2be9f3fdb05e..89ed9a22ff4f 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -878,16 +878,24 @@ static int madvise_inject_error(int behavior,
> >  		 */
> >  		order = compound_order(compound_head(page));
> >  
> > -		if (PageHWPoison(page)) {
> > -			put_page(page);
> > +		/*
> > +		 * The get_user_pages_fast() is just to get the pfn of the
> > +		 * given address, and the refcount has nothing to do with
> > +		 * what we try to test, so it should be released immediately.
> > +		 * This is racy but it's intended because the real hardware
> > +		 * errors could happen at any moment and memory error handlers
> > +		 * must properly handle the race.
> > +		 */
> > +		put_page(page);
> > +
> > +		if (PageHWPoison(page))
> >  			continue;
> > -		}
> >  
> >  		if (behavior == MADV_SOFT_OFFLINE) {
> >  			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
> >  					pfn, start);
> >  
> > -			ret = soft_offline_page(page, MF_COUNT_INCREASED);
> > +			ret = soft_offline_page(page, 0);
> 
> What does prevent this struct page to go away completely?

Nothing does it.  Memory error handler tries to pin by itself and
then determines what state the page is in now.

Thanks,
Naoya Horiguchi

> 
> >  			if (ret)
> >  				return ret;
> >  			continue;
> > @@ -895,14 +903,6 @@ static int madvise_inject_error(int behavior,
> >  
> >  		pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
> >  				pfn, start);
> > -
> > -		/*
> > -		 * Drop the page reference taken by get_user_pages_fast(). In
> > -		 * the absence of MF_COUNT_INCREASED the memory_failure()
> > -		 * routine is responsible for pinning the page to prevent it
> > -		 * from being released back to the page allocator.
> > -		 */
> > -		put_page(page);
> >  		ret = memory_failure(pfn, 0);
> >  		if (ret)
> >  			return ret;
> > -- 
> > 2.12.3
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs
>
Michal Hocko Oct. 21, 2019, 12:20 p.m. UTC | #3
On Mon 21-10-19 07:02:55, Naoya Horiguchi wrote:
> On Fri, Oct 18, 2019 at 01:52:27PM +0200, Michal Hocko wrote:
> > On Thu 17-10-19 16:21:09, Oscar Salvador wrote:
> > > From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > > 
> > > The call to get_user_pages_fast is only to get the pointer to a struct
> > > page of a given address, pinning it is memory-poisoning handler's job,
> > > so drop the refcount grabbed by get_user_pages_fast
> > > 
> > > Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > > Signed-off-by: Oscar Salvador <osalvador@suse.de>
> > > ---
> > >  mm/madvise.c | 24 ++++++++++++------------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 2be9f3fdb05e..89ed9a22ff4f 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -878,16 +878,24 @@ static int madvise_inject_error(int behavior,
> > >  		 */
> > >  		order = compound_order(compound_head(page));
> > >  
> > > -		if (PageHWPoison(page)) {
> > > -			put_page(page);
> > > +		/*
> > > +		 * The get_user_pages_fast() is just to get the pfn of the
> > > +		 * given address, and the refcount has nothing to do with
> > > +		 * what we try to test, so it should be released immediately.
> > > +		 * This is racy but it's intended because the real hardware
> > > +		 * errors could happen at any moment and memory error handlers
> > > +		 * must properly handle the race.
> > > +		 */
> > > +		put_page(page);
> > > +
> > > +		if (PageHWPoison(page))
> > >  			continue;
> > > -		}
> > >  
> > >  		if (behavior == MADV_SOFT_OFFLINE) {
> > >  			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
> > >  					pfn, start);
> > >  
> > > -			ret = soft_offline_page(page, MF_COUNT_INCREASED);
> > > +			ret = soft_offline_page(page, 0);
> > 
> > What does prevent this struct page to go away completely?
> 
> Nothing does it.  Memory error handler tries to pin by itself and
> then determines what state the page is in now.

OK, but the page is not pinned by this context so it can go away at any
time, right? Or do I miss your point? Who would be the Error handler
context in this case?

Patch
diff mbox series

diff --git a/mm/madvise.c b/mm/madvise.c
index 2be9f3fdb05e..89ed9a22ff4f 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -878,16 +878,24 @@  static int madvise_inject_error(int behavior,
 		 */
 		order = compound_order(compound_head(page));
 
-		if (PageHWPoison(page)) {
-			put_page(page);
+		/*
+		 * The get_user_pages_fast() is just to get the pfn of the
+		 * given address, and the refcount has nothing to do with
+		 * what we try to test, so it should be released immediately.
+		 * This is racy but it's intended because the real hardware
+		 * errors could happen at any moment and memory error handlers
+		 * must properly handle the race.
+		 */
+		put_page(page);
+
+		if (PageHWPoison(page))
 			continue;
-		}
 
 		if (behavior == MADV_SOFT_OFFLINE) {
 			pr_info("Soft offlining pfn %#lx at process virtual address %#lx\n",
 					pfn, start);
 
-			ret = soft_offline_page(page, MF_COUNT_INCREASED);
+			ret = soft_offline_page(page, 0);
 			if (ret)
 				return ret;
 			continue;
@@ -895,14 +903,6 @@  static int madvise_inject_error(int behavior,
 
 		pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
 				pfn, start);
-
-		/*
-		 * Drop the page reference taken by get_user_pages_fast(). In
-		 * the absence of MF_COUNT_INCREASED the memory_failure()
-		 * routine is responsible for pinning the page to prevent it
-		 * from being released back to the page allocator.
-		 */
-		put_page(page);
 		ret = memory_failure(pfn, 0);
 		if (ret)
 			return ret;