On Tue, Aug 20, 2019 at 9:40 AM Roman Gushchin <guro@xxxxxx> wrote: > > On Tue, Aug 20, 2019 at 09:16:01AM +0800, Yafang Shao wrote: > > On Tue, Aug 20, 2019 at 5:12 AM Roman Gushchin <guro@xxxxxx> wrote: > > > > > > On Sun, Aug 18, 2019 at 09:18:06PM -0400, Yafang Shao wrote: > > > > In the current memory.min design, the system is going to do OOM instead > > > > of reclaiming the reclaimable pages protected by memory.min if the > > > > system is lack of free memory. While under this condition, the OOM > > > > killer may kill the processes in the memcg protected by memory.min. > > > > This behavior is very weird. > > > > In order to make it more reasonable, I make some changes in the OOM > > > > killer. In this patch, the OOM killer will do two-round scan. It will > > > > skip the processes under memcg protection at the first scan, and if it > > > > can't kill any processes it will rescan all the processes. > > > > > > > > Regarding the overhead this change may takes, I don't think it will be a > > > > problem because this only happens under system memory pressure and > > > > the OOM killer can't find any proper victims which are not under memcg > > > > protection. > > > > > > Hi Yafang! > > > > > > The idea makes sense at the first glance, but actually I'm worried > > > about mixing per-memcg and per-process characteristics. > > > Actually, it raises many questions: > > > 1) if we do respect memory.min, why not memory.low too? > > > > memroy.low is different with memory.min, as the OOM killer will not be > > invoked when it is reached. > > If memory.low should be considered as well, we can use > > mem_cgroup_protected() here to repclace task_under_memcg_protection() > > here. > > > > > 2) if the task is 200Gb large, does 10Mb memory protection make any > > > difference? if so, why would we respect it? > > > > Same with above, only consider it when the proctecion is enabled. > > Right, but memory.min is a number, not a boolean flag. It defines > how much memory is protected. You're using it as an on-off knob, > which is sub-optimal from my point of view. > I mean using mem_cgroup_protected(), sam with memory.min is implementad in the global reclaim path. > > > > > 3) if it works for global OOMs, why not memcg-level OOMs? > > > > memcg OOM is when the memory limit is reached and it can't find > > something to relcaim in the memcg and have to kill processes in this > > memcg. > > That is different with global OOM, because the global OOM can chose > > processes outside the memcg but the memcg OOM can't. > > Imagine the following hierarchy: > / > | > A memory.max = 10G, memory.min = 2G > / \ > B C memory.min = 1G, memory.min = 0 > > Say, you have memcg OOM in A, why B's memory min is not respected? > How it's different to the system-wide OOM? > Ah, this should be considered as well. Thanks for pointing out. > > > > > 4) if the task is prioritized to be killed by OOM (via oom_score_adj), > > > why even small memory.protection prevents it completely? > > > > Would you pls. show me some examples that when we will set both > > memory.min(meaning the porcesses in this memcg is very important) and > > higher oom_score_adj(meaning the porcesses in this memcg is not > > improtant at all) ? > > Note that the memory.min don't know which processes is important, > > while it only knows is if this process in this memcg. > > For instance, to prefer a specific process to be killed in case > of memcg OOM. > Also, memory.min can be used mostly to preserve the pagecache, > and an OOM kill means nothing but some anon memory leak. > In this case, it makes no sense to protect the leaked task. > But actually what memory.min protected is the memory usage, instead of pagecache, e.g. if the anon memory is higher than memory.min, then memroy.min can't protect file memory when swap is off. Even there is no anon memory leak, the OOM killer can also be invoked due to excess use of memroy. Plus, the memory.min can also protect the leaked anon memroy in current implementation. > > > > > 5) if there are two tasks similar in size and both protected, > > > should we prefer one with the smaller protection? > > > etc. > > > > Same with the answer in 1). > > So the problem is not that your patch is incorrect (or the idea is bad), > but you're defining a new policy, which will be impossible or very hard > to change further (as any other policy). > > So it's important to define it very well. Using the memory.min > number as a binary flag for selecting tasks seems a bit limited. > > > Thanks!
