[Intel-gfx] [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)

[Matt Roper]

On Thu, Mar 08, 2018 at 01:11:33PM +0000, Chris Wilson wrote:
> Quoting Chris Wilson (2018-03-08 11:32:04)
> > Quoting Matt Roper (2018-03-06 23:46:59)
> > > 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.
> > > 
> > > v2:
> > >  - Rebase onto new cgroup_priv API
> > >  - Use current instead of drm_file->pid to determine which process to
> > >    lookup priority for. (Chris)
> > >  - Don't forget to subtract priority offset in context_getparam ioctl to
> > >    make it match setparam behavior. (Chris)
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > 
> > For ctx->priority/ctx->priority_offset
> > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> As a reminder, we do have the question of how to bound ctx->priority +
> ctx->priority_offset. Currently, outside of the user range is privileged
> space reserved for the kernel (both above and below). Now we can move
> those even further to accommodate multiple non-overlapping cgroups.

Yep, I mentioned this as an open question in the series coverletter.

My understanding is that today we limit userspace-assigned priorities to
[1023,1023] and that the kernel can use the userspace range plus -1024
(for the kernel idle context), 1024 (for boosting contexts the display
is waiting on), and INT_MAX (for the preempt context).  Are there any
other special values we use today that I'm overlooking?

I think we have the following options with how to proceed:

 * Option 1:  Silently limit (priority+priority offset) to the existing
   [-1023,1023] range.  That means that if, for example, a user context
   had a priority offset of 600 and tried to manually boost its context
   priority to 600, it would simply be treated as a 1023 for scheduling
   purposes.  This approach is simple, but doesn't allow us to force
   contexts in different cgroups into non-overlapping priority ranges
   (i.e., lowest possible priority in one cgroup is still higher than
   the highest possible priority in another cgroup).  It also isn't
   possible to define a class of applications as "more important than
   display" via cgroup, which I think might be useful in some cases.

 * Option 2:  Allow priority offset to be set in a much larger range
   (e.g., [SHRT_MIN+1023,SHRT_MAX-1023]).  This allows cgroups to have
   effective priority ranges that don't overlap.  cgroup ranges can also
   be intentionally set above I915_PRIORITY_DISPLAY to allow us to
   define classes of applications whose work is more important than
   maintaining a stable framerate on the display.  We'd also probably
   need to shift the kernel idle context down to something like INT_MIN
   instead of using -1024.

 * Option 3: No limits, leave behavior as it stands now in this patch
   series (unrestricted range).  If you're privileged enough to define
   the cgroup settings for a system and you decide to shoot yourself in
   the foot by setting them to nonsense values, that's your own fault.

Thoughts?  Any other options to consider?

> We also have the presumption that only privileged processes can raise a
> priority, but I presume the cgroup property itself is protected.
> -Chris

The way the code is written write now, anyone who has write access to
the cgroup itself (i.e., can migrate processes into/out of the cgroup)
is allowed to adjust the i915-specific cgroup settings.  So this depends
on how the cgroups-v2 filesystem was initially mounted and/or how the
filesystem permissions were adjusted after the fact; the details may
vary from system to system depending on the policy decisions of the
system integrator / system administrator.  But I think an off-the-shelf
Linux distro will only give the system administrator sufficient
permissions to adjust cgroups (if it even mounts the cgroups-v2
filesystem in the first place), so anyone else that winds up with those
privileges has had them intentionally delegated.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795