From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f197.google.com (mail-wj0-f197.google.com [209.85.210.197]) by kanga.kvack.org (Postfix) with ESMTP id ACDED6B0038 for ; Mon, 5 Dec 2016 03:31:27 -0500 (EST) Received: by mail-wj0-f197.google.com with SMTP id he10so19963769wjc.6 for ; Mon, 05 Dec 2016 00:31:27 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com. [148.163.158.5]) by mx.google.com with ESMTPS id p123si11849323wmg.154.2016.12.05.00.31.26 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Dec 2016 00:31:26 -0800 (PST) Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uB58TStu112469 for ; Mon, 5 Dec 2016 03:31:25 -0500 Received: from e35.co.us.ibm.com (e35.co.us.ibm.com [32.97.110.153]) by mx0b-001b2d01.pphosted.com with ESMTP id 2750kgf4bg-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 05 Dec 2016 03:31:24 -0500 Received: from localhost by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 5 Dec 2016 01:31:24 -0700 Subject: Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> From: Christian Borntraeger Date: Mon, 5 Dec 2016 09:31:18 +0100 MIME-Version: 1.0 In-Reply-To: <584523E4.9030600@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Message-Id: <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> Sender: owner-linux-mm@kvack.org List-ID: To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai Cc: Linux MM , LKML , Yisheng Xie On 12/05/2016 09:23 AM, Xishi Qiu wrote: > By reading the code, I find the following code maybe optimized by > compiler, maybe page->flags and old_flags use the same register, > so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. please use READ_ONCE instead of ACCESS_ONCE for future patches. > > Signed-off-by: Xishi Qiu > --- > 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 = ACCESS_ONCE(page->flags); > last_cpupid = page_cpupid_last(page); > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); I dont thing that this is actually a problem. The code below does } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)) and the cmpxchg should be an atomic op that should already take care of everything (page->flags is passed as a pointer). -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f71.google.com (mail-wm0-f71.google.com [74.125.82.71]) by kanga.kvack.org (Postfix) with ESMTP id 6199F6B025E for ; Mon, 5 Dec 2016 03:33:21 -0500 (EST) Received: by mail-wm0-f71.google.com with SMTP id g23so15433010wme.4 for ; Mon, 05 Dec 2016 00:33:21 -0800 (PST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com. [58.251.152.64]) by mx.google.com with ESMTPS id q31si6846408lfi.259.2016.12.05.00.33.18 for (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 05 Dec 2016 00:33:20 -0800 (PST) Message-ID: <584523E4.9030600@huawei.com> Date: Mon, 5 Dec 2016 16:23:00 +0800 From: Xishi Qiu MIME-Version: 1.0 Subject: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton , Mel Gorman , Yaowei Bai Cc: Linux MM , LKML , Yisheng Xie By reading the code, I find the following code maybe optimized by compiler, maybe page->flags and old_flags use the same register, so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. Signed-off-by: Xishi Qiu --- 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 = ACCESS_ONCE(page->flags); last_cpupid = page_cpupid_last(page); flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); -- 1.8.3.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f72.google.com (mail-wm0-f72.google.com [74.125.82.72]) by kanga.kvack.org (Postfix) with ESMTP id 57C006B0261 for ; Mon, 5 Dec 2016 03:50:12 -0500 (EST) Received: by mail-wm0-f72.google.com with SMTP id a20so15577982wme.5 for ; Mon, 05 Dec 2016 00:50:12 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com. [148.163.158.5]) by mx.google.com with ESMTPS id l66si11921397wma.114.2016.12.05.00.50.10 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Dec 2016 00:50:11 -0800 (PST) Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uB58mmif096778 for ; Mon, 5 Dec 2016 03:50:10 -0500 Received: from e35.co.us.ibm.com (e35.co.us.ibm.com [32.97.110.153]) by mx0b-001b2d01.pphosted.com with ESMTP id 27548h1xpy-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 05 Dec 2016 03:50:09 -0500 Received: from localhost by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 5 Dec 2016 01:50:09 -0700 Subject: Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> From: Christian Borntraeger Date: Mon, 5 Dec 2016 09:50:02 +0100 MIME-Version: 1.0 In-Reply-To: <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Message-Id: <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> Sender: owner-linux-mm@kvack.org List-ID: To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai Cc: Linux MM , LKML , Yisheng Xie On 12/05/2016 09:31 AM, Christian Borntraeger wrote: > On 12/05/2016 09:23 AM, Xishi Qiu wrote: >> By reading the code, I find the following code maybe optimized by >> compiler, maybe page->flags and old_flags use the same register, >> so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. > > please use READ_ONCE instead of ACCESS_ONCE for future patches. > >> >> Signed-off-by: Xishi Qiu >> --- >> 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 = ACCESS_ONCE(page->flags); >> last_cpupid = page_cpupid_last(page); >> >> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > > > I dont thing that this is actually a problem. The code below does > > } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)) > > and the cmpxchg should be an atomic op that should already take care of everything > (page->flags is passed as a pointer). > Reading the code again, you might be right, but I think your patch description is somewhat misleading. I think the problem is that old_flags and flags are not necessarily the same. So what about 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f197.google.com (mail-wj0-f197.google.com [209.85.210.197]) by kanga.kvack.org (Postfix) with ESMTP id 873686B025E for ; Mon, 5 Dec 2016 04:24:44 -0500 (EST) Received: by mail-wj0-f197.google.com with SMTP id xr1so61046522wjb.7 for ; Mon, 05 Dec 2016 01:24:44 -0800 (PST) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com. [119.145.14.66]) by mx.google.com with ESMTPS id n6si14201475wjk.207.2016.12.05.01.24.41 for (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 05 Dec 2016 01:24:43 -0800 (PST) Message-ID: <584531CF.9030204@huawei.com> Date: Mon, 5 Dec 2016 17:22:23 +0800 From: Xishi Qiu MIME-Version: 1.0 Subject: Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> In-Reply-To: <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Christian Borntraeger Cc: Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie On 2016/12/5 16:50, Christian Borntraeger wrote: > On 12/05/2016 09:31 AM, Christian Borntraeger wrote: >> On 12/05/2016 09:23 AM, Xishi Qiu wrote: >>> By reading the code, I find the following code maybe optimized by >>> compiler, maybe page->flags and old_flags use the same register, >>> so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. >> >> please use READ_ONCE instead of ACCESS_ONCE for future patches. >> >>> >>> Signed-off-by: Xishi Qiu >>> --- >>> 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 = ACCESS_ONCE(page->flags); >>> last_cpupid = page_cpupid_last(page); >>> >>> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); >> >> >> I dont thing that this is actually a problem. The code below does >> >> } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)) >> >> and the cmpxchg should be an atomic op that should already take care of everything >> (page->flags is passed as a pointer). >> > > Reading the code again, you might be right, but I think your patch description > is somewhat misleading. I think the problem is that old_flags and flags are > not necessarily the same. > > So what about > > 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. > Hi Christian, I'll resend v2, thanks! > > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f198.google.com (mail-wj0-f198.google.com [209.85.210.198]) by kanga.kvack.org (Postfix) with ESMTP id 3480B6B0038 for ; Mon, 5 Dec 2016 04:39:23 -0500 (EST) Received: by mail-wj0-f198.google.com with SMTP id xr1so61202896wjb.7 for ; Mon, 05 Dec 2016 01:39:23 -0800 (PST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com. [58.251.152.64]) by mx.google.com with ESMTP id xu5si14201109wjb.254.2016.12.05.01.39.20 for ; Mon, 05 Dec 2016 01:39:22 -0800 (PST) Message-ID: <584532DF.7080805@huawei.com> Date: Mon, 5 Dec 2016 17:26:55 +0800 From: Xishi Qiu MIME-Version: 1.0 Subject: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> In-Reply-To: <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Christian Borntraeger , Andrew Morton , Mel Gorman , Yaowei Bai Cc: Linux MM , LKML , Yisheng Xie 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 = ACCESS_ONCE(page->flags); last_cpupid = page_cpupid_last(page); flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); -- 1.8.3.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f70.google.com (mail-pg0-f70.google.com [74.125.83.70]) by kanga.kvack.org (Postfix) with ESMTP id B3E096B0038 for ; Mon, 5 Dec 2016 04:44:18 -0500 (EST) Received: by mail-pg0-f70.google.com with SMTP id x23so356785151pgx.6 for ; Mon, 05 Dec 2016 01:44:18 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (001b2d01.pphosted.com. [148.163.156.1]) by mx.google.com with ESMTPS id y189si14031865pgb.131.2016.12.05.01.44.17 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Dec 2016 01:44:18 -0800 (PST) Received: from pps.filterd (m0098396.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uB59e59u119921 for ; Mon, 5 Dec 2016 04:44:17 -0500 Received: from e18.ny.us.ibm.com (e18.ny.us.ibm.com [129.33.205.208]) by mx0a-001b2d01.pphosted.com with ESMTP id 2753tgn4wg-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 05 Dec 2016 04:44:17 -0500 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 5 Dec 2016 04:44:16 -0500 Subject: Re: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> <584532DF.7080805@huawei.com> From: Christian Borntraeger Date: Mon, 5 Dec 2016 10:44:09 +0100 MIME-Version: 1.0 In-Reply-To: <584532DF.7080805@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Message-Id: Sender: owner-linux-mm@kvack.org List-ID: To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai Cc: Linux MM , LKML , Yisheng Xie On 12/05/2016 10:26 AM, 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 = ACCESS_ONCE(page->flags); please use READ_ONCE. > last_cpupid = page_cpupid_last(page); > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f70.google.com (mail-oi0-f70.google.com [209.85.218.70]) by kanga.kvack.org (Postfix) with ESMTP id 4FBDA6B0069 for ; Mon, 5 Dec 2016 20:58:04 -0500 (EST) Received: by mail-oi0-f70.google.com with SMTP id l192so585970292oih.2 for ; Mon, 05 Dec 2016 17:58:04 -0800 (PST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com. [119.145.14.65]) by mx.google.com with ESMTPS id w21si8048277oia.24.2016.12.05.17.58.01 for (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 05 Dec 2016 17:58:03 -0800 (PST) Message-ID: <58461A0A.3070504@huawei.com> Date: Tue, 6 Dec 2016 09:53:14 +0800 From: Xishi Qiu MIME-Version: 1.0 Subject: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> In-Reply-To: <584523E4.9030600@huawei.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger Cc: Linux MM , LKML , Yisheng Xie 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); flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); -- 1.8.3.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f71.google.com (mail-wm0-f71.google.com [74.125.82.71]) by kanga.kvack.org (Postfix) with ESMTP id 7C1D56B0038 for ; Wed, 7 Dec 2016 03:39:48 -0500 (EST) Received: by mail-wm0-f71.google.com with SMTP id i131so34462538wmf.3 for ; Wed, 07 Dec 2016 00:39:48 -0800 (PST) Received: from mx2.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id v2si23443515wjh.194.2016.12.07.00.39.47 for (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 07 Dec 2016 00:39:47 -0800 (PST) Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> From: Vlastimil Babka Message-ID: <70a14036-a8e7-473f-3dc1-2517ffbe27e9@suse.cz> Date: Wed, 7 Dec 2016 09:39:32 +0100 MIME-Version: 1.0 In-Reply-To: <58461A0A.3070504@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger Cc: Linux MM , LKML , Yisheng Xie On 12/06/2016 02:53 AM, 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 Acked-by: Vlastimil Babka > --- > 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); > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f200.google.com (mail-wj0-f200.google.com [209.85.210.200]) by kanga.kvack.org (Postfix) with ESMTP id 6CBA46B0038 for ; Wed, 7 Dec 2016 03:43:08 -0500 (EST) Received: by mail-wj0-f200.google.com with SMTP id hb5so81068079wjc.2 for ; Wed, 07 Dec 2016 00:43:08 -0800 (PST) Received: from mail-wj0-f195.google.com (mail-wj0-f195.google.com. [209.85.210.195]) by mx.google.com with ESMTPS id lg5si23466545wjc.131.2016.12.07.00.43.07 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Dec 2016 00:43:07 -0800 (PST) Received: by mail-wj0-f195.google.com with SMTP id he10so34273868wjc.2 for ; Wed, 07 Dec 2016 00:43:07 -0800 (PST) Date: Wed, 7 Dec 2016 09:43:05 +0100 From: Michal Hocko Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() Message-ID: <20161207084305.GA20350@dhcp22.suse.cz> References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <58461A0A.3070504@huawei.com> Sender: owner-linux-mm@kvack.org List-ID: To: Xishi Qiu Cc: Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie 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); Or this doesn't matter? > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > -- > 1.8.3.1 > -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f199.google.com (mail-wj0-f199.google.com [209.85.210.199]) by kanga.kvack.org (Postfix) with ESMTP id EDF066B0038 for ; Wed, 7 Dec 2016 03:49:08 -0500 (EST) Received: by mail-wj0-f199.google.com with SMTP id hb5so81130848wjc.2 for ; Wed, 07 Dec 2016 00:49:08 -0800 (PST) Received: from mx2.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id xs6si23455578wjc.244.2016.12.07.00.49.07 for (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 07 Dec 2016 00:49:07 -0800 (PST) Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> From: Vlastimil Babka Message-ID: <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> Date: Wed, 7 Dec 2016 09:48:52 +0100 MIME-Version: 1.0 In-Reply-To: <20161207084305.GA20350@dhcp22.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko , Xishi Qiu Cc: Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie 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? > Or this doesn't matter? I think it would matter. >> >> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); >> -- >> 1.8.3.1 >> > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f72.google.com (mail-wm0-f72.google.com [74.125.82.72]) by kanga.kvack.org (Postfix) with ESMTP id AEADB6B0038 for ; Wed, 7 Dec 2016 03:58:12 -0500 (EST) Received: by mail-wm0-f72.google.com with SMTP id a20so34688787wme.5 for ; Wed, 07 Dec 2016 00:58:12 -0800 (PST) Received: from mail-wj0-f193.google.com (mail-wj0-f193.google.com. [209.85.210.193]) by mx.google.com with ESMTPS id k11si7432630wmb.125.2016.12.07.00.58.11 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Dec 2016 00:58:11 -0800 (PST) Received: by mail-wj0-f193.google.com with SMTP id kp2so48563001wjc.0 for ; Wed, 07 Dec 2016 00:58:11 -0800 (PST) Date: Wed, 7 Dec 2016 09:58:09 +0100 From: Michal Hocko 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> Sender: owner-linux-mm@kvack.org List-ID: To: Vlastimil Babka Cc: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f72.google.com (mail-wm0-f72.google.com [74.125.82.72]) by kanga.kvack.org (Postfix) with ESMTP id 87C536B0038 for ; Wed, 7 Dec 2016 04:30:02 -0500 (EST) Received: by mail-wm0-f72.google.com with SMTP id w13so34921872wmw.0 for ; Wed, 07 Dec 2016 01:30:02 -0800 (PST) Received: from mx2.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id n6si23587022wjk.207.2016.12.07.01.30.01 for (version=TLS1 cipher=AES128-SHA bits=128/128); Wed, 07 Dec 2016 01:30:01 -0800 (PST) Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> From: Vlastimil Babka Message-ID: Date: Wed, 7 Dec 2016 10:29:44 +0100 MIME-Version: 1.0 In-Reply-To: <20161207085809.GD17136@dhcp22.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie On 12/07/2016 09:58 AM, Michal Hocko wrote: > 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? I would naively expect that it's assigned to flags first, and then from flags to old_flags. But I don't know exactly the C standard evaluation rules that apply here. > Anyway, this could be addressed easily by Yes, that way there should be no doubt. > 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 >>>> >>> > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f197.google.com (mail-pf0-f197.google.com [209.85.192.197]) by kanga.kvack.org (Postfix) with ESMTP id A462A6B0038 for ; Wed, 7 Dec 2016 04:40:55 -0500 (EST) Received: by mail-pf0-f197.google.com with SMTP id c4so593237311pfb.7 for ; Wed, 07 Dec 2016 01:40:55 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (001b2d01.pphosted.com. [148.163.156.1]) by mx.google.com with ESMTPS id z3si23365727pfd.61.2016.12.07.01.40.54 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Dec 2016 01:40:54 -0800 (PST) Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uB79cgIU006468 for ; Wed, 7 Dec 2016 04:40:54 -0500 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0a-001b2d01.pphosted.com with ESMTP id 276arh9f5d-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 07 Dec 2016 04:40:54 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 7 Dec 2016 02:40:53 -0700 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> From: Christian Borntraeger Date: Wed, 7 Dec 2016 10:40:47 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Message-Id: Sender: owner-linux-mm@kvack.org List-ID: To: Vlastimil Babka , Michal Hocko Cc: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie On 12/07/2016 10:29 AM, Vlastimil Babka wrote: > On 12/07/2016 09:58 AM, Michal Hocko wrote: >> 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? > > I would naively expect that it's assigned to flags first, and then from > flags to old_flags. But I don't know exactly the C standard evaluation > rules that apply here. > >> Anyway, this could be addressed easily by > > Yes, that way there should be no doubt. That change would make it clearer, but the code is correct anyway, as assignments in C are done from right to left, so old_flags = flags = READ_ONCE(page->flags); is equivalent to flags = READ_ONCE(page->flags); old_flags = flags; > >> 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 >>>>> >>>> >> > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f200.google.com (mail-wj0-f200.google.com [209.85.210.200]) by kanga.kvack.org (Postfix) with ESMTP id 482BC6B0038 for ; Wed, 7 Dec 2016 04:59:47 -0500 (EST) Received: by mail-wj0-f200.google.com with SMTP id bk3so81832869wjc.4 for ; Wed, 07 Dec 2016 01:59:47 -0800 (PST) Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com. [74.125.82.67]) by mx.google.com with ESMTPS id y130si7651190wmc.29.2016.12.07.01.59.45 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Dec 2016 01:59:45 -0800 (PST) Received: by mail-wm0-f67.google.com with SMTP id g23so26770828wme.1 for ; Wed, 07 Dec 2016 01:59:45 -0800 (PST) Date: Wed, 7 Dec 2016 10:59:44 +0100 From: Michal Hocko Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() Message-ID: <20161207095943.GF17136@dhcp22.suse.cz> References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Christian Borntraeger Cc: Vlastimil Babka , Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie On Wed 07-12-16 10:40:47, Christian Borntraeger wrote: > On 12/07/2016 10:29 AM, Vlastimil Babka wrote: > > On 12/07/2016 09:58 AM, Michal Hocko wrote: > >> 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? > > > > I would naively expect that it's assigned to flags first, and then from > > flags to old_flags. But I don't know exactly the C standard evaluation > > rules that apply here. > > > >> Anyway, this could be addressed easily by > > > > Yes, that way there should be no doubt. > > That change would make it clearer, but the code is correct anyway, > as assignments in C are done from right to left, so > old_flags = flags = READ_ONCE(page->flags); > > is equivalent to > > flags = READ_ONCE(page->flags); > old_flags = flags; OK, I guess you are right. For some reason I thought that the compiler is free to bypass flags and split an assignment a = b = c; into b = c; a = c which would still follow from right to left rule. I guess I am over speculating here though, so sorry for the noise. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f198.google.com (mail-pf0-f198.google.com [209.85.192.198]) by kanga.kvack.org (Postfix) with ESMTP id D442B6B0038 for ; Wed, 7 Dec 2016 05:03:36 -0500 (EST) Received: by mail-pf0-f198.google.com with SMTP id 17so594923696pfy.2 for ; Wed, 07 Dec 2016 02:03:36 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (001b2d01.pphosted.com. [148.163.156.1]) by mx.google.com with ESMTPS id 7si23410659pgh.30.2016.12.07.02.03.35 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Dec 2016 02:03:36 -0800 (PST) Received: from pps.filterd (m0098404.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uB7A0eY5114964 for ; Wed, 7 Dec 2016 05:03:35 -0500 Received: from e37.co.us.ibm.com (e37.co.us.ibm.com [32.97.110.158]) by mx0a-001b2d01.pphosted.com with ESMTP id 276exh3q4f-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 07 Dec 2016 05:03:35 -0500 Received: from localhost by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 7 Dec 2016 03:03:34 -0700 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> <20161207095943.GF17136@dhcp22.suse.cz> From: Christian Borntraeger Date: Wed, 7 Dec 2016 11:03:29 +0100 MIME-Version: 1.0 In-Reply-To: <20161207095943.GF17136@dhcp22.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Message-Id: <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com> Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: Vlastimil Babka , Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie On 12/07/2016 10:59 AM, Michal Hocko wrote: > On Wed 07-12-16 10:40:47, Christian Borntraeger wrote: >> On 12/07/2016 10:29 AM, Vlastimil Babka wrote: >>> On 12/07/2016 09:58 AM, Michal Hocko wrote: >>>> 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? >>> >>> I would naively expect that it's assigned to flags first, and then from >>> flags to old_flags. But I don't know exactly the C standard evaluation >>> rules that apply here. >>> >>>> Anyway, this could be addressed easily by >>> >>> Yes, that way there should be no doubt. >> >> That change would make it clearer, but the code is correct anyway, >> as assignments in C are done from right to left, so >> old_flags = flags = READ_ONCE(page->flags); >> >> is equivalent to >> >> flags = READ_ONCE(page->flags); >> old_flags = flags; > > OK, I guess you are right. For some reason I thought that the compiler > is free to bypass flags and split an assignment > a = b = c; into b = c; a = c > which would still follow from right to left rule. I guess I am over > speculating here though, so sorry for the noise. Hmmm, just rereading C, I am no longer sure... I cannot find anything right now, that adds a sequence point in here. Still looking... -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f70.google.com (mail-wm0-f70.google.com [74.125.82.70]) by kanga.kvack.org (Postfix) with ESMTP id BB2EA6B0253 for ; Wed, 7 Dec 2016 17:16:58 -0500 (EST) Received: by mail-wm0-f70.google.com with SMTP id i131so41940036wmf.3 for ; Wed, 07 Dec 2016 14:16:58 -0800 (PST) Received: from mail-wj0-x232.google.com (mail-wj0-x232.google.com. [2a00:1450:400c:c01::232]) by mx.google.com with ESMTPS id z3si26316130wjt.212.2016.12.07.14.16.57 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Dec 2016 14:16:57 -0800 (PST) Received: by mail-wj0-x232.google.com with SMTP id tg4so118505197wjb.1 for ; Wed, 07 Dec 2016 14:16:57 -0800 (PST) From: Rasmus Villemoes Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> <20161207095943.GF17136@dhcp22.suse.cz> <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com> Date: Wed, 07 Dec 2016 23:16:55 +0100 In-Reply-To: <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com> (Christian Borntraeger's message of "Wed, 7 Dec 2016 11:03:29 +0100") Message-ID: <877f7bqt9k.fsf@rasmusvillemoes.dk> MIME-Version: 1.0 Content-Type: text/plain Sender: owner-linux-mm@kvack.org List-ID: To: Christian Borntraeger Cc: Michal Hocko , Vlastimil Babka , Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie On Wed, Dec 07 2016, Christian Borntraeger wrote: > On 12/07/2016 10:59 AM, Michal Hocko wrote: >> On Wed 07-12-16 10:40:47, Christian Borntraeger wrote: >>> On 12/07/2016 10:29 AM, Vlastimil Babka wrote: >>>> On 12/07/2016 09:58 AM, Michal Hocko wrote: >>>>> On Wed 07-12-16 09:48:52, Vlastimil Babka wrote: >>>>> Anyway, this could be addressed easily by >>>> >>>> Yes, that way there should be no doubt. >>> >>> That change would make it clearer, but the code is correct anyway, >>> as assignments in C are done from right to left, so >>> old_flags = flags = READ_ONCE(page->flags); >>> >>> is equivalent to >>> >>> flags = READ_ONCE(page->flags); >>> old_flags = flags; >> >> OK, I guess you are right. For some reason I thought that the compiler >> is free to bypass flags and split an assignment >> a = b = c; into b = c; a = c >> which would still follow from right to left rule. I guess I am over >> speculating here though, so sorry for the noise. > > Hmmm, just rereading C, I am no longer sure... > I cannot find anything right now, that adds a sequence point in here. > Still looking... C99 6.5.16.3: ... An assignment expression has the value of the left operand after the assignment, .... So if the expression c can have side effects or is for any reason (e.g. volatile) not guaranteed to produce the same value if it's evaluated again, there's no way the compiler would be allowed to change a=b=c; into b=c; a=c;. (Also, this means that in "int a, c = 256; char b; a=b=c;", a ends up with the value 0.) Somewhat related: https://lwn.net/Articles/233902/ -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751510AbcLEIZS (ORCPT ); Mon, 5 Dec 2016 03:25:18 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:29319 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751161AbcLEIZR (ORCPT ); Mon, 5 Dec 2016 03:25:17 -0500 Message-ID: <584523E4.9030600@huawei.com> Date: Mon, 5 Dec 2016 16:23:00 +0800 From: Xishi Qiu User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Andrew Morton , Mel Gorman , Yaowei Bai CC: Linux MM , LKML , Yisheng Xie Subject: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.25.179] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org By reading the code, I find the following code maybe optimized by compiler, maybe page->flags and old_flags use the same register, so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. Signed-off-by: Xishi Qiu --- 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 = ACCESS_ONCE(page->flags); last_cpupid = page_cpupid_last(page); flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); -- 1.8.3.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751494AbcLEIb1 (ORCPT ); Mon, 5 Dec 2016 03:31:27 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:38908 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751039AbcLEIbZ (ORCPT ); Mon, 5 Dec 2016 03:31:25 -0500 Subject: Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai References: <584523E4.9030600@huawei.com> Cc: Linux MM , LKML , Yisheng Xie From: Christian Borntraeger Date: Mon, 5 Dec 2016 09:31:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <584523E4.9030600@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16120508-0012-0000-0000-0000114E3573 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006192; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000193; SDB=6.00789478; UDB=6.00382216; IPR=6.00567218; BA=6.00004938; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013533; XFM=3.00000011; UTC=2016-12-05 08:31:23 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16120508-0013-0000-0000-000047BC0FCD Message-Id: <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-12-05_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1612050154 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/05/2016 09:23 AM, Xishi Qiu wrote: > By reading the code, I find the following code maybe optimized by > compiler, maybe page->flags and old_flags use the same register, > so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. please use READ_ONCE instead of ACCESS_ONCE for future patches. > > Signed-off-by: Xishi Qiu > --- > 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 = ACCESS_ONCE(page->flags); > last_cpupid = page_cpupid_last(page); > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); I dont thing that this is actually a problem. The code below does } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)) and the cmpxchg should be an atomic op that should already take care of everything (page->flags is passed as a pointer). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751497AbcLEIuk (ORCPT ); Mon, 5 Dec 2016 03:50:40 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:52569 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750928AbcLEIud (ORCPT ); Mon, 5 Dec 2016 03:50:33 -0500 Subject: Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> Cc: Linux MM , LKML , Yisheng Xie From: Christian Borntraeger Date: Mon, 5 Dec 2016 09:50:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16120508-0012-0000-0000-0000114E399A X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006197; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000193; SDB=6.00789485; UDB=6.00382219; IPR=6.00567224; BA=6.00004938; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013538; XFM=3.00000011; UTC=2016-12-05 08:50:07 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16120508-0013-0000-0000-000047BC1F12 Message-Id: <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-12-05_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1612050159 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/05/2016 09:31 AM, Christian Borntraeger wrote: > On 12/05/2016 09:23 AM, Xishi Qiu wrote: >> By reading the code, I find the following code maybe optimized by >> compiler, maybe page->flags and old_flags use the same register, >> so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. > > please use READ_ONCE instead of ACCESS_ONCE for future patches. > >> >> Signed-off-by: Xishi Qiu >> --- >> 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 = ACCESS_ONCE(page->flags); >> last_cpupid = page_cpupid_last(page); >> >> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > > > I dont thing that this is actually a problem. The code below does > > } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)) > > and the cmpxchg should be an atomic op that should already take care of everything > (page->flags is passed as a pointer). > Reading the code again, you might be right, but I think your patch description is somewhat misleading. I think the problem is that old_flags and flags are not necessarily the same. So what about 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752237AbcLEJXv (ORCPT ); Mon, 5 Dec 2016 04:23:51 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:30640 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751955AbcLEJXT (ORCPT ); Mon, 5 Dec 2016 04:23:19 -0500 Message-ID: <584531CF.9030204@huawei.com> Date: Mon, 5 Dec 2016 17:22:23 +0800 From: Xishi Qiu User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Christian Borntraeger CC: Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , "Yisheng Xie" Subject: Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> In-Reply-To: <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.25.179] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/12/5 16:50, Christian Borntraeger wrote: > On 12/05/2016 09:31 AM, Christian Borntraeger wrote: >> On 12/05/2016 09:23 AM, Xishi Qiu wrote: >>> By reading the code, I find the following code maybe optimized by >>> compiler, maybe page->flags and old_flags use the same register, >>> so use ACCESS_ONCE in page_cpupid_xchg_last() to fix the problem. >> >> please use READ_ONCE instead of ACCESS_ONCE for future patches. >> >>> >>> Signed-off-by: Xishi Qiu >>> --- >>> 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 = ACCESS_ONCE(page->flags); >>> last_cpupid = page_cpupid_last(page); >>> >>> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); >> >> >> I dont thing that this is actually a problem. The code below does >> >> } while (unlikely(cmpxchg(&page->flags, old_flags, flags) != old_flags)) >> >> and the cmpxchg should be an atomic op that should already take care of everything >> (page->flags is passed as a pointer). >> > > Reading the code again, you might be right, but I think your patch description > is somewhat misleading. I think the problem is that old_flags and flags are > not necessarily the same. > > So what about > > 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. > Hi Christian, I'll resend v2, thanks! > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752295AbcLEJ1w (ORCPT ); Mon, 5 Dec 2016 04:27:52 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:16619 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751372AbcLEJ1b (ORCPT ); Mon, 5 Dec 2016 04:27:31 -0500 Message-ID: <584532DF.7080805@huawei.com> Date: Mon, 5 Dec 2016 17:26:55 +0800 From: Xishi Qiu User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Christian Borntraeger , Andrew Morton , Mel Gorman , "Yaowei Bai" CC: Linux MM , LKML , Yisheng Xie Subject: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> In-Reply-To: <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.25.179] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 = ACCESS_ONCE(page->flags); last_cpupid = page_cpupid_last(page); flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); -- 1.8.3.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752263AbcLEJoZ (ORCPT ); Mon, 5 Dec 2016 04:44:25 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:58551 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751244AbcLEJoR (ORCPT ); Mon, 5 Dec 2016 04:44:17 -0500 Subject: Re: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai References: <584523E4.9030600@huawei.com> <26c66f28-d836-4d6e-fb40-3e2189a540ed@de.ibm.com> <0cc3c2bb-e292-2d7b-8d44-16c8e6c19899@de.ibm.com> <584532DF.7080805@huawei.com> Cc: Linux MM , LKML , Yisheng Xie From: Christian Borntraeger Date: Mon, 5 Dec 2016 10:44:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <584532DF.7080805@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16120509-0044-0000-0000-000001F08DED X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006197; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000194; SDB=6.00789502; UDB=6.00382230; IPR=6.00567242; BA=6.00004938; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013539; XFM=3.00000011; UTC=2016-12-05 09:44:14 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16120509-0045-0000-0000-0000061C98D4 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-12-05_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1612050173 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/05/2016 10:26 AM, 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 = ACCESS_ONCE(page->flags); please use READ_ONCE. > last_cpupid = page_cpupid_last(page); > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752236AbcLFBxj (ORCPT ); Mon, 5 Dec 2016 20:53:39 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:38759 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751515AbcLFBxg (ORCPT ); Mon, 5 Dec 2016 20:53:36 -0500 Message-ID: <58461A0A.3070504@huawei.com> Date: Tue, 6 Dec 2016 09:53:14 +0800 From: Xishi Qiu User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger CC: Linux MM , LKML , Yisheng Xie Subject: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() References: <584523E4.9030600@huawei.com> In-Reply-To: <584523E4.9030600@huawei.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.25.179] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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); flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); -- 1.8.3.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752629AbcLGIjt (ORCPT ); Wed, 7 Dec 2016 03:39:49 -0500 Received: from mx2.suse.de ([195.135.220.15]:34101 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752110AbcLGIjs (ORCPT ); Wed, 7 Dec 2016 03:39:48 -0500 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() To: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> Cc: Linux MM , LKML , Yisheng Xie From: Vlastimil Babka Message-ID: <70a14036-a8e7-473f-3dc1-2517ffbe27e9@suse.cz> Date: Wed, 7 Dec 2016 09:39:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <58461A0A.3070504@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/06/2016 02:53 AM, 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 Acked-by: Vlastimil Babka > --- > 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); > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752780AbcLGInd (ORCPT ); Wed, 7 Dec 2016 03:43:33 -0500 Received: from mail-wj0-f196.google.com ([209.85.210.196]:34412 "EHLO mail-wj0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752418AbcLGIn3 (ORCPT ); Wed, 7 Dec 2016 03:43:29 -0500 Date: Wed, 7 Dec 2016 09:43:05 +0100 From: Michal Hocko To: Xishi Qiu Cc: 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: <20161207084305.GA20350@dhcp22.suse.cz> References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <58461A0A.3070504@huawei.com> 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 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); Or this doesn't matter? > > flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); > -- > 1.8.3.1 > -- Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752575AbcLGItK (ORCPT ); Wed, 7 Dec 2016 03:49:10 -0500 Received: from mx2.suse.de ([195.135.220.15]:34961 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752243AbcLGItJ (ORCPT ); Wed, 7 Dec 2016 03:49:09 -0500 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() To: Michal Hocko , Xishi Qiu References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> Cc: Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie From: Vlastimil Babka Message-ID: <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> Date: Wed, 7 Dec 2016 09:48:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <20161207084305.GA20350@dhcp22.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? > Or this doesn't matter? I think it would matter. >> >> flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT); >> -- >> 1.8.3.1 >> > From mboxrd@z Thu Jan 1 00:00:00 1970 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 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752466AbcLGJaO (ORCPT ); Wed, 7 Dec 2016 04:30:14 -0500 Received: from mx2.suse.de ([195.135.220.15]:39117 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751792AbcLGJaM (ORCPT ); Wed, 7 Dec 2016 04:30:12 -0500 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() To: Michal Hocko References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> Cc: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Christian Borntraeger , Linux MM , LKML , Yisheng Xie From: Vlastimil Babka Message-ID: Date: Wed, 7 Dec 2016 10:29:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <20161207085809.GD17136@dhcp22.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/07/2016 09:58 AM, Michal Hocko wrote: > 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? I would naively expect that it's assigned to flags first, and then from flags to old_flags. But I don't know exactly the C standard evaluation rules that apply here. > Anyway, this could be addressed easily by Yes, that way there should be no doubt. > 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 >>>> >>> > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752644AbcLGJlL (ORCPT ); Wed, 7 Dec 2016 04:41:11 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:56960 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750813AbcLGJlK (ORCPT ); Wed, 7 Dec 2016 04:41:10 -0500 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() To: Vlastimil Babka , Michal Hocko References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> Cc: Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie From: Christian Borntraeger Date: Wed, 7 Dec 2016 10:40:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16120709-0020-0000-0000-00000A7397B8 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006208; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000194; SDB=6.00790462; UDB=6.00382806; IPR=6.00568201; BA=6.00004950; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013563; XFM=3.00000011; UTC=2016-12-07 09:40:52 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16120709-0021-0000-0000-000057E44F93 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-12-07_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1612070167 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/07/2016 10:29 AM, Vlastimil Babka wrote: > On 12/07/2016 09:58 AM, Michal Hocko wrote: >> 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? > > I would naively expect that it's assigned to flags first, and then from > flags to old_flags. But I don't know exactly the C standard evaluation > rules that apply here. > >> Anyway, this could be addressed easily by > > Yes, that way there should be no doubt. That change would make it clearer, but the code is correct anyway, as assignments in C are done from right to left, so old_flags = flags = READ_ONCE(page->flags); is equivalent to flags = READ_ONCE(page->flags); old_flags = flags; > >> 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 >>>>> >>>> >> > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753124AbcLGJ7s (ORCPT ); Wed, 7 Dec 2016 04:59:48 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:36681 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751512AbcLGJ7r (ORCPT ); Wed, 7 Dec 2016 04:59:47 -0500 Date: Wed, 7 Dec 2016 10:59:44 +0100 From: Michal Hocko To: Christian Borntraeger Cc: Vlastimil Babka , Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() Message-ID: <20161207095943.GF17136@dhcp22.suse.cz> References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 On Wed 07-12-16 10:40:47, Christian Borntraeger wrote: > On 12/07/2016 10:29 AM, Vlastimil Babka wrote: > > On 12/07/2016 09:58 AM, Michal Hocko wrote: > >> 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? > > > > I would naively expect that it's assigned to flags first, and then from > > flags to old_flags. But I don't know exactly the C standard evaluation > > rules that apply here. > > > >> Anyway, this could be addressed easily by > > > > Yes, that way there should be no doubt. > > That change would make it clearer, but the code is correct anyway, > as assignments in C are done from right to left, so > old_flags = flags = READ_ONCE(page->flags); > > is equivalent to > > flags = READ_ONCE(page->flags); > old_flags = flags; OK, I guess you are right. For some reason I thought that the compiler is free to bypass flags and split an assignment a = b = c; into b = c; a = c which would still follow from right to left rule. I guess I am over speculating here though, so sorry for the noise. -- Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753182AbcLGKDh (ORCPT ); Wed, 7 Dec 2016 05:03:37 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:45099 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751798AbcLGKDg (ORCPT ); Wed, 7 Dec 2016 05:03:36 -0500 Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() To: Michal Hocko References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> <20161207095943.GF17136@dhcp22.suse.cz> Cc: Vlastimil Babka , Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie From: Christian Borntraeger Date: Wed, 7 Dec 2016 11:03:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161207095943.GF17136@dhcp22.suse.cz> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16120710-0024-0000-0000-00001536D1A9 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006208; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000194; SDB=6.00790469; UDB=6.00382811; IPR=6.00568208; BA=6.00004950; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013563; XFM=3.00000011; UTC=2016-12-07 10:03:34 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16120710-0025-0000-0000-000046C29D5E Message-Id: <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-12-07_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1612070173 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/07/2016 10:59 AM, Michal Hocko wrote: > On Wed 07-12-16 10:40:47, Christian Borntraeger wrote: >> On 12/07/2016 10:29 AM, Vlastimil Babka wrote: >>> On 12/07/2016 09:58 AM, Michal Hocko wrote: >>>> 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? >>> >>> I would naively expect that it's assigned to flags first, and then from >>> flags to old_flags. But I don't know exactly the C standard evaluation >>> rules that apply here. >>> >>>> Anyway, this could be addressed easily by >>> >>> Yes, that way there should be no doubt. >> >> That change would make it clearer, but the code is correct anyway, >> as assignments in C are done from right to left, so >> old_flags = flags = READ_ONCE(page->flags); >> >> is equivalent to >> >> flags = READ_ONCE(page->flags); >> old_flags = flags; > > OK, I guess you are right. For some reason I thought that the compiler > is free to bypass flags and split an assignment > a = b = c; into b = c; a = c > which would still follow from right to left rule. I guess I am over > speculating here though, so sorry for the noise. Hmmm, just rereading C, I am no longer sure... I cannot find anything right now, that adds a sequence point in here. Still looking... From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933258AbcLGWRA (ORCPT ); Wed, 7 Dec 2016 17:17:00 -0500 Received: from mail-wj0-f175.google.com ([209.85.210.175]:33094 "EHLO mail-wj0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932339AbcLGWQ6 (ORCPT ); Wed, 7 Dec 2016 17:16:58 -0500 From: Rasmus Villemoes To: Christian Borntraeger Cc: Michal Hocko , Vlastimil Babka , Xishi Qiu , Andrew Morton , Mel Gorman , Yaowei Bai , Linux MM , LKML , Yisheng Xie Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() Organization: D03 References: <584523E4.9030600@huawei.com> <58461A0A.3070504@huawei.com> <20161207084305.GA20350@dhcp22.suse.cz> <7b74a021-e472-a21e-7936-6741e07906b5@suse.cz> <20161207085809.GD17136@dhcp22.suse.cz> <20161207095943.GF17136@dhcp22.suse.cz> <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com> X-Hashcash: 1:20:161207:borntraeger@de.ibm.com::aKoP0v5xHvsx/Iek:0000000000000000000000000000000000000000el+ X-Hashcash: 1:20:161207:xieyisheng1@huawei.com::wo6Mv5tt0s5lGSfh:0000000000000000000000000000000000000001iVd X-Hashcash: 1:20:161207:vbabka@suse.cz::3TaDJ8PKRk/LoR4k:0001upE X-Hashcash: 1:20:161207:baiyaowei@cmss.chinamobile.com::CkbCZJN6L73Me++n:00000000000000000000000000000001wlV X-Hashcash: 1:20:161207:linux-kernel@vger.kernel.org::Z30bKPxWyLpXAXpQ:0000000000000000000000000000000001XR+ X-Hashcash: 1:20:161207:akpm@linux-foundation.org::fhmMeUiOc4ZcrmZF:0000000000000000000000000000000000001lkl X-Hashcash: 1:20:161207:mgorman@techsingularity.net::jzcLiulqGQK/IwZT:000000000000000000000000000000000020os X-Hashcash: 1:20:161207:mhocko@kernel.org::32bYFs7KL4JkRvK2:000000000000000000000000000000000000000000003gPZ X-Hashcash: 1:20:161207:qiuxishi@huawei.com::FYIrcAfH0wIR/gVV:0000000000000000000000000000000000000000002yGE X-Hashcash: 1:20:161207:linux-mm@kvack.org::JWWijUxBx1BnQ+7w:0000000000000000000000000000000000000000000DUqt Date: Wed, 07 Dec 2016 23:16:55 +0100 In-Reply-To: <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com> (Christian Borntraeger's message of "Wed, 7 Dec 2016 11:03:29 +0100") Message-ID: <877f7bqt9k.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 07 2016, Christian Borntraeger wrote: > On 12/07/2016 10:59 AM, Michal Hocko wrote: >> On Wed 07-12-16 10:40:47, Christian Borntraeger wrote: >>> On 12/07/2016 10:29 AM, Vlastimil Babka wrote: >>>> On 12/07/2016 09:58 AM, Michal Hocko wrote: >>>>> On Wed 07-12-16 09:48:52, Vlastimil Babka wrote: >>>>> Anyway, this could be addressed easily by >>>> >>>> Yes, that way there should be no doubt. >>> >>> That change would make it clearer, but the code is correct anyway, >>> as assignments in C are done from right to left, so >>> old_flags = flags = READ_ONCE(page->flags); >>> >>> is equivalent to >>> >>> flags = READ_ONCE(page->flags); >>> old_flags = flags; >> >> OK, I guess you are right. For some reason I thought that the compiler >> is free to bypass flags and split an assignment >> a = b = c; into b = c; a = c >> which would still follow from right to left rule. I guess I am over >> speculating here though, so sorry for the noise. > > Hmmm, just rereading C, I am no longer sure... > I cannot find anything right now, that adds a sequence point in here. > Still looking... C99 6.5.16.3: ... An assignment expression has the value of the left operand after the assignment, .... So if the expression c can have side effects or is for any reason (e.g. volatile) not guaranteed to produce the same value if it's evaluated again, there's no way the compiler would be allowed to change a=b=c; into b=c; a=c;. (Also, this means that in "int a, c = 256; char b; a=b=c;", a ends up with the value 0.) Somewhat related: https://lwn.net/Articles/233902/