Quoting Matt Roper (2018-02-01 19:53:14) > There are cases where a system integrator may wish to raise/lower the > priority of GPU workloads being submitted by specific OS process(es), > independently of how the software self-classifies its own priority. > Exposing "priority offset" as an i915-specific cgroup parameter will > enable such system-level configuration. > > Normally GPU contexts start with a priority value of 0 > (I915_CONTEXT_DEFAULT_PRIORITY) and then may be adjusted up/down from > there via other mechanisms. We'd like to provide a system-level input > to the priority decision that will be taken into consideration, even > when userspace later attempts to set an absolute priority value via > I915_CONTEXT_PARAM_PRIORITY. The priority offset introduced here > provides a base value that will always be added to (or subtracted from) > the software's self-assigned priority value. > > This patch makes priority offset a cgroup-specific value; contexts will > be created with a priority offset based on the cgroup membership of the > process creating the context at the time the context is created. Note > that priority offset is assigned at context creation time; migrating a > process to a different cgroup or changing the offset associated with a > cgroup will only affect new context creation and will not alter the > behavior of existing contexts previously created by the process. > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_cgroup.c | 46 +++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 7 +++++ > drivers/gpu/drm/i915/i915_gem_context.c | 6 +++-- > drivers/gpu/drm/i915/i915_gem_context.h | 9 +++++++ > include/uapi/drm/i915_drm.h | 1 + > 5 files changed, 67 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cgroup.c b/drivers/gpu/drm/i915/i915_cgroup.c > index 778af895fc00..73f3cfe31d66 100644 > --- a/drivers/gpu/drm/i915/i915_cgroup.c > +++ b/drivers/gpu/drm/i915/i915_cgroup.c > @@ -11,6 +11,8 @@ > > struct i915_cgroup_data { > struct cgroup_driver_data base; > + > + int priority_offset; > }; > > static inline struct i915_cgroup_data * > @@ -72,6 +74,8 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, > struct cgroup *cgrp; > struct file *f; > struct inode *inode = NULL; > + struct cgroup_driver_data *cgrpdata; > + struct i915_cgroup_data *i915data; > int ret; > > if (!dev_priv->i915_cgroups) { > @@ -125,6 +129,17 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, > return ret; > > switch (req->param) { > + case I915_CGROUP_PARAM_PRIORITY_OFFSET: > + cgrpdata = cgroup_driver_get_data(dev_priv->i915_cgroups, cgrp, > + NULL); dev_priv is i915. So i915->i915_cgroups? Could you just have a i915_get_cgroup_data() returning struct i915_croup_data? > + if (IS_ERR(cgrpdata)) > + return PTR_ERR(cgrpdata); > + > + DRM_DEBUG_DRIVER("Setting cgroup priority offset to %lld\n", > + req->value); > + i915data = cgrp_to_i915(cgrpdata); > + i915data->priority_offset = req->value; We'll have to consider what bounds we think are sensible, and how internal policy interacts. Presumably another cgroup-param :) > + break; > default: > DRM_DEBUG_DRIVER("Invalid cgroup parameter %lld\n", req->param); > return -EINVAL; > @@ -132,3 +147,34 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, > > return 0; > } > + > +/** > + * i915_cgroup_get_prio_offset() - get prio offset for current proc's cgroup > + * @dev_priv: drm device > + * @file_priv: file handle for calling process > + * > + * RETURNS: > + * Priority offset associated with the calling process' cgroup in the default > + * (v2) hierarchy, otherwise 0 if no explicit priority has been assigned. > + */ > +int > +i915_cgroup_get_prio_offset(struct drm_i915_private *dev_priv, > + struct drm_i915_file_private *file_priv) > +{ > + struct cgroup *cgrp; > + struct cgroup_driver_data *cgrpdata; > + > + /* Ignore internally-created contexts not associated with a process */ > + if (!file_priv) > + return 0; > + > + cgrp = drm_file_get_cgroup(file_priv->file); > + if (WARN_ON(!cgrp)) > + return 0; Ah, this needs to pull from current then not drm_file->pid. > + > + cgrpdata = cgroup_driver_get_data(dev_priv->i915_cgroups, cgrp, NULL); > + if (IS_ERR(cgrpdata)) > + return 0; > + > + return cgrp_to_i915(cgrpdata)->priority_offset; > +} > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 60e3ff6a48bb..313191e5278a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2920,6 +2920,8 @@ int i915_cgroup_init(struct drm_i915_private *dev_priv); > int i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, > struct drm_file *file); > void i915_cgroup_shutdown(struct drm_i915_private *dev_priv); > +int i915_cgroup_get_prio_offset(struct drm_i915_private *dev_priv, > + struct drm_i915_file_private *file_priv); > #else > static inline int > i915_cgroup_init(struct drm_i915_private *dev_priv) > @@ -2933,6 +2935,11 @@ i915_cgroup_setparam_ioctl(struct drm_device *dev, void *data, > { > return -EINVAL; > } > +static inline int > +i915_cgroup_get_prio_offset(struct drm_i915_private *dev_priv, > + struct drm_i915_file_private *file_priv) { > + return 0; > +} > #endif > > /* i915_drv.c */ > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 648e7536ff51..700d7bc7cfc8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -274,7 +274,9 @@ __create_hw_context(struct drm_i915_private *dev_priv, > kref_init(&ctx->ref); > list_add_tail(&ctx->link, &dev_priv->contexts.list); > ctx->i915 = dev_priv; > - ctx->priority = I915_PRIORITY_NORMAL; > + ctx->priority_offset = > + i915_cgroup_get_prio_offset(dev_priv, file_priv); > + ctx->priority = I915_PRIORITY_NORMAL + ctx->priority_offset; > > INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL); > INIT_LIST_HEAD(&ctx->handles_list); > @@ -816,7 +818,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, > !capable(CAP_SYS_NICE)) > ret = -EPERM; > else > - ctx->priority = priority; > + ctx->priority = priority + ctx->priority_offset; And subtract the offset in getparam. So user priority in == reported priority out, and they are none the wiser that it is being adjusted internally (out of their direct control). -Chris -- To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html