On Fri 06-10-17 13:04:35, Roman Gushchin wrote: > On Thu, Oct 05, 2017 at 04:31:04PM +0200, Michal Hocko wrote: > > Btw. here is how I would do the recursive oom badness. The diff is not > > the nicest one because there is some code moving but the resulting code > > is smaller and imho easier to grasp. Only compile tested though > > Thanks! > > I'm not against this approach, and maybe it can lead to a better code, > but the version you sent is just not there yet. > > There are some problems with it: > > 1) If there are nested cgroups with oom_group set, you will calculate > a badness multiple times, and rely on the fact, that top memcg will > become the largest score. It can be optimized, of course, but it's > additional code. right. As I've said we can introduce iterator helper to skip the subtree but I suspect it will not make much of a difference. > > 2) cgroup_has_tasks() probably requires additional locking. > Maybe it's ok to read nr_populated_csets without explicit locking, > but it's not obvious for me. I do not see why. Tasks are free to come and go and you only know at the time you are killing. > 3) Returning -1 from memcg_oom_badness() if eligible is equal to 0 > is suspicious. I didn't spend too much time on it. I merely wanted to point out my thinking more specifically than the pseudo code posted earlier. But this should be ok, because that would mean that either all tasks are OOM_SCORE_ADJ_MIN (eligible = 0) or there is a inflight victim (eligible = -1). Anyway the initialization should go inside the tree walk > Right now your version has exactly the same amount of code > (skipping comments). I assume, this approach just requires some additional > thinking/rework. Well, this is not about the amount of code but more about the clear logic implemented at the correct level. It is simply much easier when you evaluate the killable entity at one place rather open code it. But as I've said nothing I would want to enforce. -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
