Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Jan 10, 2023 at 11:28:08AM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 09/01/2023 17:27, Jason Ekstrand wrote:
> 
> [snip]
> 
> >      >>> AFAICT it proposes to have 1:1 between *userspace* created
> >     contexts (per
> >      >>> context _and_ engine) and drm_sched. I am not sure avoiding
> >     invasive changes
> >      >>> to the shared code is in the spirit of the overall idea and instead
> >      >>> opportunity should be used to look at way to refactor/improve
> >     drm_sched.
> > 
> > 
> > Maybe?  I'm not convinced that what Xe is doing is an abuse at all or
> > really needs to drive a re-factor.  (More on that later.)  There's only
> > one real issue which is that it fires off potentially a lot of kthreads.
> > Even that's not that bad given that kthreads are pretty light and you're
> > not likely to have more kthreads than userspace threads which are much
> > heavier.  Not ideal, but not the end of the world either.  Definitely
> > something we can/should optimize but if we went through with Xe without
> > this patch, it would probably be mostly ok.
> > 
> >      >> Yes, it is 1:1 *userspace* engines and drm_sched.
> >      >>
> >      >> I'm not really prepared to make large changes to DRM scheduler
> >     at the
> >      >> moment for Xe as they are not really required nor does Boris
> >     seem they
> >      >> will be required for his work either. I am interested to see
> >     what Boris
> >      >> comes up with.
> >      >>
> >      >>> Even on the low level, the idea to replace drm_sched threads
> >     with workers
> >      >>> has a few problems.
> >      >>>
> >      >>> To start with, the pattern of:
> >      >>>
> >      >>>    while (not_stopped) {
> >      >>>     keep picking jobs
> >      >>>    }
> >      >>>
> >      >>> Feels fundamentally in disagreement with workers (while
> >     obviously fits
> >      >>> perfectly with the current kthread design).
> >      >>
> >      >> The while loop breaks and worker exists if no jobs are ready.
> > 
> > 
> > I'm not very familiar with workqueues. What are you saying would fit
> > better? One scheduling job per work item rather than one big work item
> > which handles all available jobs?
> 
> Yes and no, it indeed IMO does not fit to have a work item which is
> potentially unbound in runtime. But it is a bit moot conceptual mismatch
> because it is a worst case / theoretical, and I think due more fundamental
> concerns.
> 
> If we have to go back to the low level side of things, I've picked this
> random spot to consolidate what I have already mentioned and perhaps expand.
> 
> To start with, let me pull out some thoughts from workqueue.rst:
> 
> """
> Generally, work items are not expected to hog a CPU and consume many cycles.
> That means maintaining just enough concurrency to prevent work processing
> from stalling should be optimal.
> """
> 
> For unbound queues:
> """
> The responsibility of regulating concurrency level is on the users.
> """
> 
> Given the unbound queues will be spawned on demand to service all queued
> work items (more interesting when mixing up with the system_unbound_wq), in
> the proposed design the number of instantiated worker threads does not
> correspond to the number of user threads (as you have elsewhere stated), but
> pessimistically to the number of active user contexts. That is the number
> which drives the maximum number of not-runnable jobs that can become
> runnable at once, and hence spawn that many work items, and in turn unbound
> worker threads.
> 
> Several problems there.
> 
> It is fundamentally pointless to have potentially that many more threads
> than the number of CPU cores - it simply creates a scheduling storm.
> 

We can use a different work queue if this is an issue, have a FIXME
which indicates we should allow the user to pass in the work queue.

> Unbound workers have no CPU / cache locality either and no connection with
> the CPU scheduler to optimize scheduling patterns. This may matter either on
> large systems or on small ones. Whereas the current design allows for
> scheduler to notice userspace CPU thread keeps waking up the same drm
> scheduler kernel thread, and so it can keep them on the same CPU, the
> unbound workers lose that ability and so 2nd CPU might be getting woken up
> from low sleep for every submission.
>

I guess I don't understand kthread vs. workqueue scheduling internals.
 
> Hence, apart from being a bit of a impedance mismatch, the proposal has the
> potential to change performance and power patterns and both large and small
> machines.
>

We are going to have to test this out I suppose and play around to see
if this design has any real world impacts. As Jason said, yea probably
will need a bit of help here from others. Will CC relavent parties on
next rev. 
 
