Re: [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)
On Thu, Mar 08, 2018 at 06:55:34PM +, Chris Wilson wrote: > Quoting Chris Wilson (2018-03-08 18:48:51) > > Quoting Matt Roper (2018-03-08 18:22:06) > > > * 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. > > > > INT_MIN+1 just to be awkward. (INT_MIN is I915_PRIORITY_INVALID.) > > > > Something to bear in mind is that I want to reserve the low 8 bits of > > ctx->priority for internal use (heuristic adjustments by the scheduler). > > Can shrink that to say 3 bits in the current scheme. > > > > 3bits for internal adjustment > > 20bits for user priority. > > 8bits for cgroup offset. > > signbit. > > > > Nothing prohibits us from moving to 64b if required. But 32b should be > > enough range for plenty of clients and cgroups, imo. Reducing cgroup > > offset to 6 bits would be nice :) > > Forgot to comment on the policy decision of I915_PRIORITY_DISPLAY. > It's naughty that it's a magic constant that isn't exposed to the > sysadmin, so I would still set it to the maximum priority possible under > the extended scheme (i.e. INT_MAX), but allow for it to be adjusted. > I915_SETPARAM *shivers* Maybe that's a topic for cgroup as well if we can > associate the pageflip with a process and find its interactivity settings. > > Can I expose just about any random policy decision through cgroup? > -Chris If the policy applies to a cgroup of processes, then we can deal with pretty much any kind of policy as long as we stick to the driver ioctl approach in this series. E.g., we could add a cgroup setting "eligible for display boost" so that only specific processes are eligible for a display boost. If we want to control a single overall system value (e.g., "set the display boost fixed value") we could technically do that by having such parameters set on the v2 hierarchy's root cgroup, but that seems a bit counterintuitive and I'm not sure if there's a benefit to doing so. Using a more general interface like I915_SETPARAM or sysfs or something seems more appropriate in that case. Matt -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)
Quoting Chris Wilson (2018-03-08 18:48:51) > Quoting Matt Roper (2018-03-08 18:22:06) > > * 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. > > INT_MIN+1 just to be awkward. (INT_MIN is I915_PRIORITY_INVALID.) > > Something to bear in mind is that I want to reserve the low 8 bits of > ctx->priority for internal use (heuristic adjustments by the scheduler). > Can shrink that to say 3 bits in the current scheme. > > 3bits for internal adjustment > 20bits for user priority. > 8bits for cgroup offset. > signbit. > > Nothing prohibits us from moving to 64b if required. But 32b should be > enough range for plenty of clients and cgroups, imo. Reducing cgroup > offset to 6 bits would be nice :) Forgot to comment on the policy decision of I915_PRIORITY_DISPLAY. It's naughty that it's a magic constant that isn't exposed to the sysadmin, so I would still set it to the maximum priority possible under the extended scheme (i.e. INT_MAX), but allow for it to be adjusted. I915_SETPARAM *shivers* Maybe that's a topic for cgroup as well if we can associate the pageflip with a process and find its interactivity settings. Can I expose just about any random policy decision through cgroup? -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)
Quoting Matt Roper (2018-03-08 18:22:06) > On Thu, Mar 08, 2018 at 01:11:33PM +, 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 > > > > > > For ctx->priority/ctx->priority_offset > > > Reviewed-by: Chris Wilson > > > > 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. Agreed, non-overlapping seems to be a useful property (apply the usual disclaimer for the rudimentary scheduler). > * 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. INT_MIN+1 just to be awkward. (INT_MIN is I915_PRIORITY_INVALID.) Something to bear in mind is that I want to reserve the low 8 bits of ctx->priority for internal use (heuristic adjustments by the scheduler). Can shrink that to say 3 bits in the current scheme. 3bits for internal adjustment 20bits for user priority. 8bits for cgroup offset. signbit. Nothing prohibits us from moving to 64b if required. But 32b should be enough range for plenty of clients and cgroups, imo. Reducing cgroup offset to 6 bits would be nice :) > * Option 3: No limits, leave behavior as it stands now in this patch >series (unrestricted range). If you're privileged enough to define >the
Re: [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)
On Thu, Mar 08, 2018 at 01:11:33PM +, 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 > > > > For ctx->priority/ctx->priority_offset > > Reviewed-by: Chris Wilson > > 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 dis
Re: [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)
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 > > For ctx->priority/ctx->priority_offset > Reviewed-by: Chris Wilson 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. We also have the presumption that only privileged processes can raise a priority, but I presume the cgroup property itself is protected. -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 5/6] drm/i915: Introduce 'priority offset' for GPU contexts (v2)
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 For ctx->priority/ctx->priority_offset Reviewed-by: Chris Wilson At the end of the day, everything that is modifiable by context is going to want cgroup constraint, but like priority_offset each will require some thought as to how to express the constraint. Interesting conundrum, and still we want a consistent interface for all the gpus on a system. -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
