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
