[RFC] tools: Introduce intel_cgroup tool

Submitted by Matt Roper on Feb. 1, 2018, 7:56 p.m.

Details

Message ID 20180201195615.5124-1-matthew.d.roper@intel.com
State New
Series "tools: Introduce intel_cgroup tool"
Headers show

Commit Message

Matt Roper Feb. 1, 2018, 7:56 p.m.
intel_cgroup is used to modify i915 cgroup parameters.  At the moment only a
single parameter is supported (GPU priority offset).  In the future the driver
may support additional parameters as well (e.g., limits on memory usage).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 tools/Makefile.sources |   1 +
 tools/intel_cgroup.c   | 103 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 tools/intel_cgroup.c

Patch hide | download patch | download mbox

diff --git a/tools/Makefile.sources b/tools/Makefile.sources
index abd23a0f..b30216c4 100644
--- a/tools/Makefile.sources
+++ b/tools/Makefile.sources
@@ -11,6 +11,7 @@  tools_prog_lists =		\
 	intel_reg		\
 	intel_backlight		\
 	intel_bios_dumper	\
+	intel_cgroup		\
 	intel_display_crc	\
 	intel_display_poller	\
 	intel_forcewaked	\
diff --git a/tools/intel_cgroup.c b/tools/intel_cgroup.c
new file mode 100644
index 00000000..ce781b08
--- /dev/null
+++ b/tools/intel_cgroup.c
@@ -0,0 +1,103 @@ 
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <fcntl.h>
+#include <getopt.h>
+#include <i915_drm.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include "igt.h"
+#include "xf86drm.h"
+#include "xf86drmMode.h"
+
+#define I915_CGROUP_PARAM_PRIORITY_OFFSET       0x1
+
+char short_opts[] = "p:";
+struct option long_opts[] = {
+	{ "set-prio",	required_argument, NULL, 'p'},
+	{ 0 },
+};
+
+static void usage(void)
+{
+	puts("Usage:");
+	printf("  intel_cgroup --set-prio=<val> <cgroup-v2>\n");
+}
+
+int main(int argc, char **argv)
+{
+	int drm_fd, cgrp_fd;
+	struct drm_i915_cgroup_param req;
+	int opt, ret;
+	struct {
+		bool do_prio;
+		int prio_val;
+	} updates = { 0 };
+
+	while ((opt = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) {
+		switch (opt) {
+		case 'p':
+			updates.do_prio = true;
+			updates.prio_val = atoi(optarg);
+			break;
+		default:
+			igt_assert(false);
+		}
+	}
+
+	if (argc - optind != 1) {
+		usage();
+		return 1;
+	}
+
+	drm_fd = drm_open_driver(DRIVER_INTEL);
+	if (drm_fd < 0) {
+		perror("Invalid DRM device");
+		return 1;
+	}
+
+	cgrp_fd = open(argv[optind], O_RDONLY|O_DIRECTORY, 0);
+	if (cgrp_fd < 0) {
+		perror("Invalid cgroup directory");
+		return 1;
+	}
+
+	req.cgroup_fd = cgrp_fd;
+	req.flags = 0;
+
+	if (updates.do_prio) {
+		req.param = I915_CGROUP_PARAM_PRIORITY_OFFSET;
+		req.value = updates.prio_val;
+
+		ret = drmIoctl(drm_fd, DRM_IOCTL_I915_CGROUP_SETPARAM, &req);
+		if (ret)
+			perror("Failed to set cgroup parameter");
+	}
+
+	close(cgrp_fd);
+	close(drm_fd);
+
+	return ret;
+}

Comments

Chris Wilson Feb. 1, 2018, 8:27 p.m.
Quoting Matt Roper (2018-02-01 19:56:15)
> intel_cgroup is used to modify i915 cgroup parameters.  At the moment only a
> single parameter is supported (GPU priority offset).  In the future the driver
> may support additional parameters as well (e.g., limits on memory usage).
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  tools/Makefile.sources |   1 +
>  tools/intel_cgroup.c   | 103 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 tools/intel_cgroup.c
> 
> diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> index abd23a0f..b30216c4 100644
> --- a/tools/Makefile.sources
> +++ b/tools/Makefile.sources
> @@ -11,6 +11,7 @@ tools_prog_lists =            \
>         intel_reg               \
>         intel_backlight         \
>         intel_bios_dumper       \
> +       intel_cgroup            \
>         intel_display_crc       \
>         intel_display_poller    \
>         intel_forcewaked        \
> diff --git a/tools/intel_cgroup.c b/tools/intel_cgroup.c
> new file mode 100644
> index 00000000..ce781b08
> --- /dev/null
> +++ b/tools/intel_cgroup.c
> @@ -0,0 +1,103 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <fcntl.h>
> +#include <getopt.h>
> +#include <i915_drm.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +#include "igt.h"
> +#include "xf86drm.h"
> +#include "xf86drmMode.h"
> +
> +#define I915_CGROUP_PARAM_PRIORITY_OFFSET       0x1

Hmm. Could we not avoid drm_ioctl + well known param names and use a
more generic tool to set cgroup attributes? Just feels wrong that a
such a generic interface boils down to a driver specific ioctl.
-Chris
Matt Roper Feb. 1, 2018, 11:14 p.m.
+cgroups list since this discussion goes back to the general design

On Thu, Feb 01, 2018 at 08:27:33PM +0000, Chris Wilson wrote:
> Quoting Matt Roper (2018-02-01 19:56:15)
> > intel_cgroup is used to modify i915 cgroup parameters.  At the moment only a
> > single parameter is supported (GPU priority offset).  In the future the driver
> > may support additional parameters as well (e.g., limits on memory usage).
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
<snip>
> > +#define I915_CGROUP_PARAM_PRIORITY_OFFSET       0x1
> 
> Hmm. Could we not avoid drm_ioctl + well known param names and use a
> more generic tool to set cgroup attributes? Just feels wrong that a
> such a generic interface boils down to a driver specific ioctl.
> -Chris

There are a few different design choices here, so feedback like this is
definitely what I'm looking for with this series.  A few goals we should
keep in mind as we consider options:

 * I'd like to keep the attributes we set tied to a driver/device in
   some manner.  E.g., if we have a machine with multiple GPU's, we
   should be able to set card0 priority independently of card1 priority
   A DRM or driver ioctl makes this easy, but it certainly isn't the
   only approach.

 * Some of the attributes we want to manage don't fall under the design
   goals of formal cgroup controllers.  I.e., they don't care about the
   hierarchy layout of cgroups at all; they only care about the details
   of the process' final leaf node.  GPU priority as implemented in this
   series is an example of that.

 * Other attributes we'll eventually want to control (such as graphics
   memory usage), probably _will_ want to take the hierarchy design into
   account, the same way real cgroup controllers like the mem controller
   do.

 * The drivers that want to make use of this functionality may be built
   as modules rather than compiled directly into the kernel.  This is
   important because the cgroups subsystem removed the ability to have
   cgroup controllers in modules a few years ago.  While it might be
   possible to resurrect module-based cgroup controller support, my
   impression is that the cgroups community would prefer a separate
   non-controller mechanism for doing what we want to do.  E.g., see
   https://www.spinics.net/lists/cgroups/msg18674.html for some brief
   discussion on this topic.

