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
> >
