Messages in this thread | | | From | KP Singh <> | Date | Mon, 29 Aug 2022 00:48:02 +0200 | Subject | Re: [PATCH] selftests/bpf: simplify cgroup_hierarchical_stats selftest |
| |
On Sat, Aug 27, 2022 at 1:06 AM Yosry Ahmed <yosryahmed@google.com> 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@google.com>
Acked-by: KP Singh <kpsingh@kernel.org>
> --- > > 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 >
| |