[RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

[RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

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

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

Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

[Christian Borntraeger]

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

Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

[Christian Borntraeger]

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.

Re: [RFC PATCH] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

[Xishi Qiu]

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!

> 
> 

[RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

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

Re: [RFC PATCH v2] mm: use ACCESS_ONCE in page_cpupid_xchg_last()

[Christian Borntraeger]

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

[RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Vlastimil Babka]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Michal Hocko]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Vlastimil Babka]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Michal Hocko]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Vlastimil Babka]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Christian Borntraeger]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Michal Hocko]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Christian Borntraeger]

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

Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

[Rasmus Villemoes]

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/