From: Andrew Morton <akpm@linux-foundation.org>
To: Dima Zavin <dmitriyz@waymo.com>
Cc: Christopher Lameter <cl@linux.com>, Li Zefan <lizefan@huawei.com>,
Pekka Enberg <penberg@kernel.org>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, Cliff Spradlin <cspradlin@waymo.com>,
Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [PATCH v2] cpuset: fix a deadlock due to incomplete patching of cpusets_enabled()
Date: Thu, 27 Jul 2017 12:48:55 -0700 [thread overview]
Message-ID: <20170727124855.aeb97ea9f74af2d3e47e1787@linux-foundation.org> (raw)
In-Reply-To: <20170727164608.12701-1-dmitriyz@waymo.com>
On Thu, 27 Jul 2017 09:46:08 -0700 Dima Zavin <dmitriyz@waymo.com> wrote:
> In codepaths that use the begin/retry interface for reading
> mems_allowed_seq with irqs disabled, there exists a race condition that
> stalls the patch process after only modifying a subset of the
> static_branch call sites.
>
> This problem manifested itself as a dead lock in the slub
> allocator, inside get_any_partial. The loop reads
> mems_allowed_seq value (via read_mems_allowed_begin),
> performs the defrag operation, and then verifies the consistency
> of mem_allowed via the read_mems_allowed_retry and the cookie
> returned by xxx_begin. The issue here is that both begin and retry
> first check if cpusets are enabled via cpusets_enabled() static branch.
> This branch can be rewritted dynamically (via cpuset_inc) if a new
> cpuset is created. The x86 jump label code fully synchronizes across
> all CPUs for every entry it rewrites. If it rewrites only one of the
> callsites (specifically the one in read_mems_allowed_retry) and then
> waits for the smp_call_function(do_sync_core) to complete while a CPU is
> inside the begin/retry section with IRQs off and the mems_allowed value
> is changed, we can hang. This is because begin() will always return 0
> (since it wasn't patched yet) while retry() will test the 0 against
> the actual value of the seq counter.
>
> The fix is to cache the value that's returned by cpusets_enabled() at the
> top of the loop, and only operate on the seqcount (both begin and retry) if
> it was true.
Tricky. Hence we should have a nice code comment somewhere describing
all of this.
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -16,6 +16,11 @@
> #include <linux/mm.h>
> #include <linux/jump_label.h>
>
> +struct cpuset_mems_cookie {
> + unsigned int seq;
> + bool was_enabled;
> +};
At cpuset_mems_cookie would be a good site - why it exists, what it
does, when it is used and how.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org>
To: Dima Zavin <dmitriyz@waymo.com>
Cc: Christopher Lameter <cl@linux.com>, Li Zefan <lizefan@huawei.com>,
Pekka Enberg <penberg@kernel.org>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, Cliff Spradlin <cspradlin@waymo.com>,
Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [PATCH v2] cpuset: fix a deadlock due to incomplete patching of cpusets_enabled()
Date: Thu, 27 Jul 2017 12:48:55 -0700 [thread overview]
Message-ID: <20170727124855.aeb97ea9f74af2d3e47e1787@linux-foundation.org> (raw)
In-Reply-To: <20170727164608.12701-1-dmitriyz@waymo.com>
On Thu, 27 Jul 2017 09:46:08 -0700 Dima Zavin <dmitriyz@waymo.com> wrote:
> In codepaths that use the begin/retry interface for reading
> mems_allowed_seq with irqs disabled, there exists a race condition that
> stalls the patch process after only modifying a subset of the
> static_branch call sites.
>
> This problem manifested itself as a dead lock in the slub
> allocator, inside get_any_partial. The loop reads
> mems_allowed_seq value (via read_mems_allowed_begin),
> performs the defrag operation, and then verifies the consistency
> of mem_allowed via the read_mems_allowed_retry and the cookie
> returned by xxx_begin. The issue here is that both begin and retry
> first check if cpusets are enabled via cpusets_enabled() static branch.
> This branch can be rewritted dynamically (via cpuset_inc) if a new
> cpuset is created. The x86 jump label code fully synchronizes across
> all CPUs for every entry it rewrites. If it rewrites only one of the
> callsites (specifically the one in read_mems_allowed_retry) and then
> waits for the smp_call_function(do_sync_core) to complete while a CPU is
> inside the begin/retry section with IRQs off and the mems_allowed value
> is changed, we can hang. This is because begin() will always return 0
> (since it wasn't patched yet) while retry() will test the 0 against
> the actual value of the seq counter.
>
> The fix is to cache the value that's returned by cpusets_enabled() at the
> top of the loop, and only operate on the seqcount (both begin and retry) if
> it was true.
Tricky. Hence we should have a nice code comment somewhere describing
all of this.
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -16,6 +16,11 @@
> #include <linux/mm.h>
> #include <linux/jump_label.h>
>
> +struct cpuset_mems_cookie {
> + unsigned int seq;
> + bool was_enabled;
> +};
At cpuset_mems_cookie would be a good site - why it exists, what it
does, when it is used and how.
next prev parent reply other threads:[~2017-07-27 19:48 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-26 16:50 [RFC PATCH] mm/slub: fix a deadlock due to incomplete patching of cpusets_enabled() Dima Zavin
2017-07-26 16:50 ` Dima Zavin
2017-07-26 16:50 ` Dima Zavin
2017-07-26 17:02 ` Christopher Lameter
2017-07-26 19:52 ` Dima Zavin
2017-07-26 19:54 ` Dima Zavin
2017-07-26 19:54 ` Dima Zavin
2017-07-27 16:46 ` [PATCH v2] cpuset: " Dima Zavin
2017-07-27 16:46 ` Dima Zavin
2017-07-27 16:46 ` Dima Zavin
2017-07-27 19:48 ` Andrew Morton [this message]
2017-07-27 19:48 ` Andrew Morton
2017-07-27 21:41 ` Dima Zavin
2017-07-27 21:41 ` Dima Zavin
2017-07-28 7:45 ` Vlastimil Babka
2017-07-28 7:45 ` Vlastimil Babka
[not found] ` <41954034-9de1-de8e-f915-51a4b0334f98-AlSwsSmVLrQ@public.gmane.org>
2017-07-28 8:48 ` Dima Zavin
2017-07-28 8:48 ` Dima Zavin
2017-07-28 8:48 ` Dima Zavin
2017-07-28 9:30 ` Peter Zijlstra
2017-07-28 9:30 ` Peter Zijlstra
[not found] ` <20170728093047.ykgbufjj74xa5x3r-Nxj+rRp3nVydTX5a5knrm8zTDFooKrT+cvkQGrU6aU0@public.gmane.org>
2017-07-28 14:05 ` Vlastimil Babka
2017-07-28 14:05 ` Vlastimil Babka
2017-07-28 14:05 ` Vlastimil Babka
2017-07-28 16:52 ` Dima Zavin
2017-07-28 16:52 ` Dima Zavin
2017-07-31 4:01 ` [PATCH v3] " Dima Zavin
2017-07-31 4:01 ` Dima Zavin
[not found] ` <20170731040113.14197-1-dmitriyz-FA3Td6GO0iIAvxtiuMwx3w@public.gmane.org>
2017-07-31 4:04 ` Dima Zavin
2017-07-31 4:04 ` Dima Zavin
2017-07-31 4:04 ` Dima Zavin
2017-07-31 8:02 ` Vlastimil Babka
2017-07-31 8:02 ` Vlastimil Babka
2017-07-31 9:05 ` Dima Zavin
2017-07-31 9:05 ` Dima Zavin
[not found] ` <20170727164608.12701-1-dmitriyz-FA3Td6GO0iIAvxtiuMwx3w@public.gmane.org>
2017-07-27 19:51 ` [PATCH v2] " Andrew Morton
2017-07-27 19:51 ` Andrew Morton
2017-07-27 19:51 ` Andrew Morton
2017-07-27 21:41 ` Dima Zavin
2017-07-27 21:41 ` Dima Zavin
2017-07-29 4:56 ` kbuild test robot
2017-07-29 4:56 ` kbuild test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170727124855.aeb97ea9f74af2d3e47e1787@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=cl@linux.com \
--cc=cspradlin@waymo.com \
--cc=dmitriyz@waymo.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lizefan@huawei.com \
--cc=mgorman@techsingularity.net \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.