From: Andrew Morton <akpm@linux-foundation.org>
To: "Steven J. Hill" <steven.hill@cavium.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH] mm/vmstat.c: Fix vmstat_update() preemption BUG.
Date: Wed, 21 Mar 2018 16:24:47 -0700 [thread overview]
Message-ID: <20180321162447.ad8990ecb547f0e016d7cd12@linux-foundation.org> (raw)
In-Reply-To: <1520881552-25659-1-git-send-email-steven.hill@cavium.com>
On Mon, 12 Mar 2018 14:05:52 -0500 "Steven J. Hill" <steven.hill@cavium.com> wrote:
> Attempting to hotplug CPUs with CONFIG_VM_EVENT_COUNTERS enabled
> can cause vmstat_update() to report a BUG due to preemption not
> being disabled around smp_processor_id(). Discovered on Ubiquiti
> EdgeRouter Pro with Cavium Octeon II processor.
>
> BUG: using smp_processor_id() in preemptible [00000000] code:
> kworker/1:1/269
> caller is vmstat_update+0x50/0xa0
> CPU: 0 PID: 269 Comm: kworker/1:1 Not tainted
> 4.16.0-rc4-Cavium-Octeon-00009-gf83bbd5-dirty #1
> Workqueue: mm_percpu_wq vmstat_update
> Stack : 0000002600000026 0000000010009ce0 0000000000000000 0000000000000001
> 0000000000000000 0000000000000000 0000000000000005 8001180000000800
> 00000000000000bf 0000000000000000 00000000000000bf 766d737461745f75
> ffffffff83ad0000 0000000000000007 0000000000000000 0000000008000000
> 0000000000000000 ffffffff818d0000 0000000000000001 ffffffff81818a70
> 0000000000000000 0000000000000000 ffffffff8115bbb0 ffffffff818a0000
> 0000000000000005 ffffffff8144dc50 0000000000000000 0000000000000000
> 8000000088980000 8000000088983b30 0000000000000088 ffffffff813d3054
> 0000000000000000 ffffffff83ace622 00000000000000be 0000000000000000
> 00000000000000be ffffffff81121fb4 0000000000000000 0000000000000000
> ...
> Call Trace:
> [<ffffffff81121fb4>] show_stack+0x94/0x128
> [<ffffffff813d3054>] dump_stack+0xa4/0xe0
> [<ffffffff813fcfb8>] check_preemption_disabled+0x118/0x120
> [<ffffffff811eafd8>] vmstat_update+0x50/0xa0
> [<ffffffff8115b954>] process_one_work+0x144/0x348
> [<ffffffff8115bd00>] worker_thread+0x150/0x4b8
> [<ffffffff811622a0>] kthread+0x110/0x140
> [<ffffffff8111c304>] ret_from_kernel_thread+0x14/0x1c
>
> ...
>
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1839,9 +1839,11 @@ static void vmstat_update(struct work_struct *w)
> * to occur in the future. Keep on running the
> * update worker thread.
> */
> + preempt_disable();
> queue_delayed_work_on(smp_processor_id(), mm_percpu_wq,
> this_cpu_ptr(&vmstat_work),
> round_jiffies_relative(sysctl_stat_interval));
> + preempt_enable();
> }
> }
hm, I suspect this warning is a false-positive. vmstat_update() is
called from a workqueue and so vmstat_update()'s execution is pinned to
a particular CPU, so smp_processor_id()'s return will not be
invalidated by a preemption-induced CPU switch. Unless the workqueue
code changed more than I think it did ;)
The patch is OK and will work, but I wonder if a better fix is to use
raw_smp_processor_id() and to add a little comment explaining why it's
needed and is safe.
prev parent reply other threads:[~2018-03-21 23:24 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-12 19:05 Steven J. Hill
2018-03-21 23:24 ` Andrew Morton [this message]
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=20180321162447.ad8990ecb547f0e016d7cd12@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=steven.hill@cavium.com \
--cc=tj@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox