LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Rob Clark <robdclark@gmail.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: "Rob Clark" <robdclark@chromium.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"Sai Prakash Ranjan" <saiprakash.ranjan@codeaurora.org>,
	"Jonathan Marek" <jonathan@marek.ca>,
	"David Airlie" <airlied@linux.ie>,
	freedreno <freedreno@lists.freedesktop.org>,
	"Sharat Masetty" <smasetty@codeaurora.org>,
	"Akhil P Oommen" <akhilpo@codeaurora.org>,
	"Jordan Crouse" <jordan@cosmicpenguin.net>,
	"open list:DRM DRIVER FOR MSM ADRENO GPU"
	<linux-arm-msm@vger.kernel.org>, "Sean Paul" <sean@poorly.run>,
	"Christian König" <christian.koenig@amd.com>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 12/13] drm/msm: Utilize gpu scheduler priorities
Date: Wed, 25 May 2022 20:15:47 -0700	[thread overview]
Message-ID: <CAF6AEGsvmQYjzoFgEMTer3oDmb62y2Hq_unDbq2UEoZ6CA3CSw@mail.gmail.com> (raw)
In-Reply-To: <1972f50b-d71a-9e2e-d10b-cc4f13bb208f@linux.intel.com>

On Wed, May 25, 2022 at 9:11 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 24/05/2022 15:57, Rob Clark wrote:
> > On Tue, May 24, 2022 at 6:45 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >>
> >> On 23/05/2022 23:53, Rob Clark wrote:
> >>>
> >>> btw, one fun (but unrelated) issue I'm hitting with scheduler... I'm
> >>> trying to add an igt test to stress shrinker/eviction, similar to the
> >>> existing tests/i915/gem_shrink.c.  But we hit an unfortunate
> >>> combination of circumstances:
> >>> 1. Pinning memory happens in the synchronous part of the submit ioctl,
> >>> before enqueuing the job for the kthread to handle.
> >>> 2. The first run_job() callback incurs a slight delay (~1.5ms) while
> >>> resuming the GPU
> >>> 3. Because of that delay, userspace has a chance to queue up enough
> >>> more jobs to require locking/pinning more than the available system
> >>> RAM..
> >>
> >> Is that one or multiple threads submitting jobs?
> >
> > In this case multiple.. but I think it could also happen with a single
> > thread (provided it didn't stall on a fence, directly or indirectly,
> > from an earlier submit), because of how resume and actual job
> > submission happens from scheduler kthread.
> >
> >>> I'm not sure if we want a way to prevent userspace from getting *too*
> >>> far ahead of the kthread.  Or maybe at some point the shrinker should
> >>> sleep on non-idle buffers?
> >>
> >> On the direct reclaim path when invoked from the submit ioctl? In i915
> >> we only shrink idle objects on direct reclaim and leave active ones for
> >> the swapper. It depends on how your locking looks like whether you could
> >> do them, whether there would be coupling of locks and fs-reclaim context.
> >
> > I think the locking is more or less ok, although lockdep is unhappy
> > about one thing[1] which is I think a false warning (ie. not
> > recognizing that we'd already successfully acquired the obj lock via
> > trylock).  We can already reclaim idle bo's in this path.  But the
> > problem with a bunch of submits queued up in the scheduler, is that
> > they are already considered pinned and active.  So at some point we
> > need to sleep (hopefully interruptabley) until they are no longer
> > active, ie. to throttle userspace trying to shove in more submits
> > until some of the enqueued ones have a chance to run and complete.
>
> Odd I did not think trylock could trigger that. Looking at your code it
> indeed seems two trylocks. I am pretty sure we use the same trylock
> trick to avoid it. I am confused..

The sequence is,

1. kref_get_unless_zero()
2. trylock, which succeeds
3. attempt to evict or purge (which may or may not have succeeded)
4. unlock

 ... meanwhile this has raced with submit (aka execbuf) finishing and
retiring and dropping *other* remaining reference to bo...

5. drm_gem_object_put() which triggers drm_gem_object_free()
6. in our free path we acquire the obj lock again and then drop it.
Which arguably is unnecessary and only serves to satisfy some
GEM_WARN_ON(!msm_gem_is_locked(obj)) in code paths that are also used
elsewhere

lockdep doesn't realize the previously successful trylock+unlock
sequence so it assumes that the code that triggered recursion into
shrinker could be holding the objects lock.

>
> Otherwise if you can afford to sleep you can of course throttle
> organically via direct reclaim. Unless I am forgetting some key gotcha -
> it's been a while I've been active in this area.

