linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Hugh Dickins <hughd@google.com>,
	Mel Gorman <mgorman@techsingularity.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Li Wang <liwang@redhat.com>,
	Alex Shi <alex.shi@linux.alibaba.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH] mm, page_alloc: capture page in task context only
Date: Mon, 15 Jun 2020 14:03:50 -0700 (PDT)	[thread overview]
Message-ID: <alpine.LSU.2.11.2006151337150.11413@eggly.anvils> (raw)
In-Reply-To: <01287865-153d-42e7-afd8-1178ec6bc5b9@suse.cz>

On Fri, 12 Jun 2020, Vlastimil Babka wrote:
> On 6/10/20 10:48 PM, Hugh Dickins wrote:
> > While stressing compaction, one run oopsed on NULL capc->cc in
> > __free_one_page()'s task_capc(zone): compact_zone_order() had been
> > interrupted, and a page was being freed in the return from interrupt.
> > 
> > Though you would not expect it from the source, both gccs I was using
> > (a 4.8.1 and a 7.5.0) had chosen to compile compact_zone_order() with
> > the ".cc = &cc" implemented by mov %rbx,-0xb0(%rbp) immediately before
> > callq compact_zone - long after the "current->capture_control = &capc".
> > An interrupt in between those finds capc->cc NULL (zeroed by an earlier
> > rep stos).
> 
> Ugh, nasty. Same here with gcc 10.

Thanks for checking, nice to know that I'm in good company :)

> 
> > This could presumably be fixed by a barrier() before setting
> > current->capture_control in compact_zone_order(); but would also need
> > more care on return from compact_zone(), in order not to risk leaking
> > a page captured by interrupt just before capture_control is reset.
> 
> I was hoping a WRITE_ONCE(current->capture_control) would be enough,
> but apparently it's not (I tried).

Right, I don't think volatiles themselves actually constitute barriers;
but I'd better keep quiet, I notice the READ_ONCE/WRITE_ONCE/data_race
industry has been busy recently, and I'm likely out-of-date and mistaken.

> 
> > Maybe that is the preferable fix, but I felt safer for task_capc() to
> > exclude the rather surprising possibility of capture at interrupt time.
> 
> > Fixes: 5e1f0f098b46 ("mm, compaction: capture a page under direct compaction")
> > Cc: stable@vger.kernel.org # 5.1+
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks, and to Mel for his.

> 
> But perhaps I would also make sure that we don't expose the half initialized
> capture_control and run into this problem again later. It's not like this is a
> fast path where barriers hurt. Something like this then? (with added comments)

Would it be very rude if I leave that to you and to Mel? to add, or
to replace mine if you wish - go ahead.  I can easily see that more
sophistication at the compact_zone_order() end may be preferable to
another test and branch inside __free_one_page() (and would task_capc()
be better with an "unlikely" in it?).

But it seems unnecessary to have a fix at both ends, and I'm rather too
wound up in other things at the moment, to want to read up on the current
state of such barriers, and sign off on the Vlastipatch below myself (but
I do notice that READ_ONCE seems to have more in it today than I remember,
which probably accounts for why you did not put the barrier() I expected
to see on the way out).

Hugh

> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index fd988b7e5f2b..c89e26817278 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2316,15 +2316,17 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
>  		.page = NULL,
>  	};
>  
> -	current->capture_control = &capc;
> +	barrier();
> +
> +	WRITE_ONCE(current->capture_control, &capc);
>  
>  	ret = compact_zone(&cc, &capc);
>  
>  	VM_BUG_ON(!list_empty(&cc.freepages));
>  	VM_BUG_ON(!list_empty(&cc.migratepages));
>  
> -	*capture = capc.page;
> -	current->capture_control = NULL;
> +	WRITE_ONCE(current->capture_control, NULL);
> +	*capture = READ_ONCE(capc.page);
>  
>  	return ret;
>  }

  reply	other threads:[~2020-06-15 21:04 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10 20:48 Hugh Dickins
2020-06-11 15:43 ` Mel Gorman
2020-06-12 10:30 ` Vlastimil Babka
2020-06-15 21:03   ` Hugh Dickins [this message]
2020-06-16  7:45     ` Vlastimil Babka
2020-06-16  8:26       ` [PATCH 1/2] mm, compaction: make capture control handling safe wrt interrupts Vlastimil Babka
2020-06-16  8:26         ` [PATCH 2/2] mm, page_alloc: use unlikely() in task_capc() Vlastimil Babka
2020-06-16 20:29           ` Hugh Dickins
2020-06-17  9:55             ` Vlastimil Babka
2020-06-22  8:58               ` Mel Gorman
2020-06-16 20:18         ` [PATCH 1/2] mm, compaction: make capture control handling safe wrt interrupts Hugh Dickins

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=alpine.LSU.2.11.2006151337150.11413@eggly.anvils \
    --to=hughd@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=alex.shi@linux.alibaba.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=liwang@redhat.com \
    --cc=mgorman@techsingularity.net \
    --cc=vbabka@suse.cz \
    --subject='Re: [PATCH] mm, page_alloc: capture page in task context only' \
    /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: link

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).