On Sat, Aug 27, 2022 at 1:06 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > The cgroup_hierarchical_stats selftest is complicated. It has to be, > because it tests an entire workflow of recording, aggregating, and > dumping cgroup stats. However, some of the complexity is unnecessary. > The test now enables the memory controller in a cgroup hierarchy, invokes > reclaim, measure reclaim time, THEN uses that reclaim time to test the > stats collection and aggregation. We don't need to use such a > complicated stat, as the context in which the stat is collected is > orthogonal. > > Simplify the test by using a simple stat instead of reclaim time, the > total number of times a process has ever entered a cgroup. This makes > the test simpler and removes the dependency on the memory controller and > the memory reclaim interface. > > Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> Acked-by: KP Singh <kpsingh@xxxxxxxxxx> > --- > > When the test failed on Alexei's setup because the memory controller was > not enabled I realized this is an unnecessary dependency for the test, > which inspired this patch :) I am not sure if this prompt a Fixes tag as > the test wasn't broken. yeah, this is an improvement, I don't think a fixes tag is needed here. > > --- > .../prog_tests/cgroup_hierarchical_stats.c | 157 ++++++--------- > .../bpf/progs/cgroup_hierarchical_stats.c | 181 ++++++------------ > 2 files changed, 118 insertions(+), 220 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c > index bed1661596f7..12a6d4ddbd77 100644 > --- a/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_hierarchical_stats.c > @@ -1,6 +1,9 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* > - * Functions to manage eBPF programs attached to cgroup subsystems > + * This test runs a BPF program that keeps a stat of the number of processes > + * that ever attached to a cgroup, and makes sure that BPF integrates well with > + * the rstat framework to efficiently collect those stat percpu to avoid > + * locking, and to efficiently aggregate the stat across the hierarchy. > * > * Copyright 2022 Google LLC. > */ > @@ -21,8 +24,10 @@ > #define PAGE_SIZE 4096 > #define MB(x) (x << 20) > > +#define PROCESSES_PER_CGROUP 3 > + > #define BPFFS_ROOT "/sys/fs/bpf/" > -#define BPFFS_VMSCAN BPFFS_ROOT"vmscan/" > +#define BPFFS_ATTACH_COUNTERS BPFFS_ROOT"attach_counters/" minor nit: Is there a missing space here? i.e #define BPFFS_ATTACH_COUNTERS BPFFS_ROOT "attach_counters/" (this was a case in the line you changed so I am not sure if it's intentional) The rest looks good to me, so maintainers could, potentially, push it with the minor edit if needed? > > #define CG_ROOT_NAME "root" > #define CG_ROOT_ID 1 > @@ -79,7 +84,7 @@ static int setup_bpffs(void) > return err; > > [...] > - return 1; > + return 0; > } > -- > 2.37.2.672.g94769d06f0-goog >
