Message-ID: <aBAjpKvXoT62zG6h@google.com>
Date: Mon, 28 Apr 2025 17:56:04 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: kvm@...r.kernel.org, Maxim Levitsky <mlevitsk@...hat.com>, 
	Axel Rasmussen <axelrasmussen@...gle.com>, Tejun Heo <tj@...nel.org>, 
	Johannes Weiner <hannes@...xchg.org>, mkoutny@...e.com, Yosry Ahmed <yosry.ahmed@...ux.dev>, 
	Yu Zhao <yuzhao@...gle.com>, David Matlack <dmatlack@...gle.com>, cgroups@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/5] KVM: selftests: access_tracking_perf_test: Use
 MGLRU for access tracking

On Mon, Apr 28, 2025, James Houghton wrote:
> On Fri, Apr 25, 2025 at 8:31 PM Sean Christopherson <seanjc@...gle.com> wrote:
> >
> > On Mon, Apr 14, 2025, James Houghton wrote:
> > > By using MGLRU's debugfs for invoking test_young() and clear_young(), we
> > > avoid page_idle's incompatibility with MGLRU, and we can mark pages as
> > > idle (clear_young()) much faster.
> > >
> > > The ability to use page_idle is left in, as it is useful for kernels
> > > that do not have MGLRU built in. If MGLRU is enabled but is not usable
> > > (e.g. we can't access the debugfs mount), the test will fail, as
> > > page_idle is not compatible with MGLRU.
> > >
> > > cgroup utility functions have been borrowed so that, when running with
> > > MGLRU, we can create a memcg in which to run our test.
> > >
> > > Other MGLRU-debugfs-specific parsing code has been added to
> > > lru_gen_util.{c,h}.
> >
> > This fails on my end due to not being able to find the cgroup.  I spent about 15
> > minutes poking at it and gave it.  FWIW, this is on our devrez hosts, so it's
> > presumably similar hardware to what you tested on.
> 
> Ah sorry, yes, this selftest needs to be patched when running the
> devrez userspace, which uses a combination of cgroup-v1 and cgroup-v2.
> Simply hard-coding the root to "/dev/cgroup/memory" (which is in fact
> a cgroup-v1 mount) should be what you need if you want to give it
> another go.
>
> > Even if this turns out to be PEBKAC or some CONFIG_XXX incompatibility, there
> > needs to be better hints provided to the user of how they can some this.
> 
> Yeah this can be better. I should at least check that the found
> cgroup-v2 root's cgroup.controllers contains "memory". In your case,
> it did not.
> 
> (cgroup.controllers is not available for cgroup-v1 -- because it
> doesn't make sense -- so if I patch the selftest to check this file,
> using cgroup-v1 mounts will stop working. So, again, you'd need to
> patch the test to work on devrez.)

Or, and I know this going to sound crazy, what if we simply make the test work
with v1 or v2?  That is not hard to do, at all.  Please slot the below into the
next version of the series.  Feel free to modify it as needed, e.g. to address
other maintainer feedback.  The only thing I care about is the selftest not failing.

> > And this would be a perfect opportunity to clean up this:
> >
> >         __TEST_REQUIRE(page_idle_fd >= 0,
> >                        "CONFIG_IDLE_PAGE_TRACKING is not enabled");
> 
> I think the change I've already made to this string is sufficient
> (happy to change it further if you like):

Doh, I missed that your patch did already improve the skip message to spit out
/sys/kernel/mm/page_idle/bitmap; that part I definitely like.

> > > +               __TEST_REQUIRE(page_idle_fd >= 0,
> > > +                              "Couldn't open /sys/kernel/mm/page_idle/bitmap. "
> > > +                              "Is CONFIG_IDLE_PAGE_TRACKING enabled?");
> 
> > I can't count the number of times I've forgotten to run the test with root
> > privileges, and wasted a bunch of time remembering it's not that the kernel
> > doesn't have CONFIG_IDLE_PAGE_TRACKING, but that /sys/kernel/mm/page_idle/bitmap
> > isn't accessible.
> >
> > I mention that, because on a kernel with MGRLU available but disabled, and
> > CONFIG_IDLE_PAGE_TRACKING=n, the user has no idea that they _can_ run the test
> > without mucking with their kernel.
> 
> Fair enough, I'll change the output from the test for that
> configuration to say something like: "please either enable the missing
> MGLRU features (e.g. `echo 3 > /sys/kernel/mm/lru_gen/enabled`) or
> recompile your kernel with CONFIG_IDLE_PAGE_TRACKING=y."

That's still misleading.  In my case, my kernels are already built with
CONFIG_IDLE_PAGE_TRACKING=y.

Looking at this again, we can do much better.  For my permissions issue, open()
should fail with -EACCES, whereas the CONFIG_IDLE_PAGE_TRACKING=n case should
fail with ENOENT.  And that is easy enough to handle in open_path_or_exit().

I'll send a small series to clean that up, and then will apply this series on top.
The resulting conflict will be trivial to resolve, so don't worry about rebasing
on top of my mini-series.

--
From: Sean Christopherson <seanjc@...gle.com>
Date: Mon, 28 Apr 2025 17:36:06 -0700
Subject: [PATCH] cgroup: selftests: Add API to find root of specific
 controller

Add an API in the cgroups library to find the root of a specific
controller.  KVM selftests will use the API to find the memory controller.

Search for the controller on both v1 and v2 mounts, as KVM selftests'
usage will be completely oblivious of v1 versus v2.

Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 .../selftests/cgroup/lib/cgroup_util.c        | 32 +++++++++++++++----
 .../cgroup/lib/include/cgroup_util.h          |  1 +
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/cgroup/lib/cgroup_util.c b/tools/testing/selftests/cgroup/lib/cgroup_util.c
index 69a68f43e3fa..4e7e7329b226 100644
--- a/tools/testing/selftests/cgroup/lib/cgroup_util.c
+++ b/tools/testing/selftests/cgroup/lib/cgroup_util.c
@@ -217,7 +217,8 @@ int cg_write_numeric(const char *cgroup, const char *control, long value)
 	return cg_write(cgroup, control, buf);
 }
 
