Commit 7f0f15464185a92f9d8791ad231bcd7bf6df54e4
Committed by
Linus Torvalds
Linus Torvalds
1 parent
11cad320a4
Exists in
master
and in
4 other branches
memcg: fix css_id() RCU locking for real
Commit ad4ba375373937817404fd92239ef4cadbded23b ("memcg: css_id() must be called under rcu_read_lock()") modifies memcontol.c for fixing RCU check message. But Andrew Morton pointed out that the fix doesn't seems sane and it was just for hidining lockdep messages. This is a patch for do proper things. Checking again, all places, accessing without rcu_read_lock, that commit fixies was intentional.... all callers of css_id() has reference count on it. So, it's not necessary to be under rcu_read_lock(). Considering again, we can use rcu_dereference_check for css_id(). We know css->id is valid if css->refcnt > 0. (css->id never changes and freed after css->refcnt going to be 0.) This patch makes use of rcu_dereference_check() in css_id/depth and remove unnecessary rcu-read-lock added by the commit. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> Cc: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> Cc: Balbir Singh <balbir@linux.vnet.ibm.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Showing 2 changed files with 18 additions and 16 deletions Side-by-side Diff
kernel/cgroup.c
| ... | ... | @@ -4435,8 +4435,16 @@ |
| 4435 | 4435 | */ |
| 4436 | 4436 | unsigned short css_id(struct cgroup_subsys_state *css) |
| 4437 | 4437 | { |
| 4438 | - struct css_id *cssid = rcu_dereference(css->id); | |
| 4438 | + struct css_id *cssid; | |
| 4439 | 4439 | |
| 4440 | + /* | |
| 4441 | + * This css_id() can return correct value when somone has refcnt | |
| 4442 | + * on this or this is under rcu_read_lock(). Once css->id is allocated, | |
| 4443 | + * it's unchanged until freed. | |
| 4444 | + */ | |
| 4445 | + cssid = rcu_dereference_check(css->id, | |
| 4446 | + rcu_read_lock_held() || atomic_read(&css->refcnt)); | |
| 4447 | + | |
| 4440 | 4448 | if (cssid) |
| 4441 | 4449 | return cssid->id; |
| 4442 | 4450 | return 0; |
| ... | ... | @@ -4445,7 +4453,10 @@ |
| 4445 | 4453 | |
| 4446 | 4454 | unsigned short css_depth(struct cgroup_subsys_state *css) |
| 4447 | 4455 | { |
| 4448 | - struct css_id *cssid = rcu_dereference(css->id); | |
| 4456 | + struct css_id *cssid; | |
| 4457 | + | |
| 4458 | + cssid = rcu_dereference_check(css->id, | |
| 4459 | + rcu_read_lock_held() || atomic_read(&css->refcnt)); | |
| 4449 | 4460 | |
| 4450 | 4461 | if (cssid) |
| 4451 | 4462 | return cssid->depth; |
mm/memcontrol.c
| ... | ... | @@ -2314,9 +2314,7 @@ |
| 2314 | 2314 | |
| 2315 | 2315 | /* record memcg information */ |
| 2316 | 2316 | if (do_swap_account && swapout && memcg) { |
| 2317 | - rcu_read_lock(); | |
| 2318 | 2317 | swap_cgroup_record(ent, css_id(&memcg->css)); |
| 2319 | - rcu_read_unlock(); | |
| 2320 | 2318 | mem_cgroup_get(memcg); |
| 2321 | 2319 | } |
| 2322 | 2320 | if (swapout && memcg) |
| 2323 | 2321 | |
| ... | ... | @@ -2373,10 +2371,8 @@ |
| 2373 | 2371 | { |
| 2374 | 2372 | unsigned short old_id, new_id; |
| 2375 | 2373 | |
| 2376 | - rcu_read_lock(); | |
| 2377 | 2374 | old_id = css_id(&from->css); |
| 2378 | 2375 | new_id = css_id(&to->css); |
| 2379 | - rcu_read_unlock(); | |
| 2380 | 2376 | |
| 2381 | 2377 | if (swap_cgroup_cmpxchg(entry, old_id, new_id) == old_id) { |
| 2382 | 2378 | mem_cgroup_swap_statistics(from, false); |
| ... | ... | @@ -4044,16 +4040,11 @@ |
| 4044 | 4040 | put_page(page); |
| 4045 | 4041 | } |
| 4046 | 4042 | /* throught */ |
| 4047 | - if (ent.val && do_swap_account && !ret) { | |
| 4048 | - unsigned short id; | |
| 4049 | - rcu_read_lock(); | |
| 4050 | - id = css_id(&mc.from->css); | |
| 4051 | - rcu_read_unlock(); | |
| 4052 | - if (id == lookup_swap_cgroup(ent)) { | |
| 4053 | - ret = MC_TARGET_SWAP; | |
| 4054 | - if (target) | |
| 4055 | - target->ent = ent; | |
| 4056 | - } | |
| 4043 | + if (ent.val && do_swap_account && !ret && | |
| 4044 | + css_id(&mc.from->css) == lookup_swap_cgroup(ent)) { | |
| 4045 | + ret = MC_TARGET_SWAP; | |
| 4046 | + if (target) | |
| 4047 | + target->ent = ent; | |
| 4057 | 4048 | } |
| 4058 | 4049 | return ret; |
| 4059 | 4050 | } |