Re: [patch 1/2] mm, memcg: avoid oom notification when current needsaccess to memory reserves

[Michal Hocko]

On Tue 03-12-13 15:50:41, David Rientjes wrote:
> On Tue, 3 Dec 2013, Michal Hocko wrote:
> 
> > OK, as it seems that the notification part is too controversial, how
> > would you like the following? It reverts the notification part and still
> > solves the fault on exit path. I will prepare the full patch with the
> > changelog if this looks reasonable:
> 
> Um, no, that's not satisfactory because it obviously does the check after 
> mem_cgroup_oom_notify().  There is absolutely no reason why userspace 
> should be woken up when current simply needs access to memory reserves to 
> exit. 

Let me repeat, that the only reason I liked the patch was that it solves
the fault during exit with oom disabled issue which I am really worried
about.
A nice side effect was that it moves the TIF_MEMDIE logic into a common
place. It seems that you are selling the side effect as a primary
feature.
Johannes is obviously against such a change for the reasons I won't
repeat here again. It is true that such a change wouldn't give us the
"notify only when an action is taken" semantic because oom path might
bail out few more times before killing anything.  Until we have that,
or agree what is the actual semantic that makes the most sense let's
backout with this and fix the actual bug which is real and drop the
tweak that just it only half way.

> You can already get such notification by memory thresholds at the 
> memcg limit.
> 
> I'll repeat: Section 10 of Documentation/cgroups/memory.txt specifies what 
> userspace should do when waking up; one of those options is not "check if 
> the memcg is still actually oom in a short period of time once a charging 
> task with a pending SIGKILL or in the exit path has been able to exit."
> Users of this interface typically also disable the memcg oom killer 
> through the same file, it's ludicrous to put the responsibility on 
> userspace to determine if the wakeup is actionable and requires it to 
> intervene in one of the methods listed in section 10.

David, you would need to show us that such a condition happens in real
loads often enough that such a tweak is worth it. Repeating that a race
exists doesn't help, because yeah it does and it will after your patch
as well. So show us that it happens considerably less often with this
check.
 
[...]
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/