| Message ID | 20180122154435.18146-1-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22 January 2018 at 15:44, Matt Roper <matthew.d.roper@intel.com> wrote: > drm_set_cgrp_param is a simple tool to set DRM parameters associated with a > cgroup. It is intended to be called at system initialization time (e.g., from > a sysv-init script or systemd service) to configure graphics policy and > resource management according to the wishes of the system integrator. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > configure.ac | 1 + > tests/Makefile.am | 2 +- > tests/drm_set_cgrp_param/Makefile.am | 18 ++++++ > tests/drm_set_cgrp_param/drm_set_cgrp_param.c | 80 +++++++++++++++++++++++++++ > 4 files changed, 100 insertions(+), 1 deletion(-) > create mode 100644 tests/drm_set_cgrp_param/Makefile.am > create mode 100644 tests/drm_set_cgrp_param/drm_set_cgrp_param.c > Hi Matt, Adding a small test/demo in libdrm sounds good. Although I think we need some IGT tests, if you haven't prepped them already. After all we need to ensure the kernel correctly validates and errors when we feed it the wrong info through the IOCTL. There's a small suggestions about the IOCTL design. s/reserved/flags/ to make future extension are possible - as mentioned in [2] Thanks Emil [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt?h=v4.15-rc9#n64
On Fri, Jan 26, 2018 at 05:08:48PM +0000, Emil Velikov wrote: > On 22 January 2018 at 15:44, Matt Roper <matthew.d.roper@intel.com> wrote: > > drm_set_cgrp_param is a simple tool to set DRM parameters associated with a > > cgroup. It is intended to be called at system initialization time (e.g., from > > a sysv-init script or systemd service) to configure graphics policy and > > resource management according to the wishes of the system integrator. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > configure.ac | 1 + > > tests/Makefile.am | 2 +- > > tests/drm_set_cgrp_param/Makefile.am | 18 ++++++ > > tests/drm_set_cgrp_param/drm_set_cgrp_param.c | 80 +++++++++++++++++++++++++++ > > 4 files changed, 100 insertions(+), 1 deletion(-) > > create mode 100644 tests/drm_set_cgrp_param/Makefile.am > > create mode 100644 tests/drm_set_cgrp_param/drm_set_cgrp_param.c > > > Hi Matt, > > Adding a small test/demo in libdrm sounds good. Although I think we > need some IGT tests, if you haven't prepped them already. > After all we need to ensure the kernel correctly validates and errors > when we feed it the wrong info through the IOCTL. Yeah, agreed. Writing real IGT's was in the TODO list of the cover-letter for my kernel patch series. This kernel work is still a pretty early RFC just to gather feedback on the general approach of integrating cgroups with DRM; I figured I'd wait on writing real IGT's until we're more confident that this is the right approach in general. I'm working on a v2 right now that makes some pretty significant changes to the series, but I'm not sure yet whether the uapi will change in my next iteration or not. > There's a small suggestions about the IOCTL design. > s/reserved/flags/ to make future extension are possible - as mentioned in [2] Yeah, that's why I added the reserved field; we don't have any actual flags yet, but as soon as we do we'd rename the field to flags and document it accordingly. I can rename the field immediately if you think that's easier. I think the most important thing that's missing at the moment is that the kernel patches forgot to check that the reserved field is actually empty (i.e., we should reject calls with any garbage in there now so that we don't break ABI in the future when we start really using those bits for something). Thanks for the feedback! Matt > > Thanks > Emil > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt?h=v4.15-rc9#n64
On 26 January 2018 at 17:27, Matt Roper <matthew.d.roper@intel.com> wrote: > On Fri, Jan 26, 2018 at 05:08:48PM +0000, Emil Velikov wrote: >> On 22 January 2018 at 15:44, Matt Roper <matthew.d.roper@intel.com> wrote: >> > drm_set_cgrp_param is a simple tool to set DRM parameters associated with a >> > cgroup. It is intended to be called at system initialization time (e.g., from >> > a sysv-init script or systemd service) to configure graphics policy and >> > resource management according to the wishes of the system integrator. >> > >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> > --- >> > configure.ac | 1 + >> > tests/Makefile.am | 2 +- >> > tests/drm_set_cgrp_param/Makefile.am | 18 ++++++ >> > tests/drm_set_cgrp_param/drm_set_cgrp_param.c | 80 +++++++++++++++++++++++++++ >> > 4 files changed, 100 insertions(+), 1 deletion(-) >> > create mode 100644 tests/drm_set_cgrp_param/Makefile.am >> > create mode 100644 tests/drm_set_cgrp_param/drm_set_cgrp_param.c >> > >> Hi Matt, >> >> Adding a small test/demo in libdrm sounds good. Although I think we >> need some IGT tests, if you haven't prepped them already. >> After all we need to ensure the kernel correctly validates and errors >> when we feed it the wrong info through the IOCTL. > > Yeah, agreed. Writing real IGT's was in the TODO list of the > cover-letter for my kernel patch series. This kernel work is still a > pretty early RFC just to gather feedback on the general approach of > integrating cgroups with DRM; I figured I'd wait on writing real IGT's > until we're more confident that this is the right approach in general. > > I'm working on a v2 right now that makes some pretty significant changes > to the series, but I'm not sure yet whether the uapi will change in my > next iteration or not. > Good call - have an agreement about the interface and usage first. Then iron out all the fiddly bits. >> There's a small suggestions about the IOCTL design. >> s/reserved/flags/ to make future extension are possible - as mentioned in [2] > > Yeah, that's why I added the reserved field; we don't have any actual > flags yet, but as soon as we do we'd rename the field to flags and > document it accordingly. I can rename the field immediately if you > think that's easier. I think the most important thing that's missing at > the moment is that the kernel patches forgot to check that the reserved > field is actually empty (i.e., we should reject calls with any garbage > in there now so that we don't break ABI in the future when we start > really using those bits for something). > Please rename it now. Otherwise we'll get a build break for new kernel and old userspace. And yes, validating (returning -EINVAL IIRC) the flags is a must. Thanks Emil
diff --git a/configure.ac b/configure.ac index 35378b3384..9b5f340e2a 100644 --- a/configure.ac +++ b/configure.ac @@ -567,6 +567,7 @@ AC_CONFIG_FILES([ tests/nouveau/Makefile tests/etnaviv/Makefile tests/util/Makefile + tests/drm_set_cgrp_param/Makefile man/Makefile libdrm.pc]) AC_OUTPUT diff --git a/tests/Makefile.am b/tests/Makefile.am index 0355a9255f..d77a8639c8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,4 +1,4 @@ -SUBDIRS = util kms modeprint proptest modetest vbltest +SUBDIRS = util kms modeprint proptest modetest vbltest drm_set_cgrp_param if HAVE_LIBKMS SUBDIRS += kmstest diff --git a/tests/drm_set_cgrp_param/Makefile.am b/tests/drm_set_cgrp_param/Makefile.am new file mode 100644 index 0000000000..c32ec1c440 --- /dev/null +++ b/tests/drm_set_cgrp_param/Makefile.am @@ -0,0 +1,18 @@ +AM_CFLAGS = \ + $(WARN_CFLAGS)\ + -I$(top_srcdir)/include/drm \ + -I$(top_srcdir)/tests \ + -I$(top_srcdir) + +if HAVE_INSTALL_TESTS +bin_PROGRAMS = \ + drm_set_cgrp_param +else +noinst_PROGRAMS = \ + drm_set_cgrp_param +endif + +drm_set_cgrp_param_SOURCES = \ + drm_set_cgrp_param.c +drm_set_cgrp_param_LDADD = \ + $(top_builddir)/libdrm.la diff --git a/tests/drm_set_cgrp_param/drm_set_cgrp_param.c b/tests/drm_set_cgrp_param/drm_set_cgrp_param.c new file mode 100644 index 0000000000..987bd18e88 --- /dev/null +++ b/tests/drm_set_cgrp_param/drm_set_cgrp_param.c @@ -0,0 +1,80 @@ +/* + * \file drm_set_cgrp_prop.c + * Simple tool to set a DRM property for a cgroup. Intended for use in + * system-specific initialization handling (e.g., sysv init, systemd, etc.). + */ + +/* + * 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 <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +#include "xf86drm.h" +#include "xf86drmMode.h" + +#include "util/common.h" + +int main(int argc, char **argv) +{ + int drm_fd, cgrp_fd; + struct drm_cgroup_setparam req; + int ret; + + if (argc != 5) { + puts("Usage:"); + printf(" %s <drm device> <cgroup-v2> <prop> <val>\n\n", argv[0]); + puts("Example:"); + printf(" %s /dev/dri/card0 /sys/fs/cgroup-2/highprio 1 10\n", + argv[0]); + return 1; + } + + drm_fd = open(argv[1], O_RDWR, 0); + if (drm_fd < 0) { + perror("Invalid DRM device"); + return 1; + } + + cgrp_fd = open(argv[2], O_RDONLY|O_DIRECTORY, 0); + if (cgrp_fd < 0) { + perror("Invalid cgroup directory"); + return 1; + } + + req.cgroup_fd = cgrp_fd; + req.reserved = 0; + req.param = (uint64_t)strtoul(argv[3], NULL, 0); + req.value = (int64_t)strtol(argv[4], NULL, 0); + + ret = drmIoctl(drm_fd, DRM_IOCTL_CGROUP_SETPARAM, &req); + if (ret) + perror("Failed to set cgroup parameter"); + + close(cgrp_fd); + close(drm_fd); + + return ret; +}
drm_set_cgrp_param is a simple tool to set DRM parameters associated with a cgroup. It is intended to be called at system initialization time (e.g., from a sysv-init script or systemd service) to configure graphics policy and resource management according to the wishes of the system integrator. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- configure.ac | 1 + tests/Makefile.am | 2 +- tests/drm_set_cgrp_param/Makefile.am | 18 ++++++ tests/drm_set_cgrp_param/drm_set_cgrp_param.c | 80 +++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 tests/drm_set_cgrp_param/Makefile.am create mode 100644 tests/drm_set_cgrp_param/drm_set_cgrp_param.c