I think the nicest interface for setting cgroup attributes would be to
find a way to add our own kernfs entries to the cgroup filesystem, the
same way real cgroup controllers do.  Then we could do something like
"echo 123 > /cgroup2/apps/highprio/i915.card0.priority" and not need to
call any ioctl's at all.  Without creating an actual cgroup controller,
I think this might be challenging, but I'm investigating it on the side
right now to see if it's a viable option.

There are other options that might be suitable as well, but I haven't
throught through them in details:

 * Hang the ioctl() off the cgroup filesystem entry rather than the DRM
   device node.  Not sure if this gains us anything given that we still
   want to track data on a per-device basis.

 * Add a cgroup filesystem entry that just handles write() events of the
   form "drivername:key:value" to trigger driver callbacks.  This might
   be a decent compromise.  E.g., we could issue a command like:
      echo "i915.card0:prio_base:123" > /cgroup2/foo/cgroup.driver_attr
   and the kernfs handler for that file would looks for a cgroup_driver
   registered under the name "i915.card0" to figure out what driver
   callback to call for the key/value data.

The second bullet above is growing on me as I think more about it, so I
might try implementing that approach in the third version of my series
if nobody has other suggestions or feedback.


Matt
Tejun Heo Feb. 7, 2018, 9:50 p.m.
Hello, Matt, Chris.

On Thu, Feb 01, 2018 at 03:14:38PM -0800, Matt Roper wrote:
> > Hmm. Could we not avoid drm_ioctl + well known param names and use a
> > more generic tool to set cgroup attributes? Just feels wrong that a
> > such a generic interface boils down to a driver specific ioctl.

So, everything which shows up in the cgroup hierarchy should satisfy
the following two conditions.

* The control mechanism should adhere to one of the resource
  distribution models defined in Documentation/cgroup-v2.txt.  This is
  to ensure consistency across different resources which in turn
  allows things like delegation.

* This one is a bit vague and I probably should find a better way to
  describe it but each controller should encapsulate a generic core
  resource.  Here, I don't think it makes sense to have intel gfx
  controller when there are a lot of commmonalities in the graphics
  hardware across different vendors.  It should be better abstracted.

It's true that it's difficult to figure out the right generic design
without actually trying, and I think that's why it'd be better to
start scoped in the scope of the specific driver.  The smaller scope
would allow for less strict expectations, more latitude, and easier
experimentations.

> I think the nicest interface for setting cgroup attributes would be to
> find a way to add our own kernfs entries to the cgroup filesystem, the
> same way real cgroup controllers do.  Then we could do something like
> "echo 123 > /cgroup2/apps/highprio/i915.card0.priority" and not need to
> call any ioctl's at all.  Without creating an actual cgroup controller,
> I think this might be challenging, but I'm investigating it on the side
> right now to see if it's a viable option.

So, I strongly believe that it isn't the right approach to add i915
prefixed interface files to cgroup interface.

Thanks.
Tejun Heo Feb. 7, 2018, 9:54 p.m.
Hello,

Forgot to respond to one point.

On Thu, Feb 01, 2018 at 03:14:38PM -0800, Matt Roper wrote:
>  * The drivers that want to make use of this functionality may be built
>    as modules rather than compiled directly into the kernel.  This is
>    important because the cgroups subsystem removed the ability to have
>    cgroup controllers in modules a few years ago.  While it might be
>    possible to resurrect module-based cgroup controller support, my
>    impression is that the cgroups community would prefer a separate
>    non-controller mechanism for doing what we want to do.  E.g., see
>    https://www.spinics.net/lists/cgroups/msg18674.html for some brief
>    discussion on this topic.

So, this isn't a concern.  If we need to have modular controllers, we
can resurrect module support, hopefully in a simpler way, or could do
something similar to what rdma controller did.  We shouldn't make
userland interface decisions based on this sort of implementation
details.

Thanks.