* [PATCH] mm, memcg: assign shrinker_map before kvfree
@ 2019-09-20 12:29 Cyrill Gorcunov
2019-09-20 13:21 ` Kirill A. Shutemov
2019-09-20 14:11 ` Kirill Tkhai
0 siblings, 2 replies; 6+ messages in thread
From: Cyrill Gorcunov @ 2019-09-20 12:29 UTC (permalink / raw)
To: LKML
Cc: Linux MM, Johannes Weiner, Michal Hocko, Vladimir Davydov, Kirill Tkhai
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.
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
mm/memcontrol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-tip.git/mm/memcontrol.c
===================================================================
--- linux-tip.git.orig/mm/memcontrol.c
+++ linux-tip.git/mm/memcontrol.c
@@ -364,9 +364,9 @@ static void memcg_free_shrinker_maps(str
for_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);
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm, memcg: assign shrinker_map before kvfree 2019-09-20 12:29 [PATCH] mm, memcg: assign shrinker_map before kvfree Cyrill Gorcunov @ 2019-09-20 13:21 ` Kirill A. Shutemov 2019-09-20 13:40 ` Cyrill Gorcunov 2019-09-20 14:20 ` Kirill Tkhai 2019-09-20 14:11 ` Kirill Tkhai 1 sibling, 2 replies; 6+ messages in thread From: Kirill A. Shutemov @ 2019-09-20 13:21 UTC (permalink / raw) To: Cyrill Gorcunov Cc: LKML, Linux MM, Johannes Weiner, Michal Hocko, Vladimir Davydov, Kirill Tkhai On Fri, Sep 20, 2019 at 03:29:07PM +0300, Cyrill Gorcunov wrote: > 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. With this logic it will still look suspicious since you don't wait a grace period before freeing the map. -- Kirill A. Shutemov ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memcg: assign shrinker_map before kvfree 2019-09-20 13:21 ` Kirill A. Shutemov @ 2019-09-20 13:40 ` Cyrill Gorcunov 2019-09-20 14:20 ` Kirill Tkhai 1 sibling, 0 replies; 6+ messages in thread From: Cyrill Gorcunov @ 2019-09-20 13:40 UTC (permalink / raw) To: Kirill A. Shutemov Cc: LKML, Linux MM, Johannes Weiner, Michal Hocko, Vladimir Davydov, Kirill Tkhai On Fri, Sep 20, 2019 at 04:21:14PM +0300, Kirill A. Shutemov wrote: > On Fri, Sep 20, 2019 at 03:29:07PM +0300, Cyrill Gorcunov wrote: > > 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. > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memcg: assign shrinker_map before kvfree 2019-09-20 13:21 ` Kirill A. Shutemov 2019-09-20 13:40 ` Cyrill Gorcunov @ 2019-09-20 14:20 ` Kirill Tkhai 1 sibling, 0 replies; 6+ messages in thread From: Kirill Tkhai @ 2019-09-20 14:20 UTC (permalink / raw) To: Kirill A. Shutemov, Cyrill Gorcunov Cc: LKML, Linux MM, Johannes Weiner, Michal Hocko, Vladimir Davydov On 20.09.2019 16:21, Kirill A. Shutemov wrote: > On Fri, Sep 20, 2019 at 03:29:07PM +0300, Cyrill Gorcunov wrote: >> 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. > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memcg: assign shrinker_map before kvfree 2019-09-20 12:29 [PATCH] mm, memcg: assign shrinker_map before kvfree Cyrill Gorcunov 2019-09-20 13:21 ` Kirill A. Shutemov @ 2019-09-20 14:11 ` Kirill Tkhai 2019-09-20 14:31 ` Cyrill Gorcunov 1 sibling, 1 reply; 6+ messages in thread From: Kirill Tkhai @ 2019-09-20 14:11 UTC (permalink / raw) To: Cyrill Gorcunov, LKML Cc: Linux MM, Johannes Weiner, Michal Hocko, Vladimir Davydov On 20.09.2019 15:29, Cyrill Gorcunov wrote: > 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. > > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com> > Cc: Kirill Tkhai <ktkhai@virtuozzo.com> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> > --- > mm/memcontrol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: linux-tip.git/mm/memcontrol.c > =================================================================== > --- linux-tip.git.orig/mm/memcontrol.c > +++ linux-tip.git/mm/memcontrol.c > @@ -364,9 +364,9 @@ static void memcg_free_shrinker_maps(str > for_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: diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1c4c08b45e44..454303c3aa0f 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -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; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memcg: assign shrinker_map before kvfree 2019-09-20 14:11 ` Kirill Tkhai @ 2019-09-20 14:31 ` Cyrill Gorcunov 0 siblings, 0 replies; 6+ messages in thread From: Cyrill Gorcunov @ 2019-09-20 14:31 UTC (permalink / raw) To: Kirill Tkhai Cc: LKML, Linux MM, Johannes Weiner, Michal Hocko, Vladimir Davydov On Fri, Sep 20, 2019 at 05:11:00PM +0300, Kirill Tkhai wrote: > > 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: 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-20 14:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-20 12:29 [PATCH] mm, memcg: assign shrinker_map before kvfree Cyrill Gorcunov 2019-09-20 13:21 ` Kirill A. Shutemov 2019-09-20 13:40 ` Cyrill Gorcunov 2019-09-20 14:20 ` Kirill Tkhai 2019-09-20 14:11 ` Kirill Tkhai 2019-09-20 14:31 ` Cyrill Gorcunov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).