> >      >>> Secondly, it probably demands separate workers (not optional),
> >     otherwise
> >      >>> behaviour of shared workqueues has either the potential to
> >     explode number
> >      >>> kernel threads anyway, or add latency.
> >      >>>
> >      >>
> >      >> Right now the system_unbound_wq is used which does have a limit
> >     on the
> >      >> number of threads, right? I do have a FIXME to allow a worker to be
> >      >> passed in similar to TDR.
> >      >>
> >      >> WRT to latency, the 1:1 ratio could actually have lower latency
> >     as 2 GPU
> >      >> schedulers can be pushing jobs into the backend / cleaning up
> >     jobs in
> >      >> parallel.
> >      >>
> >      >
> >      > Thought of one more point here where why in Xe we absolutely want
> >     a 1 to
> >      > 1 ratio between entity and scheduler - the way we implement
> >     timeslicing
> >      > for preempt fences.
> >      >
> >      > Let me try to explain.
> >      >
> >      > Preempt fences are implemented via the generic messaging
> >     interface [1]
> >      > with suspend / resume messages. If a suspend messages is received to
> >      > soon after calling resume (this is per entity) we simply sleep in the
> >      > suspend call thus giving the entity a timeslice. This completely
> >     falls
> >      > apart with a many to 1 relationship as now a entity waiting for a
> >      > timeslice blocks the other entities. Could we work aroudn this,
> >     sure but
> >      > just another bunch of code we'd have to add in Xe. Being to
> >     freely sleep
> >      > in backend without affecting other entities is really, really
> >     nice IMO
> >      > and I bet Xe isn't the only driver that is going to feel this way.
> >      >
> >      > Last thing I'll say regardless of how anyone feels about Xe using
> >     a 1 to
> >      > 1 relationship this patch IMO makes sense as I hope we can all
> >     agree a
> >      > workqueue scales better than kthreads.
> > 
> >     I don't know for sure what will scale better and for what use case,
> >     combination of CPU cores vs number of GPU engines to keep busy vs other
> >     system activity. But I wager someone is bound to ask for some
> >     numbers to
> >     make sure proposal is not negatively affecting any other drivers.
> > 
> > 
> > Then let them ask.  Waving your hands vaguely in the direction of the
> > rest of DRM and saying "Uh, someone (not me) might object" is profoundly
> > unhelpful.  Sure, someone might.  That's why it's on dri-devel.  If you
> > think there's someone in particular who might have a useful opinion on
> > this, throw them in the CC so they don't miss the e-mail thread.
> > 
> > Or are you asking for numbers?  If so, what numbers are you asking for?
> 
> It was a heads up to the Xe team in case people weren't appreciating how the
> proposed change has the potential influence power and performance across the
> board. And nothing in the follow up discussion made me think it was
> considered so I don't think it was redundant to raise it.
> 
> In my experience it is typical that such core changes come with some
> numbers. Which is in case of drm scheduler is tricky and probably requires
> explicitly asking everyone to test (rather than count on "don't miss the
> email thread"). Real products can fail to ship due ten mW here or there.
> Like suddenly an extra core prevented from getting into deep sleep.
> 
> If that was "profoundly unhelpful" so be it.
> 
> > Also, If we're talking about a design that might paint us into an
> > Intel-HW-specific hole, that would be one thing.  But we're not.  We're
> > talking about switching which kernel threading/task mechanism to use for
> > what's really a very generic problem.  The core Xe design works without
> > this patch (just with more kthreads).  If we land this patch or
> > something like it and get it wrong and it causes a performance problem
> > for someone down the line, we can revisit it.
> 
> For some definition of "it works" - I really wouldn't suggest shipping a
> kthread per user context at any point.
>

Yea, this is why using a workqueue rathre than a kthread was suggested
to me by AMD. I should've put a suggested by on the commit message, need
to dig through my emails and figure out who exactly suggested this.
 
> >     In any case that's a low level question caused by the high level design
> >     decision. So I'd think first focus on the high level - which is the 1:1
> >     mapping of entity to scheduler instance proposal.
> > 
> >     Fundamentally it will be up to the DRM maintainers and the community to
> >     bless your approach. And it is important to stress 1:1 is about
> >     userspace contexts, so I believe unlike any other current scheduler
> >     user. And also important to stress this effectively does not make Xe
> >     _really_ use the scheduler that much.
> > 
> > 
> > I don't think this makes Xe nearly as much of a one-off as you think it
> > does.  I've already told the Asahi team working on Apple M1/2 hardware
> > to do it this way and it seems to be a pretty good mapping for them. I
> > believe this is roughly the plan for nouveau as well.  It's not the way
> > it currently works for anyone because most other groups aren't doing FW
> > scheduling yet.  In the world of FW scheduling and hardware designed to
> > support userspace direct-to-FW submit, I think the design makes perfect
> > sense (see below) and I expect we'll see more drivers move in this
> > direction as those drivers evolve.  (AMD is doing some customish thing
> > for how with gpu_scheduler on the front-end somehow. I've not dug into
> > those details.)
> > 
> >     I can only offer my opinion, which is that the two options mentioned in
> >     this thread (either improve drm scheduler to cope with what is
> >     required,
> >     or split up the code so you can use just the parts of drm_sched which
> >     you want - which is frontend dependency tracking) shouldn't be so
> >     readily dismissed, given how I think the idea was for the new driver to
> >     work less in a silo and more in the community (not do kludges to
> >     workaround stuff because it is thought to be too hard to improve common
> >     code), but fundamentally, "goto previous paragraph" for what I am
> >     concerned.
> > 
> > 
> > Meta comment:  It appears as if you're falling into the standard i915
> > team trap of having an internal discussion about what the community
> > discussion might look like instead of actually having the community
> > discussion.  If you are seriously concerned about interactions with
> > other drivers or whether or setting common direction, the right way to
> > do that is to break a patch or two out into a separate RFC series and
> > tag a handful of driver maintainers.  Trying to predict the questions
> > other people might ask is pointless. Cc them and asking for their input
> > instead.
> 
> I don't follow you here. It's not an internal discussion - I am raising my
> concerns on the design publicly. I am supposed to write a patch to show
> something, but am allowed to comment on a RFC series?
> 
> It is "drm/sched: Convert drm scheduler to use a work queue rather than
> kthread" which should have Cc-ed _everyone_ who use drm scheduler.
>

Yea, will do on next rev.
 
> > 
> >     Regards,
> > 
> >     Tvrtko
> > 
> >     P.S. And as a related side note, there are more areas where drm_sched
> >     could be improved, like for instance priority handling.
> >     Take a look at msm_submitqueue_create / msm_gpu_convert_priority /
> >     get_sched_entity to see how msm works around the drm_sched hardcoded
> >     limit of available priority levels, in order to avoid having to leave a
> >     hw capability unused. I suspect msm would be happier if they could have
> >     all priority levels equal in terms of whether they apply only at the
> >     frontend level or completely throughout the pipeline.
> > 
> >      > [1]
> >     https://patchwork.freedesktop.org/patch/515857/?series=112189&rev=1
> >     <https://patchwork.freedesktop.org/patch/515857/?series=112189&rev=1>
> >      >
> >      >>> What would be interesting to learn is whether the option of
> >     refactoring
> >      >>> drm_sched to deal with out of order completion was considered
> >     and what were
> >      >>> the conclusions.
> >      >>>
> >      >>
> >      >> I coded this up a while back when trying to convert the i915 to
> >     the DRM
> >      >> scheduler it isn't all that hard either. The free flow control
> >     on the
> >      >> ring (e.g. set job limit == SIZE OF RING / MAX JOB SIZE) is
> >     really what
> >      >> sold me on the this design.
> > 
> > 
> > You're not the only one to suggest supporting out-of-order completion.
> > However, it's tricky and breaks a lot of internal assumptions of the
> > scheduler. It also reduces functionality a bit because it can no longer
> > automatically rate-limit HW/FW queues which are often fixed-size.  (Ok,
> > yes, it probably could but it becomes a substantially harder problem.)
> > 
> > It also seems like a worse mapping to me.  The goal here is to turn
> > submissions on a userspace-facing engine/queue into submissions to a FW
> > queue submissions, sorting out any dma_fence dependencies.  Matt's
> > description of saying this is a 1:1 mapping between sched/entity doesn't
> > tell the whole story. It's a 1:1:1 mapping between xe_engine,
> > gpu_scheduler, and GuC FW engine.  Why make it a 1:something:1 mapping?
> > Why is that better?
> 
> As I have stated before, what I think what would fit well for Xe is one
> drm_scheduler per engine class. In specific terms on our current hardware,
> one drm scheduler instance for render, compute, blitter, video and video
> enhance. Userspace contexts remain scheduler entities.
>

I disagree.
 
> That way you avoid the whole kthread/kworker story and you have it actually
> use the entity picking code in the scheduler, which may be useful when the
> backend is congested.
>

In practice the backend shouldn't be congested but if it is a mutex
provides fairness probably better than using a shared scheduler. Also
what you are suggesting doesn't make sense at all as the congestion is
per-GT, so if anything we should use 1 scheduler per-GT not per engine
class.
 
> Yes you have to solve the out of order problem so in my mind that is
> something to discuss. What the problem actually is (just TDR?), how tricky
> and why etc.
>

Cleanup of jobs, TDR, replaying jobs, etc... It has decent amount of
impact.
 
> And yes you lose the handy LRCA ring buffer size management so you'd have to
> make those entities not runnable in some other way.
>

Also we lose our preempt fence implemenation too. Again I don't see how
the design you are suggesting is a win.
 
> Regarding the argument you raise below - would any of that make the frontend
> / backend separation worse and why? Do you think it is less natural? If
> neither is true then all remains is that it appears extra work to support
> out of order completion of entities has been discounted in favour of an easy
> but IMO inelegant option.
> 
> > There are two places where this 1:1:1 mapping is causing problems:
> > 
> >   1. It creates lots of kthreads. This is what this patch is trying to
> > solve. IDK if it's solving it the best way but that's the goal.
> > 
> >   2. There are a far more limited number of communication queues between
> > the kernel and GuC for more meta things like pausing and resuming
> > queues, getting events back from GuC, etc. Unless we're in a weird
> > pressure scenario, the amount of traffic on this queue should be low so
> > we can probably just have one per physical device.  The vast majority of
> > kernel -> GuC communication should be on the individual FW queue rings
> > and maybe smashing in-memory doorbells.
> 
> I don't follow your terminology here. I suppose you are talking about global
> GuC CT and context ringbuffers. If so then isn't "far more limited" actually
> one?
> 

We have 1 GuC GT per-GT.

Matt

> Regards,
> 
> Tvrtko
> 
> > Doing out-of-order completion sort-of solves the 1 but does nothing for
> > 2 and actually makes managing FW queues harder because we no longer have
> > built-in rate limiting.  Seems like a net loss to me.
> > 
> >      >>> Second option perhaps to split out the drm_sched code into
> >     parts which would
> >      >>> lend themselves more to "pick and choose" of its functionalities.
> >      >>> Specifically, Xe wants frontend dependency tracking, but not
> >     any scheduling
> >      >>> really (neither least busy drm_sched, neither FIFO/RQ entity
> >     picking), so
> >      >>> even having all these data structures in memory is a waste.
> >      >>>
> >      >>
> >      >> I don't think that we are wasting memory is a very good argument for
> >      >> making intrusive changes to the DRM scheduler.
> > 
> > 
> > Worse than that, I think the "we could split it up" kind-of misses the
> > point of the way Xe is using drm/scheduler.  It's not just about
> > re-using a tiny bit of dependency tracking code.  Using the scheduler in
> > this way provides a clean separation between front-end and back-end.
> > The job of the userspace-facing ioctl code is to shove things on the
> > scheduler.  The job of the run_job callback is to encode the job into
> > the FW queue format, stick it in the FW queue ring, and maybe smash a
> > doorbell.  Everything else happens in terms of managing those queues
> > side-band.  The gpu_scheduler code manages the front-end queues and Xe
> > manages the FW queues via the Kernel <-> GuC communication rings.  From
> > a high level, this is a really clean design.  There are potentially some
> > sticky bits around the dual-use of dma_fence for scheduling and memory
> > management but none of those are solved by breaking the DRM scheduler
> > into chunks or getting rid of the 1:1:1 mapping.
> > 
> > If we split it out, we're basically asking the driver to implement a
> > bunch of kthread or workqueue stuff, all the ring rate-limiting, etc.
> > It may not be all that much code but also, why?  To save a few bytes of
> > memory per engine?  Each engine already has 32K(ish) worth of context
> > state and a similar size ring to communicate with the FW.  No one is
> > going to notice an extra CPU data structure.
> > 
> > I'm not seeing a solid argument against the 1:1:1 design here other than
> > that it doesn't seem like the way DRM scheduler was intended to be
> > used.  I won't argue that.  It's not.  But it is a fairly natural way to
> > take advantage of the benefits the DRM scheduler does provide while also
> > mapping it to hardware that was designed for userspace direct-to-FW
> > submit.
> > 
> > --Jason
> > 
> >      >>> With the first option then the end result could be drm_sched
> >     per engine
> >      >>> class (hardware view), which I think fits with the GuC model.
> >     Give all
> >      >>> schedulable contexts (entities) to the GuC and then mostly
> >     forget about
> >      >>> them. Timeslicing and re-ordering and all happens transparently
> >     to the
> >      >>> kernel from that point until completion.
> >      >>>
> >      >>
> >      >> Out-of-order problem still exists here.
> >      >>
> >      >>> Or with the second option you would build on some smaller
> >     refactored
> >      >>> sub-components of drm_sched, by maybe splitting the dependency
> >     tracking from
> >      >>> scheduling (RR/FIFO entity picking code).
> >      >>>
> >      >>> Second option is especially a bit vague and I haven't thought
> >     about the
> >      >>> required mechanics, but it just appeared too obvious the
> >     proposed design has
> >      >>> a bit too much impedance mismatch.
> >      >>>
> >      >>
> >      >> IMO ROI on this is low and again lets see what Boris comes up with.
> >      >>
> >      >> Matt
> >      >>
> >      >>> Oh and as a side note, when I went into the drm_sched code base
> >     to remind
> >      >>> myself how things worked, it is quite easy to find some FIXME
> >     comments which
> >      >>> suggest people working on it are unsure of locking desing there
> >     and such. So
> >      >>> perhaps that all needs cleanup too, I mean would benefit from
> >      >>> refactoring/improving work as brainstormed above anyway.
> >      >>>
> >      >>> Regards,
> >      >>>
> >      >>> Tvrtko
> >