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(-)
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
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 <[email protected]>
> ---
> 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).
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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
>>> ---
>>> 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!
>
>
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(-)
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
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 <[email protected]>
> Suggested-by: Christian Borntraeger <[email protected]>
> ---
> 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);
>
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(-)
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
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 <[email protected]>
> Suggested-by: Christian Borntraeger <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
> ---
> 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);
>
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 <[email protected]>
> Suggested-by: Christian Borntraeger <[email protected]>
> ---
> 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
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 <[email protected]>
>> Suggested-by: Christian Borntraeger <[email protected]>
>> ---
>> 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
>>
>
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 <[email protected]>
> >> Suggested-by: Christian Borntraeger <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
>>>> Suggested-by: Christian Borntraeger <[email protected]>
>>>> ---
>>>> 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
>>>>
>>>
>
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 <[email protected]>
>>>>> Suggested-by: Christian Borntraeger <[email protected]>
>>>>> ---
>>>>> 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
>>>>>
>>>>
>>
>
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 <[email protected]>
> >>>>> Suggested-by: Christian Borntraeger <[email protected]>
> >>>>> ---
> >>>>> 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
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 <[email protected]>
>>>>>>> Suggested-by: Christian Borntraeger <[email protected]>
>>>>>>> ---
>>>>>>> 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...
On Wed, Dec 07 2016, Christian Borntraeger <[email protected]> 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/