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@xxxxxxxxxx> > >> Suggested-by: Christian Borntraeger <borntraeger@xxxxxxxxxx> > >> --- > >> 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@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>