LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Rob Clark <robdclark@gmail.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: Thu, 26 May 2022 14:29:24 +0100	[thread overview]
Message-ID: <d15cccd3-4b77-992e-23f7-0c4808592a9f@linux.intel.com> (raw)
In-Reply-To: <CAF6AEGsvmQYjzoFgEMTer3oDmb62y2Hq_unDbq2UEoZ6CA3CSw@mail.gmail.com>


On 26/05/2022 04:15, Rob Clark wrote:
> 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.

Ah yes, missed that lock after trylock in msm_gem_shrinker/scan(). Well 
i915 has the same sequence in our shrinker, but the difference is we use 
delayed work to actually free, _and_ use trylock in the delayed worker. 
It does feel a bit inelegant (objects with no reference count which 
cannot be trylocked?!), but as this is the code recently refactored by 
Maarten so I think best try and sync with him for the full story.

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

We play similar games, although by a quick look I am not sure we quite 
manage to honour/propagate signals. This has certainly been a 
historically fiddly area. If you first ask for no reclaim allocations 
and invoke the shrinker manually first, then falling back to a bigger 
hammer, you should be able to do it.

Regards,

Tvrtko

  reply	other threads:[~2022-05-26 13:29 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
2022-05-26 13:29               ` Tvrtko Ursulin [this message]
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=d15cccd3-4b77-992e-23f7-0c4808592a9f@linux.intel.com \
    --to=tvrtko.ursulin@linux.intel.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=robdclark@gmail.com \
    --cc=saiprakash.ranjan@codeaurora.org \
    --cc=sean@poorly.run \
    --cc=smasetty@codeaurora.org \
    --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).