Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Fri, Nov 16, 2018 at 07:44:57PM +0100, Michal Hocko wrote:
> On Fri 16-11-18 13:19:04, Sasha Levin wrote:
> > On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
> [...]
> > > > Race condition with memory hotplug due to missing locks:
> > > >
> > > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
> > >
> > > Memory hotplug locking is dubious at best and this patch doesn't really
> > > fix it. It fixes a theoretical problem. I am not aware anybody would be
> > > hitting in practice. We need to rework the locking quite extensively.
> >
> > The word "theoretical" used in the stable rules file does not mean
> > that we need to have actual reports of users hitting bugs before we
> > start backporting the relevant patch, it simply suggests that there
> > needs to be a reasonable explanation of how this issue can be hit.
> >
> > For this memory hotplug patch in particular, I use the hv_balloon driver
> > at this very moment (running a linux guest on windows, with "dynamic
> > memory" enabled). Should I wait for it to crash before I can fix it?
> >
> > Is the upstream code perfect? No, but that doesn't mean that it's not
> > working at all, and if there are users they expect to see fixes going in
> > and not just sitting idly waiting for a big rewrite that will come in a
> > few years.
> >
> > Memory hotplug fixes are not something you think should go to stable?
> > Andrew sent a few of them to stable, so that can't be the case.
>
> I am not arguing about hotplug fixes in general. I was arguing that this
> particular one is a theoretical one and hotplug locking is quite subtle.
> E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
> http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
> So in general unless the issue is really triggered easily I am rather
> conservative.

We have millions of machines running linux, everything is triggered
"easily" at that scale.

> > > > Raising an OOM event that causes issues in userspace when no OOM has
> > > > actually occured:
> > > >
> > > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
> > >
> > > The patch makes sense I just do not think this is a stable material. The
> > > semantic of the event was and still is suboptimal.
> >
> > I really fail to understand your reasoning about -stable here. This
> > patch is something people actually hit in the field, spent time on
> > triaging and analysing it, and submitting a fix which looks reasonably
> > straightforward.
> >
> > That fix was acked by quite a few folks (including yourself) and merged
> > in. And as far as we can tell, it actually fixed the problem.
> >
> > Why is it not stable material?
>
> Because the semantic of the OOM event is quite tricky itself. We have
> discussed this patch and concluded that the updated one is more
> sensible. But it is not yet clear whether this is actually what other
> users expect as well. That to me does sound quite risky for a stable
> kernel.

So there's another patch following this one that fixes it? Sure - can I
take both?

Users expect to not have their containers die randomly, if you're saying
that you're still working on a fix for that then that is a different
story than saying "we fixed it, but it should not go to stable".

And let's also draw a line there, users will not wait for the OOM event
logic to be perfect before they can expect their workloads to run
without issues.

> > My understanding is that you're concerned with the patch itself being
> > "suboptimal", but in that case - why did you ack it?
> >
> > > > I think that all 3 cases represent a "real" bug users can hit, and I
> > > > honestly don't know why they were not tagged for stable.
> > >
> > > It would be much better to ask in the respective email thread rather
> > > than spamming mailing with AUTOSEL patches which rarely get any
> > > attention.
> >
> > I actually tried it, but the comments I got is that it gets in the way
> > and people preferred something they can filter.
>
> which means that AUTOSEL just goes to /dev/null...

Or just not get mixed with the process? for some people it's easier to
see AUTOSEL mails with the way it works now rather than if they suddenly
show up as a continuation of a weeks old thread.

> > > We have been through this discussion several times already and I thought
> > > we have agreed that those subsystems which are seriously considering stable
> > > are opted out from the AUTOSEL automagic. Has anything changed in that
> > > regards.
> >
> > I checked in with Andrew to get his input on this, he suggested that
> > these patches should be sent to linux-mm and he'll give it a close look.
>
> If Andrew is happy to get AUTOSEL patches then I will not object of
> course but let's not merge these patches without and expclicit OK.

This is fair. I think that the process has caused some unnecessary
friction: we all want the same result but just disagree on the means :)

I won't merge any mm/ AUTOSEL patches until this gets clearer.

--
Thanks,
Sasha