On Thu 22-03-18 01:26:13, David Rientjes wrote:
> On Wed, 21 Mar 2018, Michal Hocko wrote:
>
> > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > index d1a917b5b7b7..08accbcd1a18 100644
> > > > --- a/mm/memcontrol.c
> > > > +++ b/mm/memcontrol.c
> > > > @@ -1493,7 +1493,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg)
> > > >
> > > > static void mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order)
> > > > {
> > > > - if (!current->memcg_may_oom)
> > > > + if (!current->memcg_may_oom || order > PAGE_ALLOC_COSTLY_ORDER)
> > > > return;
> > > > /*
> > > > * We are in the middle of the charge context here, so we
> > >
> > > What bug reports have you received about order-4 and higher order non thp
> > > charges that this fixes?
> >
> > We do not have any costly _OOM killable_ allocations but THP AFAIR. Or
> > am I missing any?
> >
>
> So now you're making a generalized policy change to the memcg charge path
> to fix what is obviously only thp and caused by removing the __GFP_NORETRY
> from thp allocations in commit 2516035499b9?
Yes, because relying on __GFP_NORETRY for the oom handling has proven to
be subtle and error prone. And as I've repeated few times already there
is _no_ reason why the oom policy for the memcg charge should be any
different from the allocator's one.
> I don't know what orders
> people enforce for slub_min_order. I assume that people who don't want to
> cause a memcg oom kill are using __GFP_NORETRY because that's how it has
> always worked. The fact that the page allocator got more sophisticated
> logic for the various thp fault and defrag policies doesn't change that.
They simply cannot because kmalloc performs the change under the cover.
So you would have to use kmalloc(gfp|__GFP_NORETRY) to be absolutely
sure to not trigger _any_ oom killer. This is just wrong thing to do.
> You're implementing the exact same behavior that commit 2516035499b9 was
> trying to avoid; it's trying to avoid special-casing thp in general logic.
It is not trying to special case THP. It special cases _all_ costly
charges.
> order > PAGE_ALLOC_COSTLY_ORDER is a terrible heuristic to identify thp
> allocations.
>
> > > PAGE_ALLOC_COSTLY_ORDER is a heuristic used by the page allocator because
> > > it cannot free high-order contiguous memory. Memcg just needs to reclaim
> > > a number of pages. Two order-3 charges can cause a memcg oom kill but now
> > > an order-4 charge cannot. It's an unfair bias against high-order charges
> > > that are not explicitly using __GFP_NORETRY.
> >
> > PAGE_ALLOC_COSTLY_ORDER is documented and people know what to expect
> > from such a request. Diverging from that behavior just comes as a
> > surprise. There is no reason for that and as the above outlines it is
> > error prone.
> >
>
> You're diverging from it because the memcg charge path has never had this
> heuristic.
Which is arguably a bug which just didn't matter because we do not
have costly order oom eligible charges in general and THP was subtly
different and turned out to be error prone.
> I'm somewhat stunned this has to be repeated:
> PAGE_ALLOC_COSTLY_ORDER is about the ability to allocate _contiguous_
> memory, it's not about the _amount_ of memory. Changing the memcg charge
> path to factor order into oom kill decisions is new, and should be
> proposed as a follow-up patch to my bug fix to describe what else is being
> impacted by your patch and what is fixed by it.
>
> Yours is a heuristic change, mine is a bug fix.
Nobody is really arguing about this. I have just pointed out my
reservation that your bug fix is adding more special casing and a more
generic solution is due. If you absolutely believe that your bugfix is
so important to make it to rc7 I will not object. It is however strange
that we haven't seen a _single_ bug report in last two years about this
being a problem. So I am not really sure the urgency is due.
> Look, commit 2516035499b9 pulled off __GFP_NORETRY for GFP_TRANSHUGE and
> forgot to fix it up for memcg charging. I'm setting the bit again to
> prevent the oom kill. It's what should be merged for rc7. I can't make a
> stable case for it because the stable rules want it to impact more than
> one user and I haven't seen other bug reports. It can be backported if
> others are affected to meet the rules.
Exactly, so why the urgency and having half fix that will preserve the
subtle behavior?
> Your change is broken and I wouldn't push it to Linus for rc7 if my life
> depended on it. What is the response when someone complains that they
> start getting a ton of MEMCG_OOM notifications for every thp fallback?
> They will, because yours is a broken implementation.
I fail to see what is broken. Could you be more specific?
> I'm trying to fix the problem introduced by commit 2516035499b9 wrt how
> memcg charges treat high order non-__GFP_NORETRY allocations, and fix it
> directly with something that is obviously right. I'm specifically not
> trying to change heuristics as a bug fix. Please feel free to send a
> follow-up patch for 4.17 that lays out why memcg doesn't want to oom kill
> for order-4 and above (why does memcg fail for 64KB charges when the
> caller specifically left off __GFP_NORETRY again?) as a policy change and
> why that is helpful.
>
> Respectfully, allow the bugfix to fix what was obviously left off from
> commit 2516035499b9.
I haven't nacked the patch AFAIR so nothing really prevents it from
being merged.
--
Michal Hocko
SUSE Labs
