From: Vlastimil Babka <email@example.com>
To: Sebastian Andrzej Siewior <firstname.lastname@example.org>,
Andrew Morton <email@example.com>
Cc: firstname.lastname@example.org, email@example.com,
firstname.lastname@example.org, "Steven J . Hill" <email@example.com>,
Tejun Heo <firstname.lastname@example.org>, Christoph Lameter <email@example.com>
Subject: Re: [PATCH REPOST] Revert mm/vmstat.c: fix vmstat_update() preemption BUG
Date: Thu, 10 May 2018 08:32:50 +0200 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
On 05/10/2018 12:35 AM, Sebastian Andrzej Siewior wrote:
> On 2018-05-08 16:02:57 [-0700], Andrew Morton wrote:
>> On Mon, 7 May 2018 09:31:05 +0200 Vlastimil Babka <email@example.com> wrote:
>>> In any case I agree that the revert should be done immediately even
>>> before fixing the underlying bug. The preempt_disable/enable doesn't
>>> prevent the bug, it only prevents the debugging code from actually
>>> reporting it! Note that it's debugging code (CONFIG_DEBUG_PREEMPT) that
>>> production kernels most likely don't have enabled, so we are not even
>>> helping them not crash (while allowing possible data corruption).
>> I don't see much benefit in emitting warnings into end-users' logs for
>> bugs which we already know about.
> not end-users (not to mention that neither Debian Stretch nor F28 has
> preemption enabled in their kernels). And if so, they may provide
> additional information for someone to fix the bug in the end. I wasn't
Even if end-users have enabled preemption, they likely won't have
enabled CONFIG_DEBUG_PREEMPT anyway.
> able to reproduce the bug but I don't have access to anything MIPSish
> where I can boot my own kernels. At least two people were looking at the
> code after I posted the revert and nobody spotted the bug.
>> The only thing this buys us is that people will hassle us if we forget
>> to fix the bug, and how pathetic is that? I mean, we may as well put
>> printk("don't forget to fix the vmstat_update() bug!\n");
> No that is different. That would be seen by everyone. The bug was only
> reported by Steven J. Hill which did not respond since. This message
> would also imply that we know how to fix the bug but didn't do it yet
> which is not the case. We seen that something was wrong but have no idea
> *how* it got there.
> The preempt_disable() was added by the end of v4.16. The
> smp_processor_id() in vmstat_update() was added in commit 7cc36bbddde5
> ("vmstat: on-demand vmstat workers V8") which was in v3.18-rc1. The
> hotplug rework took place in v4.10-rc1. And it took (counting from the
> hotplug rework) 6 kernel releases for someone to trigger that warning
> _if_ this was related to the hotplug rework.
> What we have *now* is way worse: We have a possible bug that triggered
> the warning. As we see in report the code in question was _already_
> invoked on the wrong CPU. The preempt_disable() just silences the
> warning, hiding the real issue so nobody will do a thing about it since
> it will be never reported again (in a kernel with preemption and debug
Fully agree with everything you said!
We could extend the warning to e.g. print affinity mask of the thread,
and e.g. state of cpus that are subject to ongoing hotplug/hotremove.
But maybe it's not so useful in general, as the common case is likely
indeed a missing preempt_disable, and this is an exception? In any case,
I would hope that Steven applies some patch locally and we get more
details about what's going on at that MIPS machine.
next prev parent reply other threads:[~2018-05-10 6:32 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-04 10:44 [PATCH REPOST] Revert mm/vmstat.c: fix vmstat_update() preemption BUG Sebastian Andrzej Siewior
2018-05-07 7:31 ` Vlastimil Babka
2018-05-08 23:02 ` Andrew Morton
2018-05-09 22:35 ` Sebastian Andrzej Siewior
2018-05-10 6:32 ` Vlastimil Babka [this message]
2018-06-13 21:46 ` Thomas Gleixner
2018-06-14 21:27 ` Andrew Morton
2018-06-27 19:40 ` Steven Rostedt
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* 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.