mm, memcg: assign shrinker_map before kvfree
Subsystems (2)
PATCH 1/1 mm, memcg: assign shrinker_map before kvfree
Currently there is a small gap between fetching pointer, calling kvfree and assign its value to nil. In current callgraph it is not a problem (since memcg_free_shrinker_maps is running from memcg_alloc_shrinker_maps and mem_cgroup_css_free only) still this looks suspicious and we can easily eliminate the gap at all.
--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +1 -1
@@ -364,9 +364,9 @@ static void memcg_free_shrinker_maps(strfor_each_node (nid) { pn =mem_cgroup_nodeinfo (memcg, nid); map =rcu_dereference_protected (pn->shrinker_map, true); +rcu_assign_pointer (pn->shrinker_map,NULL );if (map)kvfree (map); -rcu_assign_pointer (pn->shrinker_map,NULL ); } }
Quoted 5 lines · Cyrill Gorcunov
Currently there is a small gap between fetching pointer, calling kvfree and assign its value to nil. In current callgraph it is not a problem (since
memcg_free_shrinker_mapsis running frommemcg_alloc_shrinker_mapsandmem_cgroup_css_freeonly) still this looks suspicious and we can easily eliminate the gap at all.
With this logic it will still look suspicious since you don't wait a grace period before freeing the map.
Signature · 1 line
Kirill A. Shutemov
Quoted 9 lines · Kirill A. Shutemov
Quoted 5 lines · Cyrill Gorcunov
Currently there is a small gap between fetching pointer, calling kvfree and assign its value to nil. In current callgraph it is not a problem (since
memcg_free_shrinker_mapsis running frommemcg_alloc_shrinker_mapsandmem_cgroup_css_freeonly) still this looks suspicious and we can easily eliminate the gap at all.With this logic it will still look suspicious since you don't wait a grace period before freeing the map.
Probably, but as far as I see we're using mutex here to order requests. I'm not sure, maybe ktkhai@ made the code to use free before the assign intentionally? As I said there is no bug it the code right now just forced me to stop and reread it several times due to this gap. If you look into other code places where we use similar technique we always assign before free.
Quoted 9 lines · Kirill A. Shutemov
Quoted 5 lines · Cyrill Gorcunov
Currently there is a small gap between fetching pointer, calling kvfree and assign its value to nil. In current callgraph it is not a problem (since
memcg_free_shrinker_mapsis running frommemcg_alloc_shrinker_mapsandmem_cgroup_css_freeonly) still this looks suspicious and we can easily eliminate the gap at all.With this logic it will still look suspicious since you don't wait a grace period before freeing the map.
This freeing occurs in the moment, when nobody can dereference shrinker_map in parallel:
memcg is either not yet online or its css->refcnt is already dead. This NULLifying is needed just to prevent double freeing of shrinker_map.
Please, see the explanation in my email to our namesake.
Kirill
Quoted 20 lines · Cyrill Gorcunov
Currently there is a small gap between fetching pointer, calling kvfree and assign its value to nil. In current callgraph it is not a problem (since
memcg_free_shrinker_mapsis running frommemcg_alloc_shrinker_mapsandmem_cgroup_css_freeonly) still this looks suspicious and we can easily eliminate the gap at all.Cc Johannes Weiner <[email protected]>Cc Michal Hocko <[email protected]>Cc Vladimir Davydov <[email protected]>Cc Kirill Tkhai <[email protected]>Signed-off-by Cyrill Gorcunov <[email protected]>--- mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
linux-tip.git/mm/memcontrol.c
===================================================================
mm/memcontrol.c static void memcg_free_shrinker_maps(str · line 371
Quoted 9 lines
mm/memcontrol.c +1
-1@@ -364,9 +364,9 @@ static void memcg_free_shrinker_maps(strfor_each_node (nid) { pn =mem_cgroup_nodeinfo (memcg, nid); map =rcu_dereference_protected (pn->shrinker_map, true); +rcu_assign_pointer (pn->shrinker_map,NULL );if (map)kvfree (map); -rcu_assign_pointer (pn->shrinker_map,NULL ); } }
The current scheme is following. We allocate shrinker_map in css_online, while normal freeing happens in css_free. The NULLifying of pointer is needed in case of "abnormal freeing", when memcg_free_shrinker_maps() is called from memcg_alloc_shrinker_maps(). The NULLifying guarantees, we won't free pn->shrinker_map twice.
There are no races or problems with kvfree() and rcu_assign_pointer() order, because of nobody can reference shrinker_map before memcg is online.
In case of this rcu_assign_pointer() confuses, we may just remove is from the function, and call it only on css_free. Something like the below:
mm/memcontrol.c +0 -2
@@ -372,7 +372,6 @@ static void memcg_free_shrinker_maps(struct mem_cgroup *memcg) map =rcu_dereference_protected (pn->shrinker_map, true);if (map)kvfree (map); -rcu_assign_pointer (pn->shrinker_map,NULL ); } }
@@ -389,7 +388,6 @@ static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)for_each_node (nid) { map =kvzalloc (sizeof (*map) + size,GFP_KERNEL );if (!map) { -memcg_free_shrinker_maps (memcg); ret = -ENOMEM ;break ; }
Quoted 7 lines · Kirill Tkhai
The current scheme is following. We allocate
shrinker_mapincss_online, while normal freeing happens incss_free. The NULLifying of pointer is needed in case of "abnormal freeing", whenmemcg_free_shrinker_maps()is called frommemcg_alloc_shrinker_maps(). The NULLifying guarantees, we won't freepn->shrinker_maptwice.
Quoted 5 lines
There are no races or problems with
kvfree()andrcu_assign_pointer()order, because of nobody can referenceshrinker_mapbefore memcg is online.In case of this
rcu_assign_pointer()confuses, we may just remove is from the function, and call it only oncss_free. Something like the below:
Kirill, I know that there is no problem now (as I pointed in changelog), simply a regular pattern of free after assign is being reversed, which made me nervious. Anyway dropping assigns doesn't help much from my pov so lets leave it as is. The good point is that we've this conversation and if someone get a bit confused in future the google will reveal this text. Which is enough I think.