On Fri, 7 Feb 2020 at 18:22, Qian Cai <cai@xxxxxx> wrote: > > struct mem_cgroup_per_node mz.lru_zone_size[zone_idx][lru] could be > accessed concurrently as noticed by KCSAN, > > BUG: KCSAN: data-race in lruvec_lru_size / mem_cgroup_update_lru_size > > write to 0xffff9c804ca285f8 of 8 bytes by task 50951 on cpu 12: > mem_cgroup_update_lru_size+0x11c/0x1d0 > mem_cgroup_update_lru_size at mm/memcontrol.c:1266 > isolate_lru_pages+0x6a9/0xf30 > shrink_active_list+0x123/0xcc0 > shrink_lruvec+0x8fd/0x1380 > shrink_node+0x317/0xd80 > do_try_to_free_pages+0x1f7/0xa10 > try_to_free_pages+0x26c/0x5e0 > __alloc_pages_slowpath+0x458/0x1290 > __alloc_pages_nodemask+0x3bb/0x450 > alloc_pages_vma+0x8a/0x2c0 > do_anonymous_page+0x170/0x700 > __handle_mm_fault+0xc9f/0xd00 > handle_mm_fault+0xfc/0x2f0 > do_page_fault+0x263/0x6f9 > page_fault+0x34/0x40 > > read to 0xffff9c804ca285f8 of 8 bytes by task 50964 on cpu 95: > lruvec_lru_size+0xbb/0x270 > mem_cgroup_get_zone_lru_size at include/linux/memcontrol.h:536 > (inlined by) lruvec_lru_size at mm/vmscan.c:326 > shrink_lruvec+0x1d0/0x1380 > shrink_node+0x317/0xd80 > do_try_to_free_pages+0x1f7/0xa10 > try_to_free_pages+0x26c/0x5e0 > __alloc_pages_slowpath+0x458/0x1290 > __alloc_pages_nodemask+0x3bb/0x450 > alloc_pages_current+0xa6/0x120 > alloc_slab_page+0x3b1/0x540 > allocate_slab+0x70/0x660 > new_slab+0x46/0x70 > ___slab_alloc+0x4ad/0x7d0 > __slab_alloc+0x43/0x70 > kmem_cache_alloc+0x2c3/0x420 > getname_flags+0x4c/0x230 > getname+0x22/0x30 > do_sys_openat2+0x205/0x3b0 > do_sys_open+0x9a/0xf0 > __x64_sys_openat+0x62/0x80 > do_syscall_64+0x91/0xb47 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Reported by Kernel Concurrency Sanitizer on: > CPU: 95 PID: 50964 Comm: cc1 Tainted: G W O L 5.5.0-next-20200204+ #6 > Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 07/10/2019 > > The write is under lru_lock, but the read is done as lockless. The scan > count is used to determine how aggressively the anon and file LRU lists > should be scanned. Load tearing could generate an inefficient heuristic, > so fix it by adding READ_ONCE() for the read and WRITE_ONCE() for the > writes. > > Signed-off-by: Qian Cai <cai@xxxxxx> > --- > > v2: also have WRITE_ONCE() in the writer which is necessary. Again, note that KCSAN will *not* complain if you omitted the WRITE_ONCE and only had the READ_ONCE, as long as the write aligned and up to word-size. Because we still don't have a nice way to deal with read-modify-writes, like 'var +=', '++', I don't know if we want to do the WRITE_ONCE right now. I think the kernel might need a primitive that avoids the readability issues of writing 'WRITE_ONCE(var, var + val)'. I don't have strong opinions on this, so it's up to maintainers. Thanks, -- Marco > include/linux/memcontrol.h | 2 +- > mm/memcontrol.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index a7a0a1a5c8d5..e8734dabbc61 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -533,7 +533,7 @@ unsigned long mem_cgroup_get_zone_lru_size(struct lruvec *lruvec, > struct mem_cgroup_per_node *mz; > > mz = container_of(lruvec, struct mem_cgroup_per_node, lruvec); > - return mz->lru_zone_size[zone_idx][lru]; > + return READ_ONCE(mz->lru_zone_size[zone_idx][lru]); > } > > void mem_cgroup_handle_over_high(void); > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 6f6dc8712e39..daf375cc312c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1263,7 +1263,7 @@ void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru, > lru_size = &mz->lru_zone_size[zid][lru]; > > if (nr_pages < 0) > - *lru_size += nr_pages; > + WRITE_ONCE(*lru_size, *lru_size + nr_pages); > > size = *lru_size; > if (WARN_ONCE(size < 0, > @@ -1274,7 +1274,7 @@ void mem_cgroup_update_lru_size(struct lruvec *lruvec, enum lru_list lru, > } > > if (nr_pages > 0) > - *lru_size += nr_pages; > + WRITE_ONCE(*lru_size, *lru_size + nr_pages); > } > > /** > -- > 1.8.3.1 >
