mm: use ACCESS_ONCE in page_cpupid_xchg_last()

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 <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

Quoted 3 lines · Xishi Qiu

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.

Quoted 11 lines

Signed-off-by Xishi Qiu <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 108

Quoted 8 lines

mm/mmzone.c +1 -1

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

Quoted 30 lines · Christian Borntraeger

Quoted 3 lines · Xishi Qiu

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.

Quoted 11 lines

Signed-off-by Xishi Qiu <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 108

Quoted 8 lines

mm/mmzone.c +1 -1

@@ -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

Quoted 5 lines

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

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c +0 -0

 		flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);

Signature · 1 line

1.8.3.1

Quoted 17 lines · Xishi Qiu

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644
@@ -102,7 +102,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
 	int last_cpupid;
 

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 106

Quoted 4 lines

mm/mmzone.c +1 -1

 	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?

Quoted 5 lines

mm/mmzone.c +0 -0

 		flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);

Signature · 1 line

1.8.3.1

Quoted 18 lines · Michal Hocko

Quoted 17 lines · Xishi Qiu

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644
@@ -102,7 +102,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
 	int last_cpupid;
 

Quoted 8 lines

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 106

Quoted 4 lines

mm/mmzone.c +1 -1

 	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?

Quoted 1 line

Or this doesn't matter?

I think it would matter.

Quoted 6 lines

Quoted 5 lines

mm/mmzone.c +0 -0

 		flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);

Signature · 1 line

1.8.3.1

Quoted 28 lines · Vlastimil Babka

Quoted 26 lines · Michal Hocko

Quoted 15 lines · Xishi Qiu

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 106

Quoted 6 lines

mm/mmzone.c +1 -1

@@ -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);

Quoted 2 lines

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

mm/mmzone.c +2 -2

index 5652be858e5e..b4e093dd24c1 100644
@@ -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));
 

Quoted 3 lines

Quoted 2 lines

Or this doesn't matter?

Quoted 8 lines

I think it would matter.

Quoted 6 lines

Quoted 5 lines

mm/mmzone.c +0 -0

 		flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);

Signature · 1 line

1.8.3.1

Quoted 29 lines · Michal Hocko

Quoted 28 lines · Vlastimil Babka

Quoted 26 lines · Michal Hocko

Quoted 15 lines · Xishi Qiu

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 106

Quoted 6 lines

mm/mmzone.c +1 -1

@@ -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);

Quoted 6 lines

Quoted 2 lines

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.

Quoted 1 line

Anyway, this could be addressed easily by

Yes, that way there should be no doubt.

Quoted 24 lines

mm/mmzone.c +2 -2

index 5652be858e5e..b4e093dd24c1 100644
@@ -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));
 

Quoted 6 lines

Quoted 2 lines

Or this doesn't matter?

I think it would matter.

Quoted 1 line

Quoted 1 line

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

flags &= ~(LAST_CPUPID_MASK << LAST_CPUPID_PGSHIFT);

Signature · 1 line

1.8.3.1

Quoted 37 lines · Vlastimil Babka

Quoted 29 lines · Michal Hocko

Quoted 28 lines · Vlastimil Babka

Quoted 26 lines · Michal Hocko

Quoted 15 lines · Xishi Qiu

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 106

Quoted 6 lines

mm/mmzone.c +1 -1

@@ -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);

Quoted 6 lines

Quoted 2 lines

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?

Quoted 7 lines

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.

Quoted 1 line

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;

Quoted 52 lines · Christian Borntraeger

Quoted 37 lines · Vlastimil Babka

Quoted 29 lines · Michal Hocko

Quoted 28 lines · Vlastimil Babka

Quoted 26 lines · Michal Hocko

Quoted 15 lines · Xishi Qiu

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 <[email protected]>

Suggested-by Christian Borntraeger <[email protected]>

---
 mm/mmzone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mmzone.c +0 -0

index 5652be8..e0b698e 100644

mm/mmzone.c int page_cpupid_xchg_last(struct page *page, int cpupid) · line 106

Quoted 6 lines

mm/mmzone.c +1 -1

@@ -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);

Quoted 6 lines

Quoted 2 lines

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?

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 14 lines · Michal Hocko

Quoted 13 lines · Christian Borntraeger

Quoted 5 lines · Vlastimil Babka

Quoted 2 lines · Michal Hocko

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

Quoted 8 lines

Quoted 2 lines

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