So, one thing that is awkward about sleeping in this path is that
there is no way to propagate back -EINTR, so we end up doing an
uninterruptible sleep in something that could be called indirectly
from userspace syscall.. i915 seems to deal with this by limiting it
to shrinker being called from kswapd.  I think in the shrinker we want
to know whether it is ok to sleep (ie. not syscall trigggered
codepath, and whether we are under enough memory pressure to justify
sleeping).  For the syscall path, I'm playing with something that lets
me pass __GFP_RETRY_MAYFAIL | __GFP_NOWARN to
shmem_read_mapping_page_gfp(), and then stall after the shrinker has
failed, somewhere where we can make it interruptable.  Ofc, that
doesn't help with all the other random memory allocations which can
fail, so not sure if it will turn out to be a good approach or not.
But I guess pinning the GEM bo's is the single biggest potential
consumer of pages in the submit path, so maybe it will be better than
nothing.

BR,
-R

>
> Regards,
>
> Tvrtko
>
> >
> > BR,
> > -R
> >
> > [1] https://gitlab.freedesktop.org/drm/msm/-/issues/14
> >

  reply	other threads:[~2022-05-26  3:16 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-28  1:06 [PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups Rob Clark
2021-07-28  1:06 ` [PATCH v4 01/13] drm/msm: Docs and misc cleanup Rob Clark
2021-07-28  1:06 ` [PATCH v4 02/13] drm/msm: Small submitqueue creation cleanup Rob Clark
2021-07-28  1:06 ` [PATCH v4 03/13] drm/msm: drop drm_gem_object_put_locked() Rob Clark
2021-07-28  1:06 ` [PATCH v4 04/13] drm: Drop drm_gem_object_put_locked() Rob Clark
2021-07-28  1:06 ` [PATCH v4 05/13] drm/msm/submit: Simplify out-fence-fd handling Rob Clark
2021-07-28  1:06 ` [PATCH v4 06/13] drm/msm: Consolidate submit bo state Rob Clark
2021-07-28  1:06 ` [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr Rob Clark
2021-11-10 15:28   ` Akhil P Oommen
2021-11-10 16:55     ` Rob Clark
2021-11-11 15:53       ` Akhil P Oommen
2021-11-11 17:30         ` [Freedreno] " Rob Clark
2021-07-28  1:06 ` [PATCH v4 08/13] drm/msm: Return ERR_PTR() from submit_create() Rob Clark
2021-07-28  1:06 ` [PATCH v4 09/13] drm/msm: Conversion to drm scheduler Rob Clark
2021-07-28  1:06 ` [PATCH v4 10/13] drm/msm: Drop submit bo_list Rob Clark
2021-07-28  1:06 ` [PATCH v4 11/13] drm/msm: Drop struct_mutex in submit path Rob Clark
2021-07-28  1:06 ` [PATCH v4 12/13] drm/msm: Utilize gpu scheduler priorities Rob Clark
2022-05-23 14:45   ` Tvrtko Ursulin
2022-05-23 22:53     ` Rob Clark
2022-05-24 13:45       ` Tvrtko Ursulin
2022-05-24 14:50         ` Rob Clark
2022-05-25  9:46           ` Tvrtko Ursulin
2022-05-25 13:41             ` Rob Clark
2022-05-25 16:22               ` Tvrtko Ursulin
2022-05-26  3:37                 ` Rob Clark
2022-05-26 11:38                   ` Tvrtko Ursulin
2022-05-27  4:25                     ` [Freedreno] " Rob Clark
2022-06-07 12:43                       ` Tvrtko Ursulin
2022-05-24 14:57         ` Rob Clark
2022-05-25  3:34           ` Rob Clark
2022-05-25 16:11           ` Tvrtko Ursulin
2022-05-26  3:15             ` Rob Clark [this message]
2022-05-26 13:29               ` Tvrtko Ursulin
2022-05-27  4:44                 ` Rob Clark
2021-07-28  1:06 ` [PATCH v4 13/13] drm/msm/gem: Mark active before pinning Rob Clark

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAF6AEGsvmQYjzoFgEMTer3oDmb62y2Hq_unDbq2UEoZ6CA3CSw@mail.gmail.com \
    --to=robdclark@gmail.com \
    --cc=airlied@linux.ie \
    --cc=akhilpo@codeaurora.org \
    --cc=christian.koenig@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=jonathan@marek.ca \
    --cc=jordan@cosmicpenguin.net \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@chromium.org \
    --cc=saiprakash.ranjan@codeaurora.org \
    --cc=sean@poorly.run \
    --cc=smasetty@codeaurora.org \
    --cc=tvrtko.ursulin@linux.intel.com \
    --subject='Re: [PATCH v4 12/13] drm/msm: Utilize gpu scheduler priorities' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).