Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752814AbcLGI6N (ORCPT ); Wed, 7 Dec 2016 03:58:13 -0500 Received: from mail-wj0-f195.google.com ([209.85.210.195]:35942 "EHLO mail-wj0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752400AbcLGI6M (ORCPT ); Wed, 7 Dec 2016 03:58:12 -0500 Date: Wed, 7 Dec 2016 09:58:09 +0100 From: Michal Hocko To: Vlastimil Babka Cc: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() Message-ID: <20161207085809.GD17136@dhcp22.suse.cz> References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2204 Lines: 67 On Wed 07-12-16 09:48:52, Vlastimil Babka wrote: > On 12/07/2016 09:43 AM, Michal Hocko wrote: > > On Tue 06-12-16 09:53:14, Xishi Qiu wrote: > >> A compiler could re-read "old_flags" from the memory location after reading > >> and calculation "flags" and passes a newer value into the cmpxchg making > >> the comparison succeed while it should actually fail. > >> > >> Signed-off-by: Xishi Qiu > >> Suggested-by: Christian Borntraeger > >> --- > >> mm/mmzone.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/mmzone.c b/mm/mmzone.c > >> index 5652be8..e0b698e 100644 > >> --- a/mm/mmzone.c > >> +++ b/mm/mmzone.c > >> @@ -102,7 +102,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid) > >> int last_cpupid; > >> > >> do { > >> - old_flags = flags = page->flags; > >> + old_flags = flags = READ_ONCE(page->flags); > >> last_cpupid = page_cpupid_last(page); > > > > what prevents compiler from doing? > > old_flags = READ_ONCE(page->flags); > > flags = READ_ONCE(page->flags); > > AFAIK, READ_ONCE tells the compiler that page->flags is volatile. It > can't read from volatile location more times than being told? But those are two different variables which we assign to so what prevents the compiler from applying READ_ONCE on each of them separately? Anyway, this could be addressed easily by diff --git a/mm/mmzone.c b/mm/mmzone.c index 5652be858e5e..b4e093dd24c1 100644 --- a/mm/mmzone.c +++ b/mm/mmzone.c @@ -102,10 +102,10 @@ int page_cpupid_xchg_last(struct page *page, int cpupid) int last_cpupid; do { - old_flags = flags = page->flags; + old_flags = READ_ONCE(page->flags); last_cpupid = page_cpupid_last(page); - flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); + flags = old_flags & ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); flags |= (cpupid & LAST_CPUPID_MASK) << LAST_CPUPID_PGSHIFT; } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)); > > Or this doesn't matter? > > I think it would matter. > > >> > >> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > >> -- > >> 1.8.3.1 > >> > > -- Michal Hocko SUSE Labs