* [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 8:23 ` Xishi Qiu 0 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-05 8:23 UTC (permalink / raw) 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 <qiuxishi@huawei.com> --- 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 8:23 ` Xishi Qiu 0 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-05 8:23 UTC (permalink / raw) 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 <qiuxishi@huawei.com> --- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() 2016-12-05 8:23 ` Xishi Qiu @ 2016-12-05 8:31 ` Christian Borntraeger -1 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-05 8:31 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > --- > 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 8:31 ` Christian Borntraeger 0 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-05 8:31 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > --- > 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). ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() 2016-12-05 8:31 ` Christian Borntraeger @ 2016-12-05 8:50 ` Christian Borntraeger -1 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-05 8:50 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >> --- >> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 8:50 ` Christian Borntraeger 0 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-05 8:50 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >> --- >> 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() 2016-12-05 8:50 ` Christian Borntraeger @ 2016-12-05 9:22 ` Xishi Qiu -1 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-05 9:22 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>> --- >>> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 9:22 ` Xishi Qiu 0 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-05 9:22 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>> --- >>> 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! > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() 2016-12-05 8:50 ` Christian Borntraeger @ 2016-12-05 9:26 ` Xishi Qiu -1 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-05 9:26 UTC (permalink / raw) 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 <qiuxishi@huawei.com> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> --- 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 9:26 ` Xishi Qiu 0 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-05 9:26 UTC (permalink / raw) 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 <qiuxishi@huawei.com> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> --- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() 2016-12-05 9:26 ` Xishi Qiu @ 2016-12-05 9:44 ` Christian Borntraeger -1 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-05 9:44 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last() @ 2016-12-05 9:44 ` Christian Borntraeger 0 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-05 9:44 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-05 8:23 ` Xishi Qiu @ 2016-12-06 1:53 ` Xishi Qiu -1 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-06 1:53 UTC (permalink / raw) 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 <qiuxishi@huawei.com> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> --- 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-06 1:53 ` Xishi Qiu 0 siblings, 0 replies; 32+ messages in thread From: Xishi Qiu @ 2016-12-06 1:53 UTC (permalink / raw) 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 <qiuxishi@huawei.com> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> --- 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-06 1:53 ` Xishi Qiu @ 2016-12-07 8:39 ` Vlastimil Babka -1 siblings, 0 replies; 32+ messages in thread From: Vlastimil Babka @ 2016-12-07 8:39 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 8:39 ` Vlastimil Babka 0 siblings, 0 replies; 32+ messages in thread From: Vlastimil Babka @ 2016-12-07 8:39 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > 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); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-06 1:53 ` Xishi Qiu @ 2016-12-07 8:43 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-07 8:43 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 8:43 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-07 8:43 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 8:43 ` Michal Hocko @ 2016-12-07 8:48 ` Vlastimil Babka -1 siblings, 0 replies; 32+ messages in thread From: Vlastimil Babka @ 2016-12-07 8:48 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 8:48 ` Vlastimil Babka 0 siblings, 0 replies; 32+ messages in thread From: Vlastimil Babka @ 2016-12-07 8:48 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> 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 >> > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 8:48 ` Vlastimil Babka @ 2016-12-07 8:58 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-07 8:58 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > >> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 8:58 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-07 8:58 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > >> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 8:58 ` Michal Hocko @ 2016-12-07 9:29 ` Vlastimil Babka -1 siblings, 0 replies; 32+ messages in thread From: Vlastimil Babka @ 2016-12-07 9:29 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 9:29 ` Vlastimil Babka 0 siblings, 0 replies; 32+ messages in thread From: Vlastimil Babka @ 2016-12-07 9:29 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> 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 >>>> >>> > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 9:29 ` Vlastimil Babka @ 2016-12-07 9:40 ` Christian Borntraeger -1 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-07 9:40 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>> --- >>>>> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 9:40 ` Christian Borntraeger 0 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-07 9:40 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>> --- >>>>> 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 >>>>> >>>> >> > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 9:40 ` Christian Borntraeger @ 2016-12-07 9:59 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-07 9:59 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > >>>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > >>>>> --- > >>>>> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 9:59 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-07 9:59 UTC (permalink / raw) 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 <qiuxishi@huawei.com> > >>>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> > >>>>> --- > >>>>> 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 9:59 ` Michal Hocko @ 2016-12-07 10:03 ` Christian Borntraeger -1 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-07 10:03 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>>>>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>>>> --- >>>>>>> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 10:03 ` Christian Borntraeger 0 siblings, 0 replies; 32+ messages in thread From: Christian Borntraeger @ 2016-12-07 10:03 UTC (permalink / raw) 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 <qiuxishi@huawei.com> >>>>>>> Suggested-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>>>>> --- >>>>>>> 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... ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() 2016-12-07 10:03 ` Christian Borntraeger @ 2016-12-07 22:16 ` Rasmus Villemoes -1 siblings, 0 replies; 32+ messages in thread From: Rasmus Villemoes @ 2016-12-07 22:16 UTC (permalink / raw) 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 <borntraeger@de.ibm.com> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last() @ 2016-12-07 22:16 ` Rasmus Villemoes 0 siblings, 0 replies; 32+ messages in thread From: Rasmus Villemoes @ 2016-12-07 22:16 UTC (permalink / raw) 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 <borntraeger@de.ibm.com> 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/ ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2016-12-07 22:17 UTC | newest] Thread overview: 32+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-05 8:23 [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last() Xishi Qiu 2016-12-05 8:23 ` Xishi Qiu 2016-12-05 8:31 ` Christian Borntraeger 2016-12-05 8:31 ` Christian Borntraeger 2016-12-05 8:50 ` Christian Borntraeger 2016-12-05 8:50 ` Christian Borntraeger 2016-12-05 9:22 ` Xishi Qiu 2016-12-05 9:22 ` Xishi Qiu 2016-12-05 9:26 ` [RFC PATCH v2] " Xishi Qiu 2016-12-05 9:26 ` Xishi Qiu 2016-12-05 9:44 ` Christian Borntraeger 2016-12-05 9:44 ` Christian Borntraeger 2016-12-06 1:53 ` [RFC PATCH v3] mm: use READ_ONCE " Xishi Qiu 2016-12-06 1:53 ` Xishi Qiu 2016-12-07 8:39 ` Vlastimil Babka 2016-12-07 8:39 ` Vlastimil Babka 2016-12-07 8:43 ` Michal Hocko 2016-12-07 8:43 ` Michal Hocko 2016-12-07 8:48 ` Vlastimil Babka 2016-12-07 8:48 ` Vlastimil Babka 2016-12-07 8:58 ` Michal Hocko 2016-12-07 8:58 ` Michal Hocko 2016-12-07 9:29 ` Vlastimil Babka 2016-12-07 9:29 ` Vlastimil Babka 2016-12-07 9:40 ` Christian Borntraeger 2016-12-07 9:40 ` Christian Borntraeger 2016-12-07 9:59 ` Michal Hocko 2016-12-07 9:59 ` Michal Hocko 2016-12-07 10:03 ` Christian Borntraeger 2016-12-07 10:03 ` Christian Borntraeger 2016-12-07 22:16 ` Rasmus Villemoes 2016-12-07 22:16 ` Rasmus Villemoes
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.