On Sun, Aug 28, 2022 at 3:48 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > 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) > Thanks for taking a look! Yeah I forgot that even if I added a space there C will still concatenate those strings without adding any spaces, so I guess it is better with that space. > The rest looks good to me, so maintainers could, potentially, push it > with the minor edit if needed? Yeah if that space can be added that would be great. Otherwise I can send a v2. > > > > > > #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 > >
