| Message ID | 20180201195615.5124-1-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+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
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.
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.
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; +}
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