diff for duplicates of <20190801233513.137917-1-guro@fb.com> diff --git a/a/1.txt b/N1/1.txt index e0bafc1..9439398 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,55 +1,65 @@ -Commit 72f0184c8a00 ("mm, memcg: remove hotplug locking from try_charge") -introduced css_tryget()/css_put() calls in drain_all_stock(), -which are supposed to protect the target memory cgroup from being -released during the mem_cgroup_is_descendant() call. -However, it's not completely safe. In theory, memcg can go away -between reading stock->cached pointer and calling css_tryget(). +On Thu, 1 Aug 2019 16:35:13 -0700 Roman Gushchin wrote: +> +> Commit 72f0184c8a00 ("mm, memcg: remove hotplug locking from try_charge") +> introduced css_tryget()/css_put() calls in drain_all_stock(), +> which are supposed to protect the target memory cgroup from being +> released during the mem_cgroup_is_descendant() call. +> +> However, it's not completely safe. In theory, memcg can go away +> between reading stock->cached pointer and calling css_tryget(). -So, let's read the stock->cached pointer and evaluate the memory -cgroup inside a rcu read section, and get rid of -css_tryget()/css_put() calls. +Good catch! +> +> So, let's read the stock->cached pointer and evaluate the memory +> cgroup inside a rcu read section, and get rid of +> css_tryget()/css_put() calls. -Signed-off-by: Roman Gushchin <guro@fb.com> -Cc: Michal Hocko <mhocko@suse.com> ---- - mm/memcontrol.c | 17 +++++++++-------- - 1 file changed, 9 insertions(+), 8 deletions(-) - -diff --git a/mm/memcontrol.c b/mm/memcontrol.c -index 5c7b9facb0eb..d856b64426b7 100644 ---- a/mm/memcontrol.c -+++ b/mm/memcontrol.c -@@ -2235,21 +2235,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) - for_each_online_cpu(cpu) { - struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); - struct mem_cgroup *memcg; -+ bool flush = false; - -+ rcu_read_lock(); - memcg = stock->cached; -- if (!memcg || !stock->nr_pages || !css_tryget(&memcg->css)) -- continue; -- if (!mem_cgroup_is_descendant(memcg, root_memcg)) { -- css_put(&memcg->css); -- continue; -- } -- if (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { -+ if (memcg && stock->nr_pages && -+ mem_cgroup_is_descendant(memcg, root_memcg)) -+ flush = true; -+ rcu_read_unlock(); -+ -+ if (flush && -+ !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { - if (cpu == curcpu) - drain_local_stock(&stock->work); - else - schedule_work_on(cpu, &stock->work); - } -- css_put(&memcg->css); - } - put_cpu(); - mutex_unlock(&percpu_charge_mutex); --- -2.21.0 +You need to either adjust the boundry of the rcu-protected section, or +retain the call pairs, as the memcg cache is dereferenced again in +drain_stock(). +> +> Signed-off-by: Roman Gushchin <guro@fb.com> +> Cc: Michal Hocko <mhocko@suse.com> +> --- +> mm/memcontrol.c | 17 +++++++++-------- +> 1 file changed, 9 insertions(+), 8 deletions(-) +> +> diff --git a/mm/memcontrol.c b/mm/memcontrol.c +> index 5c7b9facb0eb..d856b64426b7 100644 +> --- a/mm/memcontrol.c +> +++ b/mm/memcontrol.c +> @@ -2235,21 +2235,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) +> for_each_online_cpu(cpu) { +> struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); +> struct mem_cgroup *memcg; +> + bool flush = false; +> +> + rcu_read_lock(); +> memcg = stock->cached; +> - if (!memcg || !stock->nr_pages || !css_tryget(&memcg->css)) +> - continue; +> - if (!mem_cgroup_is_descendant(memcg, root_memcg)) { +> - css_put(&memcg->css); +> - continue; +> - } +> - if (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { +> + if (memcg && stock->nr_pages && +> + mem_cgroup_is_descendant(memcg, root_memcg)) +> + flush = true; +> + rcu_read_unlock(); +> + +> + if (flush && +> + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { +> if (cpu == curcpu) +> drain_local_stock(&stock->work); +> else +> schedule_work_on(cpu, &stock->work); +> } +> - css_put(&memcg->css); +> } +> put_cpu(); +> mutex_unlock(&percpu_charge_mutex); +> -- +> 2.21.0 +> diff --git a/a/content_digest b/N1/content_digest index 321f7db..af55fb9 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,70 +1,80 @@ - "From\0Roman Gushchin <guro@fb.com>\0" - "Subject\0[PATCH] mm: memcontrol: switch to rcu protection in drain_all_stock()\0" - "Date\0Thu, 1 Aug 2019 16:35:13 -0700\0" - "To\0Andrew Morton <akpm@linux-foundation.org>" - " <linux-mm@kvack.org>\0" - "Cc\0Michal Hocko <mhocko@kernel.org>" + "From\0Hillf Danton <hdanton@sina.com>\0" + "Subject\0Re: [PATCH] mm: memcontrol: switch to rcu protection in drain_all_stock()\0" + "Date\0Fri, 2 Aug 2019 11:33:33 +0800\0" + "To\0Roman Gushchin <guro@fb.com>\0" + "Cc\0Andrew Morton <akpm@linux-foundation.org>" + linux-mm@kvack.org + Michal Hocko <mhocko@kernel.org> Johannes Weiner <hannes@cmpxchg.org> - <linux-kernel@vger.kernel.org> - <kernel-team@fb.com> - Roman Gushchin <guro@fb.com> + linux-kernel@vger.kernel.org + kernel-team@fb.com " Michal Hocko <mhocko@suse.com>\0" "\00:1\0" "b\0" - "Commit 72f0184c8a00 (\"mm, memcg: remove hotplug locking from try_charge\")\n" - "introduced css_tryget()/css_put() calls in drain_all_stock(),\n" - "which are supposed to protect the target memory cgroup from being\n" - "released during the mem_cgroup_is_descendant() call.\n" "\n" - "However, it's not completely safe. In theory, memcg can go away\n" - "between reading stock->cached pointer and calling css_tryget().\n" + "On Thu, 1 Aug 2019 16:35:13 -0700 Roman Gushchin wrote:\n" + "> \n" + "> Commit 72f0184c8a00 (\"mm, memcg: remove hotplug locking from try_charge\")\n" + "> introduced css_tryget()/css_put() calls in drain_all_stock(),\n" + "> which are supposed to protect the target memory cgroup from being\n" + "> released during the mem_cgroup_is_descendant() call.\n" + "> \n" + "> However, it's not completely safe. In theory, memcg can go away\n" + "> between reading stock->cached pointer and calling css_tryget().\n" "\n" - "So, let's read the stock->cached pointer and evaluate the memory\n" - "cgroup inside a rcu read section, and get rid of\n" - "css_tryget()/css_put() calls.\n" + "Good catch!\n" + "> \n" + "> So, let's read the stock->cached pointer and evaluate the memory\n" + "> cgroup inside a rcu read section, and get rid of\n" + "> css_tryget()/css_put() calls.\n" "\n" - "Signed-off-by: Roman Gushchin <guro@fb.com>\n" - "Cc: Michal Hocko <mhocko@suse.com>\n" - "---\n" - " mm/memcontrol.c | 17 +++++++++--------\n" - " 1 file changed, 9 insertions(+), 8 deletions(-)\n" - "\n" - "diff --git a/mm/memcontrol.c b/mm/memcontrol.c\n" - "index 5c7b9facb0eb..d856b64426b7 100644\n" - "--- a/mm/memcontrol.c\n" - "+++ b/mm/memcontrol.c\n" - "@@ -2235,21 +2235,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)\n" - " \tfor_each_online_cpu(cpu) {\n" - " \t\tstruct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu);\n" - " \t\tstruct mem_cgroup *memcg;\n" - "+\t\tbool flush = false;\n" - " \n" - "+\t\trcu_read_lock();\n" - " \t\tmemcg = stock->cached;\n" - "-\t\tif (!memcg || !stock->nr_pages || !css_tryget(&memcg->css))\n" - "-\t\t\tcontinue;\n" - "-\t\tif (!mem_cgroup_is_descendant(memcg, root_memcg)) {\n" - "-\t\t\tcss_put(&memcg->css);\n" - "-\t\t\tcontinue;\n" - "-\t\t}\n" - "-\t\tif (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {\n" - "+\t\tif (memcg && stock->nr_pages &&\n" - "+\t\t mem_cgroup_is_descendant(memcg, root_memcg))\n" - "+\t\t\tflush = true;\n" - "+\t\trcu_read_unlock();\n" - "+\n" - "+\t\tif (flush &&\n" - "+\t\t !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {\n" - " \t\t\tif (cpu == curcpu)\n" - " \t\t\t\tdrain_local_stock(&stock->work);\n" - " \t\t\telse\n" - " \t\t\t\tschedule_work_on(cpu, &stock->work);\n" - " \t\t}\n" - "-\t\tcss_put(&memcg->css);\n" - " \t}\n" - " \tput_cpu();\n" - " \tmutex_unlock(&percpu_charge_mutex);\n" - "-- \n" - 2.21.0 + "You need to either adjust the boundry of the rcu-protected section, or\n" + "retain the call pairs, as the memcg cache is dereferenced again in\n" + "drain_stock().\n" + "> \n" + "> Signed-off-by: Roman Gushchin <guro@fb.com>\n" + "> Cc: Michal Hocko <mhocko@suse.com>\n" + "> ---\n" + "> mm/memcontrol.c | 17 +++++++++--------\n" + "> 1 file changed, 9 insertions(+), 8 deletions(-)\n" + "> \n" + "> diff --git a/mm/memcontrol.c b/mm/memcontrol.c\n" + "> index 5c7b9facb0eb..d856b64426b7 100644\n" + "> --- a/mm/memcontrol.c\n" + "> +++ b/mm/memcontrol.c\n" + "> @@ -2235,21 +2235,22 @@ static void drain_all_stock(struct mem_cgroup *root_memcg)\n" + "> \tfor_each_online_cpu(cpu) {\n" + "> \t\tstruct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu);\n" + "> \t\tstruct mem_cgroup *memcg;\n" + "> +\t\tbool flush = false;\n" + "> \n" + "> +\t\trcu_read_lock();\n" + "> \t\tmemcg = stock->cached;\n" + "> -\t\tif (!memcg || !stock->nr_pages || !css_tryget(&memcg->css))\n" + "> -\t\t\tcontinue;\n" + "> -\t\tif (!mem_cgroup_is_descendant(memcg, root_memcg)) {\n" + "> -\t\t\tcss_put(&memcg->css);\n" + "> -\t\t\tcontinue;\n" + "> -\t\t}\n" + "> -\t\tif (!test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {\n" + "> +\t\tif (memcg && stock->nr_pages &&\n" + "> +\t\t mem_cgroup_is_descendant(memcg, root_memcg))\n" + "> +\t\t\tflush = true;\n" + "> +\t\trcu_read_unlock();\n" + "> +\n" + "> +\t\tif (flush &&\n" + "> +\t\t !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) {\n" + "> \t\t\tif (cpu == curcpu)\n" + "> \t\t\t\tdrain_local_stock(&stock->work);\n" + "> \t\t\telse\n" + "> \t\t\t\tschedule_work_on(cpu, &stock->work);\n" + "> \t\t}\n" + "> -\t\tcss_put(&memcg->css);\n" + "> \t}\n" + "> \tput_cpu();\n" + "> \tmutex_unlock(&percpu_charge_mutex);\n" + "> -- \n" + "> 2.21.0\n" + > -0a5d9ba8fc64be5ffc2c19b3e8f10703bf6ad93dec4091d59e460202a567dfde +f590220d07a35b3684690ee7dafdce1d4a92ede5b5b420a9aabd3b587fe28287
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox