+ mm-consider-subtrees-in-memoryevents.patch added to -mm tree
A message that is not in the archive (sent off-list or not yet received): 20190212224542.ZW63a%[email protected]
On Tue 12-02-19 14:45:42, Andrew Morton wrote:
[...]
Quoted 42 lines
From: Chris Down <[email protected]>
Subject: mm, memcg: consider subtrees in memory.eventsmemory.stat and other files already consider subtrees in their output, and we should too in order to not present an inconsistent interface.
The current situation is fairly confusing, because people interacting with cgroups expect hierarchical behaviour in the vein of memory.stat, cgroup.events, and other files. For example, this causes confusion when debugging reclaim events under low, as currently these always read "0" at non-leaf memcg nodes, which frequently causes people to misdiagnose breach behaviour. The same confusion applies to other counters in this file when debugging issues.
Aggregation is done at write time instead of at read-time since these counters aren't hot (unlike memory.stat which is per-page, so it does it at read time), and it makes sense to bundle this with the file notifications.
After this patch, events are propagated up the hierarchy:
[root@ktst ~]# cat
/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 0
oom 0
oom_kill0
[root@ktst ~]# systemd-run -p MemoryMax=1 true Running as unit: run-r251162a189fb4562b9dabfdc9b0422f5.service [root@ktst ~]# cat/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 7
oom 1
oom_kill1As this is a change in behaviour, this can be reverted to the old behaviour by mounting with the `memory_localevents' flag set. However, we use the new behaviour by default as there's a lack of evidence that there are any current users of memory.events that would find this change undesirable.
Quoted 8 lines
Signed-off-by Chris Down <[email protected]>Acked-by Johannes Weiner <[email protected]>Cc Michal Hocko <[email protected]>Cc Tejun Heo <[email protected]>Cc Roman Gushchin <[email protected]>Cc Dennis Zhou <[email protected]>Signed-off-by Andrew Morton <[email protected]>
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
[1] http://lkml.kernel.org/r/[email protected]
Quoted 129 lines
---
Documentation/admin-guide/cgroup-v2.rst | 9 +++++++++ include/linux/cgroup-defs.h | 5 +++++ include/linux/memcontrol.h | 10 ++++++++-- kernel/cgroup/cgroup.c | 16 ++++++++++++++-- 4 files changed, 36 insertions(+), 4 deletions(-)Documentation/admin-guide/cgroup-v2.rst renamed from Documentation/admin-guide/cgroup-v2.rst~mm-consider-subtrees-in-memoryevents +9
-0@@ -177,6 +177,15 @@ cgroup v2 currently supports the followi ignored on non-init namespace mounts. Please refer to the Delegation section for details. + memory_localevents + + Only populate memory.events with data for the current cgroup, + and not any subtrees. This is legacy behaviour, the default + behaviour without this option is to include subtree counts. + This option is system wide and can only be set on mount or + modified through remount from the init namespace. The mount + option is ignored on non-init namespace mounts. + Organizing Processes and Threads --------------------------------include/linux/cgroup-defs.h renamed from include/linux/cgroup-defs.h~mm-consider-subtrees-in-memoryevents +5
-0@@ -83,6 +83,11 @@ enum {* Enable cpuset controller in v1 cgroup to use v2 behavior. */ CGRP_ROOT_CPUSET_V2_MODE = (1 <<4 ), + +/* +* Enable legacy local memory.events. +*/ + CGRP_ROOT_MEMORY_LOCAL_EVENTS = (1 << 5), };/* cftype->flags */ include/linux/memcontrol.h renamed from include/linux/memcontrol.h~mm-consider-subtrees-in-memoryevents +8
-2@@ -789,8 +789,14 @@ static inline void count_memcg_event_mm(static inline void memcg_memory_event (struct mem_cgroup *memcg,enum memcg_memory_event event) { -atomic_long_inc (&memcg->memory_events[event]); -cgroup_file_notify (&memcg->events_file); +do { +atomic_long_inc (&memcg->memory_events[event]); +cgroup_file_notify (&memcg->events_file); + +if (cgrp_dfl_root.flags &CGRP_ROOT_MEMORY_LOCAL_EVENTS ) +break ; + }while ((memcg =parent_mem_cgroup (memcg)) && + !mem_cgroup_is_root (memcg)); } static inline void memcg_memory_event_mm(struct mm_struct *mm,kernel/cgroup/cgroup.c renamed from kernel/cgroup/cgroup.c~mm-consider-subtrees-in-memoryevents +14
-2@@ -1775,11 +1775,13 @@ int cgroup_show_path(struct seq_file *sfenum cgroup2_param { Opt_nsdelegate, + Opt_memory_localevents, nr__cgroup2_params };static const struct fs_parameter_spec cgroup2_param_specs[] = { -fsparam_flag ("nsdelegate" , Opt_nsdelegate), +fsparam_flag ("nsdelegate" , Opt_nsdelegate), +fsparam_flag ("memory_localevents" , Opt_memory_localevents), {} };@@ -1802,6 +1804,9 @@ static int cgroup2_parse_param(struct fscase Opt_nsdelegate: ctx->flags |=CGRP_ROOT_NS_DELEGATE ;return 0 ; +case Opt_memory_localevents: + ctx->flags |=CGRP_ROOT_MEMORY_LOCAL_EVENTS ; +return 0 ; }return -EINVAL ; }@@ -1813,6 +1818,11 @@ static void apply_cgroup_root_flags(unsi cgrp_dfl_root.flags |=CGRP_ROOT_NS_DELEGATE ; else cgrp_dfl_root.flags &= ~CGRP_ROOT_NS_DELEGATE; + +if (root_flags &CGRP_ROOT_MEMORY_LOCAL_EVENTS ) + cgrp_dfl_root.flags |=CGRP_ROOT_MEMORY_LOCAL_EVENTS ; +else + cgrp_dfl_root.flags &= ~CGRP_ROOT_MEMORY_LOCAL_EVENTS ; } }@@ -1820,6 +1830,8 @@ static int cgroup_show_options(struct se {if (cgrp_dfl_root.flags &CGRP_ROOT_NS_DELEGATE )seq_puts (seq,",nsdelegate" ); +if (cgrp_dfl_root.flags &CGRP_ROOT_MEMORY_LOCAL_EVENTS ) +seq_puts (seq,",memory_localevents" );return 0 ; }@@ -6207,7 +6219,7 @@ static struct kobj_attribute cgroup_delestatic ssize_t features_show (struct kobject *kobj,struct kobj_attribute *attr,char *buf) { -return snprintf (buf,PAGE_SIZE ,"nsdelegate\n" ); +return snprintf (buf,PAGE_SIZE ,"nsdelegate\nmemory_localevents\n" ); }static struct kobj_attribute cgroup_features_attr =__ATTR_RO (features);_
Patches currently in -mm which might be from [email protected] are
mm-create-mem_cgroup_from_seq.patch mm-extract-memcg-maxable-seq_file-logic-to-seq_show_memcg_tunable.patch mm-proportional-memorylowmin-reclaim.patch mm-proportional-memorylowmin-reclaim-fix.patch mm-memcontrol-expose-thp-events-on-a-per-memcg-basis.patch mm-memcontrol-expose-thp-events-on-a-per-memcg-basis-fix-2.patch mm-make-memoryemin-the-baseline-for-utilisation-determination.patch mm-rename-ambiguously-named-memorystat-counters-and-functions.patch mm-consider-subtrees-in-memoryevents.patch
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 53 lines · Michal Hocko
On Tue 12-02-19 14:45:42, Andrew Morton wrote:
[...]Quoted 42 lines
From: Chris Down <[email protected]>
Subject: mm, memcg: consider subtrees in memory.eventsmemory.stat and other files already consider subtrees in their output, and we should too in order to not present an inconsistent interface.
The current situation is fairly confusing, because people interacting with cgroups expect hierarchical behaviour in the vein of memory.stat, cgroup.events, and other files. For example, this causes confusion when debugging reclaim events under low, as currently these always read "0" at non-leaf memcg nodes, which frequently causes people to misdiagnose breach behaviour. The same confusion applies to other counters in this file when debugging issues.
Aggregation is done at write time instead of at read-time since these counters aren't hot (unlike memory.stat which is per-page, so it does it at read time), and it makes sense to bundle this with the file notifications.
After this patch, events are propagated up the hierarchy:
[root@ktst ~]# cat
/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 0
oom 0
oom_kill0
[root@ktst ~]# systemd-run -p MemoryMax=1 true Running as unit: run-r251162a189fb4562b9dabfdc9b0422f5.service [root@ktst ~]# cat/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 7
oom 1
oom_kill1As this is a change in behaviour, this can be reverted to the old behaviour by mounting with the `memory_localevents' flag set. However, we use the new behaviour by default as there's a lack of evidence that there are any current users of memory.events that would find this change undesirable.
Quoted 8 lines
Signed-off-by Chris Down <[email protected]>Acked-by Johannes Weiner <[email protected]>Cc Michal Hocko <[email protected]>Cc Tejun Heo <[email protected]>Cc Roman Gushchin <[email protected]>Cc Dennis Zhou <[email protected]>Signed-off-by Andrew Morton <[email protected]>
Quoted 5 lines
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
It's been three months without any objections. Can we merge this for v5.2 please? We still have users complaining about this inconsistent behavior (the last one was yesterday) and we'd rather not carry any out of tree patches.
Quoted 54 lines · Johannes Weiner
Quoted 53 lines · Michal Hocko
On Tue 12-02-19 14:45:42, Andrew Morton wrote:
[...]Quoted 50 lines
From: Chris Down <[email protected]>
Subject: mm, memcg: consider subtrees in memory.eventsmemory.stat and other files already consider subtrees in their output, and we should too in order to not present an inconsistent interface.
The current situation is fairly confusing, because people interacting with cgroups expect hierarchical behaviour in the vein of memory.stat, cgroup.events, and other files. For example, this causes confusion when debugging reclaim events under low, as currently these always read "0" at non-leaf memcg nodes, which frequently causes people to misdiagnose breach behaviour. The same confusion applies to other counters in this file when debugging issues.
Aggregation is done at write time instead of at read-time since these counters aren't hot (unlike memory.stat which is per-page, so it does it at read time), and it makes sense to bundle this with the file notifications.
After this patch, events are propagated up the hierarchy:
[root@ktst ~]# cat
/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 0
oom 0
oom_kill0
[root@ktst ~]# systemd-run -p MemoryMax=1 true Running as unit: run-r251162a189fb4562b9dabfdc9b0422f5.service [root@ktst ~]# cat/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 7
oom 1
oom_kill1As this is a change in behaviour, this can be reverted to the old behaviour by mounting with the `memory_localevents' flag set. However, we use the new behaviour by default as there's a lack of evidence that there are any current users of memory.events that would find this change undesirable.
Signed-off-by Chris Down <[email protected]>Acked-by Johannes Weiner <[email protected]>Cc Michal Hocko <[email protected]>Cc Tejun Heo <[email protected]>Cc Roman Gushchin <[email protected]>Cc Dennis Zhou <[email protected]>Signed-off-by Andrew Morton <[email protected]>
Quoted 7 lines
Quoted 5 lines
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
It's been three months without any objections.
It's been three months without any _feedback_ from anybody. It might very well be true that people just do not read these emails or do not care one way or another.
Quoted 4 lines
Can we merge this for
v5.2 please? We still have users complaining about this inconsistent behavior (the last one was yesterday) and we'd rather not carry any out of tree patches.
Could you point me to those complains or is this something internal?
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 59 lines · Michal Hocko
Quoted 58 lines · Johannes Weiner
Quoted 57 lines · Michal Hocko
On Tue 12-02-19 14:45:42, Andrew Morton wrote:
[...]Quoted 42 lines
From: Chris Down <[email protected]>
Subject: mm, memcg: consider subtrees in memory.eventsmemory.stat and other files already consider subtrees in their output, and we should too in order to not present an inconsistent interface.
The current situation is fairly confusing, because people interacting with cgroups expect hierarchical behaviour in the vein of memory.stat, cgroup.events, and other files. For example, this causes confusion when debugging reclaim events under low, as currently these always read "0" at non-leaf memcg nodes, which frequently causes people to misdiagnose breach behaviour. The same confusion applies to other counters in this file when debugging issues.
Aggregation is done at write time instead of at read-time since these counters aren't hot (unlike memory.stat which is per-page, so it does it at read time), and it makes sense to bundle this with the file notifications.
After this patch, events are propagated up the hierarchy:
[root@ktst ~]# cat
/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 0
oom 0
oom_kill0
[root@ktst ~]# systemd-run -p MemoryMax=1 true Running as unit: run-r251162a189fb4562b9dabfdc9b0422f5.service [root@ktst ~]# cat/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 7
oom 1
oom_kill1As this is a change in behaviour, this can be reverted to the old behaviour by mounting with the `memory_localevents' flag set. However, we use the new behaviour by default as there's a lack of evidence that there are any current users of memory.events that would find this change undesirable.
Quoted 8 lines
Signed-off-by Chris Down <[email protected]>Acked-by Johannes Weiner <[email protected]>Cc Michal Hocko <[email protected]>Cc Tejun Heo <[email protected]>Cc Roman Gushchin <[email protected]>Cc Dennis Zhou <[email protected]>Signed-off-by Andrew Morton <[email protected]>FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
It's been three months without any objections.
It's been three months without any _feedback_ from anybody. It might very well be true that people just do not read these emails or do not care one way or another.
This is exactly the type of stuff that Mel was talking about at LSFMM not even two weeks ago. How one objection, however absurd, can cause "controversy" and block an effort to address a mistake we have made in the past that is now actively causing problems for real users.
And now after stalling this fix for three months to wait for unlikely objections, you're moving the goal post. This is frustrating.
Nobody else is speaking up because the current user base is very small and because the idea that anybody has developed against and is relying on the current problematic behavior is completely contrived. In reality, the behavior surprises people and causes production issues.
Quoted 6 lines
Quoted 4 lines
Can we merge this for
v5.2 please? We still have users complaining about this inconsistent behavior (the last one was yesterday) and we'd rather not carry any out of tree patches.Could you point me to those complains or is this something internal?
It's something internal, unfortunately, or I'd link to it.
In this report yesterday, the user missed OOM kills that occured in nested subgroups of individual job components. They monitor the entire job status and health at the top-level "job" cgroup: total memory usage, VM activity and trends from memory.stat, pressure for cpu, io, memory etc. All of these are recursive. They assumed they could monitor memory.events likewise and were left in the assumption that everything was fine when in reality there was OOM killing going on in one of the leaves.
Such negative surprises really suck. But what's worse is that now that they are aware of it, there is still no good solution for them because periodically polling the entire subtree for events in leaves is not practical. There could be a lot of cgroups, which is why we put so much effort recently into improving the hierarchical stat aggregation.
I'd really like to get this fixed, and preferably in a way that does deviate from upstream, and does not force the same downtimes and wasted engineering hours on everybody who is going to switch to cgroup2 in the next couple of years.
Quoted 3 lines · Johannes Weiner
Quoted 2 lines · Michal Hocko
Quoted 1 line · Johannes Weiner
On Wed, Feb 13, 2019 at 01:47:29PM +0100, Michal Hocko wrote:
[...]
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Quoted 5 lines
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
It's been three months without any objections.
It's been three months without any _feedback_ from anybody. It might very well be true that people just do not read these emails or do not care one way or another.
Quoted 7 lines
This is exactly the type of stuff that Mel was talking about at LSFMM not even two weeks ago. How one objection, however absurd, can cause "controversy" and block an effort to address a mistake we have made in the past that is now actively causing problems for real users.
And now after stalling this fix for three months to wait for unlikely objections, you're moving the goal post. This is frustrating.
I see your frustration but I find the above wording really unfair. Let me remind you that this is a considerable user visible change in the semantic and that always has to be evaluated carefuly. A change that would clearly regress anybody who rely on the current semantic. This is not an internal implementation detail kinda thing.
I have suggested an option for the new behavior to be opt-in which would be a regression safe option. You keep insisting that we absolutely have to have hierarchical reporting by default for consistency reasons. I do understand that argument but when I weigh consistency vs. potential regression risk I rather go a conservative way. This is a traditional way how we deal with semantic changes like this. There are always exceptions possible and that is why I wanted to hear from other users of cgroup v2, even from those who are not directly affected now.
If you feel so stronly about this topic and the suggested opt-in is an absolute no-go then you are free to override my opinion here. I haven't Nacked this patch.
Quoted 4 lines
Nobody else is speaking up because the current user base is very small and because the idea that anybody has developed against and is relying on the current problematic behavior is completely contrived. In reality, the behavior surprises people and causes production issues.
I strongly suspect users usually do not follow discussions on our mailing lists. They only come up later when something breaks and that is too late. I do realize that this makes the above call for a wider consensus harder but a lack of upstream bug reports also suggests that people do not care or simply haven't noticed any issues due to way how they use the said interface (maybe deeper hierarchies are not that common).
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Can we merge this for
v5.2 please? We still have users complaining about this inconsistent behavior (the last one was yesterday) and we'd rather not carry any out of tree patches.Could you point me to those complains or is this something internal?
It's something internal, unfortunately, or I'd link to it.
Quoted 8 lines
In this report yesterday, the user missed OOM kills that occured in nested subgroups of individual job components. They monitor the entire job status and health at the top-level "job" cgroup: total memory usage, VM activity and trends from memory.stat, pressure for cpu, io, memory etc. All of these are recursive. They assumed they could monitor memory.events likewise and were left in the assumption that everything was fine when in reality there was OOM killing going on in one of the leaves.
This kind of argument has been already mentioned during the discussion and I understand it.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 50 lines · Michal Hocko
Quoted 3 lines · Johannes Weiner
Quoted 2 lines · Michal Hocko
Quoted 1 line · Johannes Weiner
On Wed, Feb 13, 2019 at 01:47:29PM +0100, Michal Hocko wrote:
[...]
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Quoted 5 lines
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
It's been three months without any objections.
It's been three months without any _feedback_ from anybody. It might very well be true that people just do not read these emails or do not care one way or another.
Quoted 7 lines
This is exactly the type of stuff that Mel was talking about at LSFMM not even two weeks ago. How one objection, however absurd, can cause "controversy" and block an effort to address a mistake we have made in the past that is now actively causing problems for real users.
And now after stalling this fix for three months to wait for unlikely objections, you're moving the goal post. This is frustrating.
I see your frustration but I find the above wording really unfair. Let me remind you that this is a considerable user visible change in the semantic and that always has to be evaluated carefuly. A change that would clearly regress anybody who rely on the current semantic. This is not an internal implementation detail kinda thing.
I have suggested an option for the new behavior to be opt-in which would be a regression safe option. You keep insisting that we absolutely have to have hierarchical reporting by default for consistency reasons. I do understand that argument but when I weigh consistency vs. potential regression risk I rather go a conservative way. This is a traditional way how we deal with semantic changes like this. There are always exceptions possible and that is why I wanted to hear from other users of cgroup v2, even from those who are not directly affected now.
If you feel so stronly about this topic and the suggested opt-in is an absolute no-go then you are free to override my opinion here. I haven't Nacked this patch.
Quoted 4 lines
Nobody else is speaking up because the current user base is very small and because the idea that anybody has developed against and is relying on the current problematic behavior is completely contrived. In reality, the behavior surprises people and causes production issues.
Quoted 8 lines
I strongly suspect users usually do not follow discussions on our mailing lists. They only come up later when something breaks and that is too late. I do realize that this makes the above call for a wider consensus harder but a lack of upstream bug reports also suggests that people do not care or simply haven't noticed any issues due to way how they use the said interface (maybe deeper hierarchies are not that common).
I suspect that FB is the only one using cgroup v2 in production and others (data center) users are still evaluating/exploring. Also IMHO the cgroup v2 users are on the bleeding edge. As new cgroup v2 features and controllers are added, the users either switch to latest kernel or backport. That might be the reason no one objected. Also none of the distribution has defaulted to v2 yet, so, not many transparent v2 users yet.
Shakeel
Quoted 61 lines · Shakeel Butt
Quoted 50 lines · Michal Hocko
Quoted 3 lines · Johannes Weiner
Quoted 2 lines · Michal Hocko
Quoted 1 line · Johannes Weiner
On Wed, Feb 13, 2019 at 01:47:29PM +0100, Michal Hocko wrote:
[...]
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Quoted 5 lines
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
It's been three months without any objections.
It's been three months without any _feedback_ from anybody. It might very well be true that people just do not read these emails or do not care one way or another.
Quoted 7 lines
This is exactly the type of stuff that Mel was talking about at LSFMM not even two weeks ago. How one objection, however absurd, can cause "controversy" and block an effort to address a mistake we have made in the past that is now actively causing problems for real users.
And now after stalling this fix for three months to wait for unlikely objections, you're moving the goal post. This is frustrating.
I see your frustration but I find the above wording really unfair. Let me remind you that this is a considerable user visible change in the semantic and that always has to be evaluated carefuly. A change that would clearly regress anybody who rely on the current semantic. This is not an internal implementation detail kinda thing.
I have suggested an option for the new behavior to be opt-in which would be a regression safe option. You keep insisting that we absolutely have to have hierarchical reporting by default for consistency reasons. I do understand that argument but when I weigh consistency vs. potential regression risk I rather go a conservative way. This is a traditional way how we deal with semantic changes like this. There are always exceptions possible and that is why I wanted to hear from other users of cgroup v2, even from those who are not directly affected now.
If you feel so stronly about this topic and the suggested opt-in is an absolute no-go then you are free to override my opinion here. I haven't Nacked this patch.
Quoted 4 lines
Nobody else is speaking up because the current user base is very small and because the idea that anybody has developed against and is relying on the current problematic behavior is completely contrived. In reality, the behavior surprises people and causes production issues.
Quoted 8 lines
I strongly suspect users usually do not follow discussions on our mailing lists. They only come up later when something breaks and that is too late. I do realize that this makes the above call for a wider consensus harder but a lack of upstream bug reports also suggests that people do not care or simply haven't noticed any issues due to way how they use the said interface (maybe deeper hierarchies are not that common).
Quoted 7 lines
I suspect that FB is the only one using cgroup v2 in production and others (data center) users are still evaluating/exploring. Also IMHO the cgroup v2 users are on the bleeding edge. As new cgroup v2 features and controllers are added, the users either switch to latest kernel or backport. That might be the reason no one objected. Also none of the distribution has defaulted to v2 yet, so, not many transparent v2 users yet.
In Android we are not using cgroups v2 yet (and that's why I was refraining from commenting earlier), however when I was evaluating them for future use I was disappointed that events do not propagate up the hierarchy. One usecase that I was considering is to get a notification when OOM kill happens. With cgroups v2 we would be forced to use per-app hierarchy to avoid process migrations between memcgs when an app changes its state (background/foreground). With such a setup we would end up with many leaf cgroups. Polling each individual leaf cgroup's memory.events file to detect OOM occurrence would require lots of extra FDs registered with an epoll(). Having an ability to poll a common parent cgroup to detect that one of the leafs generated an OOM event would be way more frugal.
I realize this does not constitute a real-life usecase but hopefully possible usecases can provide some value too.
Thanks,
Suren.
Quoted 2 lines
Shakeel
Quoted 31 lines · Michal Hocko
Quoted 3 lines · Johannes Weiner
Quoted 2 lines · Michal Hocko
Quoted 1 line · Johannes Weiner
On Wed, Feb 13, 2019 at 01:47:29PM +0100, Michal Hocko wrote:
[...]
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Quoted 5 lines
FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
It's been three months without any objections.
It's been three months without any _feedback_ from anybody. It might very well be true that people just do not read these emails or do not care one way or another.
Quoted 7 lines
This is exactly the type of stuff that Mel was talking about at LSFMM not even two weeks ago. How one objection, however absurd, can cause "controversy" and block an effort to address a mistake we have made in the past that is now actively causing problems for real users.
And now after stalling this fix for three months to wait for unlikely objections, you're moving the goal post. This is frustrating.
I see your frustration but I find the above wording really unfair. Let me remind you that this is a considerable user visible change in the semantic and that always has to be evaluated carefuly. A change that would clearly regress anybody who rely on the current semantic. This is not an internal implementation detail kinda thing.
Quoted 8 lines
I have suggested an option for the new behavior to be opt-in which would be a regression safe option. You keep insisting that we absolutely have to have hierarchical reporting by default for consistency reasons. I do understand that argument but when I weigh consistency vs. potential regression risk I rather go a conservative way. This is a traditional way how we deal with semantic changes like this. There are always exceptions possible and that is why I wanted to hear from other users of cgroup v2, even from those who are not directly affected now.
I have acknowledged this concern in previous discussions. But the rule is "don't break userspace", not "never change behavior". We do allow the latter when it's highly unlikely that anyone would mind and the new behavior is a much better default for current and future users.
Let me try to make the case for exactly this:
- Adoption data suggests that cgroup2 isn't really used yet. RHEL8 was just released with cgroup1 per default. Fedora is currently debating a switch. None of the other distros default to cgroup2. There is an article on the lwn frontpage *right now* about Docker planning on switching to cgroup2 in the near future. Kubernetes is on cgroup1. Android is on cgroup1. Shakeel agrees that Facebook is probably the only serious user of cgroup2 right now. The cloud and all mainstream container software is still on cgroup1.
- Using this particular part of the interface is a fairly advanced step in the cgroup2 adoption process. We've been using cgroup2 for a while and we've only now started running into this memory.events problem as we're enhancing our monitoring and automation infrastructure. If we're the only serious deployment, and we just started noticing it, what's the chance of regressing someone else?
- Violating expectations costs users time and money either way, but the status quo is much more costly: somebody who expects these events to be local could see events that did occur at an unexpectedly higher level of the tree. But somebody who expects these events to be hierarchical will miss real events entirely!
Now, for an alarm and monitoring infrastructure, what is worse: to see occurring OOM kills reported at a tree level you don't expect? Or to *miss* occurring OOM kills that you're trying to look out for?
Automatic remediation might not be as clear-cut, but for us, and I would assume many others, an unnecessary service restart or failover would have a shorter downtime than missing a restart after a kill.
- The status quo is more likely to violate expectations, given how the cgroup2 interface as a whole is designed.
We have seen this in practice: memory.current is hierarchical, memory.stat is hierarchical, memory.pressure is hierarchical - users expect memory.events to be hierarchical. This misunderstanding has already cost us time and money.
Chances are, even if there were other users of memory.events, that they're using the interface incorrectly and haven't noticed yet, rather than relying on the inconsistency.
It's not a hypothetical, we have seen this with our fleet customers.
So combining what we know about
1. the current adoption rate
2. likely user expectations
3. the failure mode of missing pressure and OOM kill signals
means that going with the conservative option and not fixing this inconsistency puts pretty much all users that will ever use this interface at the risk of pain, outages and wasted engineering hours.
Making the fix available but opt-in has the same result for everybody that isn't following this thread/patch along.
All that to protect an unlikely existing cgroup2 user from something they are even less likely have noticed, let alone rely on.
This sounds like a terrible trade-off to me. I don't think it's a close call in this case.
I understand that we have to think harder and be more careful with changes like this. The bar *should* be high. But in this case, there doesn't seem to be a real risk of regression for anybody, while the risk of the status quo causing problems is high and happening. These circumstances should be part of the decision process.
Andrew Morton
maintainerQuoted 8 lines · On Fri, 17 May 2019 21:33:48 -0400 Johannes Weiner <[email protected]> wrote:
- Adoption data suggests that cgroup2 isn't really used yet. RHEL8 was just released with cgroup1 per default. Fedora is currently debating a switch. None of the other distros default to cgroup2. There is an article on the lwn frontpage *right now* about Docker planning on switching to cgroup2 in the near future. Kubernetes is on cgroup1. Android is on cgroup1. Shakeel agrees that Facebook is probably the only serious user of cgroup2 right now. The cloud and all mainstream container software is still on cgroup1.
I'm thinking we need a cc:stable so these forthcoming distros are more likely to pick up the new behaviour?
Generally, your arguments sound good to me - I don't see evidence that anyone is using cgroup2 in a manner which is serious enough to be seriously affected by this change.
Quoted 11 lines · Andrew Morton
Quoted 8 lines · On Fri, 17 May 2019 21:33:48 -0400 Johannes Weiner <[email protected]> wrote:
- Adoption data suggests that cgroup2 isn't really used yet. RHEL8 was just released with cgroup1 per default. Fedora is currently debating a switch. None of the other distros default to cgroup2. There is an article on the lwn frontpage *right now* about Docker planning on switching to cgroup2 in the near future. Kubernetes is on cgroup1. Android is on cgroup1. Shakeel agrees that Facebook is probably the only serious user of cgroup2 right now. The cloud and all mainstream container software is still on cgroup1.
Quoted 2 lines
I'm thinking we need a cc:stable so these forthcoming distros are more likely to pick up the new behaviour?
Yup, makes sense to me. Thank you!
Shakeel Butt
Reviewed-byQuoted 20 lines · Michal Hocko
On Tue 12-02-19 14:45:42, Andrew Morton wrote:
[...]Quoted 17 lines
From: Chris Down <[email protected]>
Subject: mm, memcg: consider subtrees in memory.eventsmemory.stat and other files already consider subtrees in their output, and we should too in order to not present an inconsistent interface.
The current situation is fairly confusing, because people interacting with cgroups expect hierarchical behaviour in the vein of memory.stat, cgroup.events, and other files. For example, this causes confusion when debugging reclaim events under low, as currently these always read "0" at non-leaf memcg nodes, which frequently causes people to misdiagnose breach behaviour. The same confusion applies to other counters in this file when debugging issues.
Aggregation is done at write time instead of at read-time since these counters aren't hot (unlike memory.stat which is per-page, so it does it at read time), and it makes sense to bundle this with the file
I think the above statement (memory.stat read-time aggregation) need to be fixed due to the latest changes.
Quoted 25 lines
Quoted 25 lines
notifications.
After this patch, events are propagated up the hierarchy:
[root@ktst ~]# cat
/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 0
oom 0
oom_kill0
[root@ktst ~]# systemd-run -p MemoryMax=1 true Running as unit: run-r251162a189fb4562b9dabfdc9b0422f5.service [root@ktst ~]# cat/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 7
oom 1
oom_kill1As this is a change in behaviour, this can be reverted to the old behaviour by mounting with the `memory_localevents' flag set. However, we use the new behaviour by default as there's a lack of evidence that there are any current users of memory.events that would find this change undesirable.
Quoted 3 lines
Quoted 3 lines
Signed-off-by Chris Down <[email protected]>Acked-by Johannes Weiner <[email protected]>
However can we please have memory.events.local merged along with this one?
Quoted 145 lines
Quoted 5 lines
Cc Michal Hocko <[email protected]>Cc Tejun Heo <[email protected]>Cc Roman Gushchin <[email protected]>Cc Dennis Zhou <[email protected]>Signed-off-by Andrew Morton <[email protected]>FTR: As I've already said here [1] I can live with this change as long as there is a larger consensus among cgroup v2 users. So let's give this some more time before merging to see whether there is such a consensus.
[1] http://lkml.kernel.org/r/[email protected]
Quoted 120 lines
---
Documentation/admin-guide/cgroup-v2.rst | 9 +++++++++ include/linux/cgroup-defs.h | 5 +++++ include/linux/memcontrol.h | 10 ++++++++-- kernel/cgroup/cgroup.c | 16 ++++++++++++++-- 4 files changed, 36 insertions(+), 4 deletions(-)Documentation/admin-guide/cgroup-v2.rst renamed from Documentation/admin-guide/cgroup-v2.rst~mm-consider-subtrees-in-memoryevents +9
-0@@ -177,6 +177,15 @@ cgroup v2 currently supports the followi ignored on non-init namespace mounts. Please refer to the Delegation section for details. + memory_localevents + + Only populate memory.events with data for the current cgroup, + and not any subtrees. This is legacy behaviour, the default + behaviour without this option is to include subtree counts. + This option is system wide and can only be set on mount or + modified through remount from the init namespace. The mount + option is ignored on non-init namespace mounts. + Organizing Processes and Threads --------------------------------include/linux/cgroup-defs.h renamed from include/linux/cgroup-defs.h~mm-consider-subtrees-in-memoryevents +5
-0@@ -83,6 +83,11 @@ enum {* Enable cpuset controller in v1 cgroup to use v2 behavior. */ CGRP_ROOT_CPUSET_V2_MODE = (1 <<4 ), + +/* +* Enable legacy local memory.events. +*/ + CGRP_ROOT_MEMORY_LOCAL_EVENTS = (1 << 5), };/* cftype->flags */ include/linux/memcontrol.h renamed from include/linux/memcontrol.h~mm-consider-subtrees-in-memoryevents +8
-2@@ -789,8 +789,14 @@ static inline void count_memcg_event_mm(static inline void memcg_memory_event (struct mem_cgroup *memcg,enum memcg_memory_event event) { -atomic_long_inc (&memcg->memory_events[event]); -cgroup_file_notify (&memcg->events_file); +do { +atomic_long_inc (&memcg->memory_events[event]); +cgroup_file_notify (&memcg->events_file); + +if (cgrp_dfl_root.flags &CGRP_ROOT_MEMORY_LOCAL_EVENTS ) +break ; + }while ((memcg =parent_mem_cgroup (memcg)) && + !mem_cgroup_is_root (memcg)); } static inline void memcg_memory_event_mm(struct mm_struct *mm,kernel/cgroup/cgroup.c renamed from kernel/cgroup/cgroup.c~mm-consider-subtrees-in-memoryevents +14
-2@@ -1775,11 +1775,13 @@ int cgroup_show_path(struct seq_file *sfenum cgroup2_param { Opt_nsdelegate, + Opt_memory_localevents, nr__cgroup2_params };static const struct fs_parameter_spec cgroup2_param_specs[] = { -fsparam_flag ("nsdelegate" , Opt_nsdelegate), +fsparam_flag ("nsdelegate" , Opt_nsdelegate), +fsparam_flag ("memory_localevents" , Opt_memory_localevents), {} };@@ -1802,6 +1804,9 @@ static int cgroup2_parse_param(struct fscase Opt_nsdelegate: ctx->flags |=CGRP_ROOT_NS_DELEGATE ;return 0 ; +case Opt_memory_localevents: + ctx->flags |=CGRP_ROOT_MEMORY_LOCAL_EVENTS ; +return 0 ; }return -EINVAL ; }@@ -1813,6 +1818,11 @@ static void apply_cgroup_root_flags(unsi cgrp_dfl_root.flags |=CGRP_ROOT_NS_DELEGATE ; else cgrp_dfl_root.flags &= ~CGRP_ROOT_NS_DELEGATE; + +if (root_flags &CGRP_ROOT_MEMORY_LOCAL_EVENTS ) + cgrp_dfl_root.flags |=CGRP_ROOT_MEMORY_LOCAL_EVENTS ; +else + cgrp_dfl_root.flags &= ~CGRP_ROOT_MEMORY_LOCAL_EVENTS ; } }@@ -1820,6 +1830,8 @@ static int cgroup_show_options(struct se {if (cgrp_dfl_root.flags &CGRP_ROOT_NS_DELEGATE )seq_puts (seq,",nsdelegate" ); +if (cgrp_dfl_root.flags &CGRP_ROOT_MEMORY_LOCAL_EVENTS ) +seq_puts (seq,",memory_localevents" );return 0 ; }@@ -6207,7 +6219,7 @@ static struct kobj_attribute cgroup_delestatic ssize_t features_show (struct kobject *kobj,struct kobj_attribute *attr,char *buf) { -return snprintf (buf,PAGE_SIZE ,"nsdelegate\n" ); +return snprintf (buf,PAGE_SIZE ,"nsdelegate\nmemory_localevents\n" ); }static struct kobj_attribute cgroup_features_attr =__ATTR_RO (features);_
Patches currently in -mm which might be from [email protected] are
Quoted 9 lines
mm-create-mem_cgroup_from_seq.patch mm-extract-memcg-maxable-seq_file-logic-to-seq_show_memcg_tunable.patch mm-proportional-memorylowmin-reclaim.patch mm-proportional-memorylowmin-reclaim-fix.patch mm-memcontrol-expose-thp-events-on-a-per-memcg-basis.patch mm-memcontrol-expose-thp-events-on-a-per-memcg-basis-fix-2.patch mm-make-memoryemin-the-baseline-for-utilisation-determination.patch mm-rename-ambiguously-named-memorystat-counters-and-functions.patch mm-consider-subtrees-in-memoryevents.patch
--
Michal Hocko
SUSE Labs
Quoted 26 lines · Shakeel Butt
Quoted 25 lines · Michal Hocko
Quoted 25 lines
notifications.
After this patch, events are propagated up the hierarchy:
[root@ktst ~]# cat
/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 0
oom 0
oom_kill0
[root@ktst ~]# systemd-run -p MemoryMax=1 true Running as unit: run-r251162a189fb4562b9dabfdc9b0422f5.service [root@ktst ~]# cat/sys/fs/cgroup/system.slice/memory.eventslow 0
high 0
max 7
oom 1
oom_kill1As this is a change in behaviour, this can be reverted to the old behaviour by mounting with the `memory_localevents' flag set. However, we use the new behaviour by default as there's a lack of evidence that there are any current users of memory.events that would find this change undesirable.
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Signed-off-by Chris Down <[email protected]>Acked-by Johannes Weiner <[email protected]>Reviewed-by Shakeel Butt <[email protected]>
Thanks, Shakeel.
Quoted 1 line
However can we please have memory.events.local merged along with this one?
Could I ask you to send a patch for this? It's not really about the code - that should be trivial. Rather it's about laying out the exact usecase for that, which is harder for me/Chris/FB since we don't have one. I imagine simliar arguments could be made for memory.stat.local, memory.pressure.local etc. since they're also reporting events and behavior manifesting in different levels of the cgroup subtree?