-int cg_find_unified_root(char *root, size_t len, bool *nsdelegate)
+static int cg_find_root(char *root, size_t len, const char *controller,
+			bool *nsdelegate)
 {
 	char buf[10 * PAGE_SIZE];
 	char *fs, *mount, *type, *options;
@@ -237,17 +238,36 @@ int cg_find_unified_root(char *root, size_t len, bool *nsdelegate)
 		strtok(NULL, delim);
 		strtok(NULL, delim);
 
-		if (strcmp(type, "cgroup2") == 0) {
-			strncpy(root, mount, len);
-			if (nsdelegate)
-				*nsdelegate = !!strstr(options, "nsdelegate");
-			return 0;
+		if (strcmp(type, "cgroup") == 0) {
+			if (!controller || !strstr(options, controller))
+				continue;
+		} else if (strcmp(type, "cgroup2") == 0) {
+			if (controller &&
+			    cg_read_strstr(mount, "cgroup.controllers", controller))
+				continue;
+		} else {
+			continue;
 		}
+		strncpy(root, mount, len);
+
+		if (nsdelegate)
+			*nsdelegate = !!strstr(options, "nsdelegate");
+		return 0;
 	}
 
 	return -1;
 }
 
+int cg_find_controller_root(char *root, size_t len, const char *controller)
+{
+	return cg_find_root(root, len, controller, NULL);
+}
+
+int cg_find_unified_root(char *root, size_t len, bool *nsdelegate)
+{
+	return cg_find_root(root, len, NULL, nsdelegate);
+}
+
 int cg_create(const char *cgroup)
 {
 	return mkdir(cgroup, 0755);
diff --git a/tools/testing/selftests/cgroup/lib/include/cgroup_util.h b/tools/testing/selftests/cgroup/lib/include/cgroup_util.h
index cbe6f0b4247d..d9e6e3090b3f 100644
--- a/tools/testing/selftests/cgroup/lib/include/cgroup_util.h
+++ b/tools/testing/selftests/cgroup/lib/include/cgroup_util.h
@@ -21,6 +21,7 @@ static inline int values_close(long a, long b, int err)
 	return labs(a - b) <= (a + b) / 100 * err;
 }
 
+extern int cg_find_controller_root(char *root, size_t len, const char *controller);
 extern int cg_find_unified_root(char *root, size_t len, bool *nsdelegate);
 extern char *cg_name(const char *root, const char *name);
 extern char *cg_name_indexed(const char *root, const char *name, int index);

base-commit: 65a87fcc85da28361af2a5718c109dbc2f8d54a2
--

Powered by blists - more mailing lists