From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: gregkh@linuxfoundation.org, daniel.vetter@ffwll.ch,
Felix.Kuehling@amd.com, linux-pci@vger.kernel.org,
amd-gfx@lists.freedesktop.org, helgaas@kernel.org,
dri-devel@lists.freedesktop.org,
"Christian König" <ckoenig.leichtzumerken@gmail.com>,
Alexander.Deucher@amd.com,
"Christian König" <christian.koenig@amd.com>
Subject: Re: [PATCH v5 15/27] drm/scheduler: Fix hang when sched_entity released
Date: Fri, 7 May 2021 12:32:22 -0400 [thread overview]
Message-ID: <e2bd01fc-1d1e-bc76-2187-72bb602fbac7@amd.com> (raw)
In-Reply-To: <YJVq4x4d81rY2GHB@phenom.ffwll.local>
On 2021-05-07 12:29 p.m., Daniel Vetter wrote:
> On Fri, Apr 30, 2021 at 12:10:57PM -0400, Andrey Grodzovsky wrote:
>>
>>
>> On 2021-04-30 2:47 a.m., Christian König wrote:
>>>
>>>
>>> Am 29.04.21 um 19:06 schrieb Andrey Grodzovsky:
>>>>
>>>>
>>>> On 2021-04-29 3:18 a.m., Christian König wrote:
>>>>> I need to take another look at this part when I don't have a
>>>>> massive headache any more.
>>>>>
>>>>> Maybe split the patch set up into different parts, something like:
>>>>> 1. Adding general infrastructure.
>>>>> 2. Making sure all memory is unpolated.
>>>>> 3. Job and fence handling
>>>>
>>>> I am not sure you mean this patch here, maybe another one ?
>>>> Also note you already RBed it.
>>>
>>> No what I meant was to send out the patches before this one as #1 and #2.
>>>
>>> That is the easier stuff which can easily go into the drm-misc-next or
>>> amd-staging-drm-next branch.
>>>
>>> The scheduler stuff certainly need to go into drm-misc-next.
>>>
>>> Christian.
>>
>> Got you. I am fine with it. What we have here is a working hot-unplug
>> code but, one with potential use after free MMIO ranges frpom the zombie
>> device. The followup patches after this patch are all about preventing
>> this and so the patch-set up until this patch including, is functional
>> on it's own. While it's necessary to solve the above issue, it's has
>> complications as can be seen from the discussion with Daniel on later
>> patch in this series. Still, in my opinion it's better to rollout some
>> initial support to hot-unplug without use after free protection then
>> having no support for hot-unplug at all. It will also make the merge
>> work easier as I need to constantly rebase the patches on top latest
>> kernel and solve new regressions.
>>
>> Daniel - given the arguments above can you sound your opinion on this
>> approach ?
>
> I'm all for incrementally landing this, because it's really hard and
> tricky. We might need to go back to some of the decisions, or clarify
> things more, or more headaches and pondering how to fix all the parts
> that works best to make sure there's no nasty races right across hotunplug
> if you're unlucky enough.
>
> But yeah better aim for something and then readjust than bikeshed forever
> out of tree.
>
> Cheers, Daniel
Thanks, I will send out V6 limited in scope up to here and fixing
any relevant comments.
Andrey
>
>>
>> Andrey
>>>
>>>>
>>>> Andrey
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>> Am 28.04.21 um 17:11 schrieb Andrey Grodzovsky:
>>>>>> Problem: If scheduler is already stopped by the time sched_entity
>>>>>> is released and entity's job_queue not empty I encountred
>>>>>> a hang in drm_sched_entity_flush. This is because
>>>>>> drm_sched_entity_is_idle
>>>>>> never becomes false.
>>>>>>
>>>>>> Fix: In drm_sched_fini detach all sched_entities from the
>>>>>> scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
>>>>>> Also wakeup all those processes stuck in sched_entity flushing
>>>>>> as the scheduler main thread which wakes them up is stopped by now.
>>>>>>
>>>>>> v2:
>>>>>> Reverse order of drm_sched_rq_remove_entity and marking
>>>>>> s_entity as stopped to prevent reinserion back to rq due
>>>>>> to race.
>>>>>>
>>>>>> v3:
>>>>>> Drop drm_sched_rq_remove_entity, only modify entity->stopped
>>>>>> and check for it in drm_sched_entity_is_idle
>>>>>>
>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
>>>>>> drivers/gpu/drm/scheduler/sched_main.c | 24
>>>>>> ++++++++++++++++++++++++
>>>>>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> index f0790e9471d1..cb58f692dad9 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> @@ -116,7 +116,8 @@ static bool
>>>>>> drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>>>>>> rmb(); /* for list_empty to work without lock */
>>>>>> if (list_empty(&entity->list) ||
>>>>>> - spsc_queue_count(&entity->job_queue) == 0)
>>>>>> + spsc_queue_count(&entity->job_queue) == 0 ||
>>>>>> + entity->stopped)
>>>>>> return true;
>>>>>> return false;
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> index 908b0b56032d..ba087354d0a8 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> @@ -897,9 +897,33 @@ EXPORT_SYMBOL(drm_sched_init);
>>>>>> */
>>>>>> void drm_sched_fini(struct drm_gpu_scheduler *sched)
>>>>>> {
>>>>>> + struct drm_sched_entity *s_entity;
>>>>>> + int i;
>>>>>> +
>>>>>> if (sched->thread)
>>>>>> kthread_stop(sched->thread);
>>>>>> + for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >=
>>>>>> DRM_SCHED_PRIORITY_MIN; i--) {
>>>>>> + struct drm_sched_rq *rq = &sched->sched_rq[i];
>>>>>> +
>>>>>> + if (!rq)
>>>>>> + continue;
>>>>>> +
>>>>>> + spin_lock(&rq->lock);
>>>>>> + list_for_each_entry(s_entity, &rq->entities, list)
>>>>>> + /*
>>>>>> + * Prevents reinsertion and marks job_queue as idle,
>>>>>> + * it will removed from rq in drm_sched_entity_fini
>>>>>> + * eventually
>>>>>> + */
>>>>>> + s_entity->stopped = true;
>>>>>> + spin_unlock(&rq->lock);
>>>>>> +
>>>>>> + }
>>>>>> +
>>>>>> + /* Wakeup everyone stuck in drm_sched_entity_flush for
>>>>>> this scheduler */
>>>>>> + wake_up_all(&sched->job_scheduled);
>>>>>> +
>>>>>> /* Confirm no work left behind accessing device structures */
>>>>>> cancel_delayed_work_sync(&sched->work_tdr);
>>>>>
>>>
>
next prev parent reply other threads:[~2021-05-07 16:32 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-28 15:11 [PATCH v5 00/27] RFC Support hot device unplug in amdgpu Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 01/27] drm/ttm: Remap all page faults to per process dummy page Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 02/27] drm/ttm: Expose ttm_tt_unpopulate for driver use Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 03/27] drm/amdgpu: Split amdgpu_device_fini into early and late Andrey Grodzovsky
2021-04-29 7:04 ` Christian König
2021-04-30 3:10 ` Alex Deucher
2021-04-30 5:19 ` Lazar, Lijo
2021-04-30 5:39 ` Andrey Grodzovsky
2021-04-30 5:49 ` Lazar, Lijo
2021-04-28 15:11 ` [PATCH v5 04/27] drm/amdkfd: Split kfd suspend from devie exit Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 05/27] drm/amdgpu: Add early fini callback Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 06/27] drm/amdgpu: Handle IOMMU enabled case Andrey Grodzovsky
2021-04-29 7:08 ` Christian König
2021-05-03 20:43 ` Andrey Grodzovsky
2021-05-04 7:03 ` Christian König
2021-05-04 15:43 ` Andrey Grodzovsky
2021-05-05 14:51 ` Andrey Grodzovsky
2021-04-30 3:13 ` Alex Deucher
2021-05-03 18:00 ` Andrey Grodzovsky
2021-05-04 17:05 ` Felix Kuehling
2021-05-05 14:05 ` Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 07/27] drm/amdgpu: Remap all page faults to per process dummy page Andrey Grodzovsky
2021-04-29 7:09 ` Christian König
2021-04-28 15:11 ` [PATCH v5 08/27] PCI: add support for dev_groups to struct pci_device_driver Andrey Grodzovsky
2021-04-28 16:53 ` Bjorn Helgaas
2021-04-29 16:53 ` Andrey Grodzovsky
2021-04-29 19:23 ` Bjorn Helgaas
2021-04-29 20:36 ` Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 09/27] dmr/amdgpu: Move some sysfs attrs creation to default_attr Andrey Grodzovsky
2021-04-28 17:23 ` Bjorn Helgaas
2021-04-29 7:11 ` Christian König
2021-04-28 15:11 ` [PATCH v5 10/27] drm/amdgpu: Guard against write accesses after device removal Andrey Grodzovsky
2021-04-29 7:14 ` Christian König
2021-04-28 15:11 ` [PATCH v5 11/27] drm/sched: Make timeout timer rearm conditional Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 12/27] drm/amdgpu: Prevent any job recoveries after device is unplugged Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 13/27] drm/amdgpu: When filizing the fence driver. stop scheduler first Andrey Grodzovsky
2021-04-29 7:15 ` Christian König
2021-04-29 17:12 ` Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 14/27] drm/amdgpu: Fix hang on device removal Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 15/27] drm/scheduler: Fix hang when sched_entity released Andrey Grodzovsky
2021-04-29 7:18 ` Christian König
2021-04-29 17:06 ` Andrey Grodzovsky
2021-04-30 6:47 ` Christian König
2021-04-30 16:10 ` Andrey Grodzovsky
2021-05-05 13:57 ` Andrey Grodzovsky
2021-05-07 16:29 ` Daniel Vetter
2021-05-07 16:32 ` Andrey Grodzovsky [this message]
2021-04-28 15:11 ` [PATCH v5 16/27] drm/amdgpu: Unmap all MMIO mappings Andrey Grodzovsky
2021-04-29 7:19 ` Christian König
2021-04-29 16:55 ` Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 17/27] drm/amdgpu: Add rw_sem to pushing job into sched queue Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 18/27] drm/sched: Expose drm_sched_entity_kill_jobs Andrey Grodzovsky
2021-04-28 15:11 ` [PATCH v5 19/27] drm/amdgpu: Finilise device fences on device remove Andrey Grodzovsky
2021-04-28 15:20 ` Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit Andrey Grodzovsky
2021-04-29 11:23 ` Daniel Vetter
2021-04-29 11:32 ` Daniel Vetter
2021-04-29 16:04 ` Andrey Grodzovsky
2021-04-29 16:15 ` Felix Kuehling
2021-04-29 16:21 ` Andrey Grodzovsky
2021-04-29 16:29 ` Felix Kuehling
2021-04-29 16:33 ` Andrey Grodzovsky
2021-04-29 19:05 ` Daniel Vetter
2021-04-29 20:34 ` Andrey Grodzovsky
2021-04-30 10:25 ` Daniel Vetter
2021-04-30 17:27 ` Andrey Grodzovsky
2021-05-05 13:57 ` Andrey Grodzovsky
2021-05-06 9:40 ` Daniel Vetter
2021-05-06 16:25 ` Andrey Grodzovsky
2021-05-07 9:11 ` Daniel Vetter
2021-05-07 15:39 ` Andrey Grodzovsky
2021-05-07 16:24 ` Daniel Vetter
2021-05-07 18:00 ` Andrey Grodzovsky
2021-05-10 15:46 ` Daniel Vetter
2021-04-28 15:12 ` [PATCH v5 21/27] drm/amdgpu: Add support for hot-unplug feature at DRM level Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 22/27] drm/amd/display: Scope all DM queued work with drm_dev_enter/exit Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 23/27] drm/amd/powerplay: Scope all PM " Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 24/27] drm/amdkfd: Scope all KFD " Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 25/27] drm/amdgpu: Scope all amdgpu " Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 26/27] drm/amd/display: Remove superflous drm_mode_config_cleanup Andrey Grodzovsky
2021-04-28 15:12 ` [PATCH v5 27/27] drm/amdgpu: Verify DMA opearations from device are done Andrey Grodzovsky
2021-04-28 17:07 ` [PATCH v5 00/27] RFC Support hot device unplug in amdgpu Bjorn Helgaas
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=e2bd01fc-1d1e-bc76-2187-72bb602fbac7@amd.com \
--to=andrey.grodzovsky@amd.com \
--cc=Alexander.Deucher@amd.com \
--cc=Felix.Kuehling@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=ckoenig.leichtzumerken@gmail.com \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=helgaas@kernel.org \
--cc=linux-pci@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).