| Message ID | 20230306154138.3775-2-findns94@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, memcg: cgroup v1 and v2 tunable load/store tearing fixes | expand |
On Mon 06-03-23 23:41:35, Yue Zhao wrote: > The knob for cgroup v2 memory controller: memory.oom.group > is not protected by any locking so it can be modified while it is used. > This is not an actual problem because races are unlikely (the knob is > usually configured long before any workloads hits actual memcg oom) > but it is better to use READ_ONCE/WRITE_ONCE to prevent compiler from > doing anything funky. > > The access of memcg->oom_group is lockless, so it can be > concurrently set at the same time as we are trying to read it. > > Signed-off-by: Yue Zhao <findns94@gmail.com> Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > mm/memcontrol.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 5abffe6f8389..06821e5f7604 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2067,7 +2067,7 @@ struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, > * highest-level memory cgroup with oom.group set. > */ > for (; memcg; memcg = parent_mem_cgroup(memcg)) { > - if (memcg->oom_group) > + if (READ_ONCE(memcg->oom_group)) > oom_group = memcg; > > if (memcg == oom_domain) > @@ -6623,7 +6623,7 @@ static int memory_oom_group_show(struct seq_file *m, void *v) > { > struct mem_cgroup *memcg = mem_cgroup_from_seq(m); > > - seq_printf(m, "%d\n", memcg->oom_group); > + seq_printf(m, "%d\n", READ_ONCE(memcg->oom_group)); > > return 0; > } > @@ -6645,7 +6645,7 @@ static ssize_t memory_oom_group_write(struct kernfs_open_file *of, > if (oom_group != 0 && oom_group != 1) > return -EINVAL; > > - memcg->oom_group = oom_group; > + WRITE_ONCE(memcg->oom_group, oom_group); > > return nbytes; > } > -- > 2.17.1
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 5abffe6f8389..06821e5f7604 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2067,7 +2067,7 @@ struct mem_cgroup *mem_cgroup_get_oom_group(struct task_struct *victim, * highest-level memory cgroup with oom.group set. */ for (; memcg; memcg = parent_mem_cgroup(memcg)) { - if (memcg->oom_group) + if (READ_ONCE(memcg->oom_group)) oom_group = memcg; if (memcg == oom_domain) @@ -6623,7 +6623,7 @@ static int memory_oom_group_show(struct seq_file *m, void *v) { struct mem_cgroup *memcg = mem_cgroup_from_seq(m); - seq_printf(m, "%d\n", memcg->oom_group); + seq_printf(m, "%d\n", READ_ONCE(memcg->oom_group)); return 0; } @@ -6645,7 +6645,7 @@ static ssize_t memory_oom_group_write(struct kernfs_open_file *of, if (oom_group != 0 && oom_group != 1) return -EINVAL; - memcg->oom_group = oom_group; + WRITE_ONCE(memcg->oom_group, oom_group); return nbytes; }
The knob for cgroup v2 memory controller: memory.oom.group is not protected by any locking so it can be modified while it is used. This is not an actual problem because races are unlikely (the knob is usually configured long before any workloads hits actual memcg oom) but it is better to use READ_ONCE/WRITE_ONCE to prevent compiler from doing anything funky. The access of memcg->oom_group is lockless, so it can be concurrently set at the same time as we are trying to read it. Signed-off-by: Yue Zhao <findns94@gmail.com> --- mm/memcontrol.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)