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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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>