On 11/1/23 15:11, Yosry Ahmed wrote:
On Wed, Nov 1, 2023 at 9:09 AM Waiman Long <longman@xxxxxxxxxx> wrote:
When cgroup_rstat_updated() isn't being called concurrently with
cgroup_rstat_flush_locked(), its run time is pretty short. When
both are called concurrently, the cgroup_rstat_updated() run time
can spike to a pretty high value due to high cpu_lock hold time in
cgroup_rstat_flush_locked(). This can be problematic if the task calling
cgroup_rstat_updated() is a realtime task running on an isolated CPU
with a strict latency requirement. The cgroup_rstat_updated() call can
happens when there is a page fault even though the task is running in
user space most of the time.
The percpu cpu_lock is used to protect the update tree -
updated_next and updated_children. This protection is only needed
when cgroup_rstat_cpu_pop_updated() is being called. The subsequent
flushing operation which can take a much longer time does not need
that protection.
To reduce the cpu_lock hold time, we need to perform all the
cgroup_rstat_cpu_pop_updated() calls up front with the lock
released afterward before doing any flushing. This patch adds a new
cgroup_rstat_flush_list() function to do just that and return a singly
linked list of cgroup_rstat_cpu structures to be flushed.
By adding some instrumentation code to measure the maximum elapsed times
of the new cgroup_rstat_flush_list() function and each cpu iteration
of cgroup_rstat_flush_locked() around the old cpu_lock lock/unlock pair
on a 2-socket x86-64 server running parallel kernel build, the maximum
elapsed times are 31us and 118us respectively. The maximum cpu_lock
hold time is now reduced to about 1/4 of the original.
This sounds promising. It's smart to empty the whole tree while
holding the lock, then do the flush only under cgroup_rstat_lock.
Thanks for doing this.
Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
---
include/linux/cgroup-defs.h | 7 +++++
kernel/cgroup/rstat.c | 57 +++++++++++++++++++++++++++----------
2 files changed, 49 insertions(+), 15 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 265da00a1a8b..22adb94ebb74 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -368,6 +368,13 @@ struct cgroup_rstat_cpu {
*/
struct cgroup *updated_children; /* terminated by self cgroup */
struct cgroup *updated_next; /* NULL iff not on the list */
+
+ /*
+ * A singly-linked list of cgroup_rstat_cpu structures to be flushed.
+ * Protected by cgroup_rstat_lock.
+ */
+ struct cgroup_rstat_cpu *flush_next;
+ struct cgroup *cgroup; /* Cgroup back pointer */
Why are we linking struct cgroup_rstat_cpu instead of directly linking
struct cgroup? AFAICT we only ever use the cgroup back pointer during
flushing anyway, right?
You are right.
Given that only one cpu can be flushed at a time, I think it should be
okay to run the list directly through struct cgroup, and have all cpus
reuse it. That pointer would essentially be scratch space for the
flushing code to use. This should also save a bit of memory:
O(cgroups) vs O(cgroups * cpus). It's not a lot either way though.
I think this may also simplify the code a bit.
Moving the flush_next pointer to struct cgroup does save a bit of
memory. Thanks for the suggestion. I will do that in the next version.
};
struct cgroup_freezer_state {
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index d80d7a608141..93ef2795a68d 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -145,6 +145,42 @@ static struct cgroup *cgroup_rstat_cpu_pop_updated(struct cgroup *pos,
return pos;
}
+/*
+ * Return a list of cgroup_rstat_cpu structures to be flushed
+ */
+static struct cgroup_rstat_cpu *cgroup_rstat_flush_list(struct cgroup *root,
nit: the name sounds like the function will flush a list, rather than
return a list of cgroups to be flushed. What about
cgroup_rstat_updated_list?
I am not good at naming. cgroup_rstat_updated_list looks good to me.
Cheers,
Longman
