[PATCH 5/5] drm/amd/sched: signal and free remaining fences in amd_sched_entity_fini
Christian König
christian.koenig at amd.com
Mon Oct 9 12:33:49 UTC 2017
Am 09.10.2017 um 13:27 schrieb Nicolai Hähnle:
> On 09.10.2017 13:12, Christian König wrote:
>>>
>>>> Nicolai, how hard would it be to handle ENODEV as failure for all
>>>> currently existing contexts?
>>>
>>> Impossible? "All currently existing contexts" is not a well-defined
>>> concept when multiple drivers co-exist in the same process.
>>
>> Ok, let me refine the question: I assume there are resources "shared"
>> between contexts like binary shader code for example which needs to
>> be reuploaded when VRAM is lost.
>>
>> How hard would it be to handle that correctly?
>
> Okay, that makes more sense :)
>
> With the current interface it's still pretty difficult, but if we
> could get a new per-device query ioctl which returns a "VRAM loss
> counter", it would be reasonably straight-forward.
The problem with the VRAM lost counter is that this isn't save either.
E.g. you could have an application which uploads shaders, a GPU reset
happens and VRAM is lost and then the application creates a new context
and makes submission with broken shader binaries.
So I would still vote for a separate IOCTL to reset the VRAM lost state
which is called *before" user space starts to reupload
shader/descriptors etc...
This way you also catch the case when another reset happens while you
reupload things.
>
>>> And what would be the purpose of this? If it's to support VRAM loss,
>>> having a per-context VRAM loss counter would enable each context to
>>> signal ECANCELED separately.
>>
>> I thought of that on top of the -ENODEV handling.
>>
>> In other words when we see -ENODEV we call an IOCTL to let the kernel
>> know we noticed that something is wrong and then reinit all shared
>> resources in userspace.
>>
>> All existing context will still see -ECANCELED when we drop their
>> command submission, but new contexts would at least not cause a new
>> lockup immediately because their shader binaries are corrupted.
>
> I don't think we need -ENODEV for this. We just need -ECANCELED to be
> returned when a submission is rejected due to reset (hang or VRAM loss).
>
> Mesa would keep a shadow of the VRAM loss counter in pipe_screen and
> pipe_context, and query the kernel's counter when it encounters
> -ECANCELED. Each context would then know to drop the CS it's built up
> so far and restart based on comparing the VRAM loss counter of
> pipe_screen to that of pipe_context, and similarly we could keep a
> copy of the VRAM loss counter for important buffer objects like shader
> binaries, descriptors, etc.
>
> This seems more robust to me than relying only on an ENODEV. We'd most
> likely keep some kind of VRAM loss counter in Mesa *anyway* (we don't
> maintain a list of all shaders, for example, and we can't nuke
> important per-context across threads), and synthesizing such a counter
> from ENODEVs is not particularly robust (what if multiple ENODEVs
> occur for the same loss event?).
>
> BTW, I still don't like ENODEV. It seems more like the kind of error
> code you'd return with hot-pluggable GPUs where the device can
> physically disappear...
Yeah, ECANCELED sounds like a better alternative. But I think we should
still somehow note the fatality of loosing VRAM to userspace.
How about ENODATA or EBADFD?
Regards,
Christian.
>
> Cheers,
> Nicolai
>
>
>>
>> Regards,
>> Christian.
>>
>> Am 09.10.2017 um 13:04 schrieb Nicolai Hähnle:
>>> On 09.10.2017 12:59, Christian König wrote:
>>>> Nicolai, how hard would it be to handle ENODEV as failure for all
>>>> currently existing contexts?
>>>
>>> Impossible? "All currently existing contexts" is not a well-defined
>>> concept when multiple drivers co-exist in the same process.
>>>
>>> And what would be the purpose of this? If it's to support VRAM loss,
>>> having a per-context VRAM loss counter would enable each context to
>>> signal ECANCELED separately.
>>>
>>> Cheers,
>>> Nicolai
>>>
>>>
>>>>
>>>> Monk, would it be ok with you when we return ENODEV only for
>>>> existing context when VRAM is lost and/or we have a strict mode GPU
>>>> reset? E.g. newly created contexts would continue work as they should.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 09.10.2017 um 12:49 schrieb Nicolai Hähnle:
>>>>> Hi Monk,
>>>>>
>>>>> Yes, you're right, we're only using ECANCELED internally. But as a
>>>>> consequence, Mesa would already handle a kernel error of ECANCELED
>>>>> on context loss correctly :)
>>>>>
>>>>> Cheers,
>>>>> Nicolai
>>>>>
>>>>> On 09.10.2017 12:35, Liu, Monk wrote:
>>>>>> Hi Christian
>>>>>>
>>>>>> You reject some of my patches that returns -ENODEV, with the
>>>>>> cause that MESA doesn't do the handling on -ENODEV
>>>>>>
>>>>>> But if Nicolai can confirm that MESA do have a handling on
>>>>>> -ECANCELED, then we need to overall align our error code, on
>>>>>> detail below IOCTL can return error code:
>>>>>>
>>>>>> Amdgpu_cs_ioctl
>>>>>> Amdgpu_cs_wait_ioctl
>>>>>> Amdgpu_cs_wait_fences_ioctl
>>>>>> Amdgpu_info_ioctl
>>>>>>
>>>>>>
>>>>>> My patches is:
>>>>>> return -ENODEV on cs_ioctl if the context is detected guilty,
>>>>>> also return -ENODEV on cs_wait|cs_wait_fences if the fence is
>>>>>> signaled but with error -ETIME,
>>>>>> also return -ENODEV on info_ioctl so UMD can query if gpu reset
>>>>>> happened after the process created (because for strict mode we
>>>>>> block process instead of context)
>>>>>>
>>>>>>
>>>>>> according to Nicolai:
>>>>>>
>>>>>> amdgpu_cs_ioctl *can* return -ECANCELED, but to be frankly
>>>>>> speaking, kernel part doesn't have any place with "-ECANCELED" so
>>>>>> this solution on MESA side doesn't align with *current* amdgpu
>>>>>> driver,
>>>>>> which only return 0 on success or -EINVALID on other error but
>>>>>> definitely no "-ECANCELED" error code,
>>>>>>
>>>>>> so if we talking about community rules we shouldn't let MESA
>>>>>> handle -ECANCELED , we should have a unified error code
>>>>>>
>>>>>> + Marek
>>>>>>
>>>>>> BR Monk
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Haehnle, Nicolai
>>>>>> Sent: 2017年10月9日 18:14
>>>>>> To: Koenig, Christian <Christian.Koenig at amd.com>; Liu, Monk
>>>>>> <Monk.Liu at amd.com>; Nicolai Hähnle <nhaehnle at gmail.com>;
>>>>>> amd-gfx at lists.freedesktop.org
>>>>>> Subject: Re: [PATCH 5/5] drm/amd/sched: signal and free remaining
>>>>>> fences in amd_sched_entity_fini
>>>>>>
>>>>>> On 09.10.2017 10:02, Christian König wrote:
>>>>>>>> For gpu reset patches (already submitted to pub) I would make
>>>>>>>> kernel
>>>>>>>> return -ENODEV if the waiting fence (in cs_wait or wait_fences
>>>>>>>> IOCTL)
>>>>>>>> founded as error, that way UMD would run into robust extension
>>>>>>>> path
>>>>>>>> and considering the GPU hang occurred,
>>>>>>> Well that is only closed source behavior which is completely
>>>>>>> irrelevant for upstream development.
>>>>>>>
>>>>>>> As far as I know we haven't pushed the change to return -ENODEV
>>>>>>> upstream.
>>>>>>
>>>>>> FWIW, radeonsi currently expects -ECANCELED on CS submissions and
>>>>>> treats those as context lost. Perhaps we could use the same error
>>>>>> on fences?
>>>>>> That makes more sense to me than -ENODEV.
>>>>>>
>>>>>> Cheers,
>>>>>> Nicolai
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>> Am 09.10.2017 um 08:42 schrieb Liu, Monk:
>>>>>>>> Christian
>>>>>>>>
>>>>>>>>> It would be really nice to have an error code set on
>>>>>>>>> s_fence->finished before it is signaled, use
>>>>>>>>> dma_fence_set_error()
>>>>>>>>> for this.
>>>>>>>> For gpu reset patches (already submitted to pub) I would make
>>>>>>>> kernel
>>>>>>>> return -ENODEV if the waiting fence (in cs_wait or wait_fences
>>>>>>>> IOCTL)
>>>>>>>> founded as error, that way UMD would run into robust extension
>>>>>>>> path
>>>>>>>> and considering the GPU hang occurred,
>>>>>>>>
>>>>>>>> Don't know if this is expected for the case of normal process
>>>>>>>> being
>>>>>>>> killed or crashed like Nicolai hit ... since there is no gpu
>>>>>>>> hang hit
>>>>>>>>
>>>>>>>>
>>>>>>>> BR Monk
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On
>>>>>>>> Behalf Of Christian K?nig
>>>>>>>> Sent: 2017年9月28日 23:01
>>>>>>>> To: Nicolai Hähnle <nhaehnle at gmail.com>;
>>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>>> Cc: Haehnle, Nicolai <Nicolai.Haehnle at amd.com>
>>>>>>>> Subject: Re: [PATCH 5/5] drm/amd/sched: signal and free remaining
>>>>>>>> fences in amd_sched_entity_fini
>>>>>>>>
>>>>>>>> Am 28.09.2017 um 16:55 schrieb Nicolai Hähnle:
>>>>>>>>> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>>>>>>>>
>>>>>>>>> Highly concurrent Piglit runs can trigger a race condition
>>>>>>>>> where a
>>>>>>>>> pending SDMA job on a buffer object is never executed because the
>>>>>>>>> corresponding process is killed (perhaps due to a crash).
>>>>>>>>> Since the
>>>>>>>>> job's fences were never signaled, the buffer object was
>>>>>>>>> effectively
>>>>>>>>> leaked. Worse, the buffer was stuck wherever it happened to be at
>>>>>>>>> the time, possibly in VRAM.
>>>>>>>>>
>>>>>>>>> The symptom was user space processes stuck in interruptible waits
>>>>>>>>> with kernel stacks like:
>>>>>>>>>
>>>>>>>>> [<ffffffffbc5e6722>] dma_fence_default_wait+0x112/0x250
>>>>>>>>> [<ffffffffbc5e6399>] dma_fence_wait_timeout+0x39/0xf0
>>>>>>>>> [<ffffffffbc5e82d2>]
>>>>>>>>> reservation_object_wait_timeout_rcu+0x1c2/0x300
>>>>>>>>> [<ffffffffc03ce56f>]
>>>>>>>>> ttm_bo_cleanup_refs_and_unlock+0xff/0x1a0
>>>>>>>>> [ttm]
>>>>>>>>> [<ffffffffc03cf1ea>] ttm_mem_evict_first+0xba/0x1a0 [ttm]
>>>>>>>>> [<ffffffffc03cf611>] ttm_bo_mem_space+0x341/0x4c0 [ttm]
>>>>>>>>> [<ffffffffc03cfc54>] ttm_bo_validate+0xd4/0x150 [ttm]
>>>>>>>>> [<ffffffffc03cffbd>] ttm_bo_init_reserved+0x2ed/0x420
>>>>>>>>> [ttm]
>>>>>>>>> [<ffffffffc042f523>]
>>>>>>>>> amdgpu_bo_create_restricted+0x1f3/0x470
>>>>>>>>> [amdgpu]
>>>>>>>>> [<ffffffffc042f9fa>] amdgpu_bo_create+0xda/0x220 [amdgpu]
>>>>>>>>> [<ffffffffc04349ea>] amdgpu_gem_object_create+0xaa/0x140
>>>>>>>>> [amdgpu]
>>>>>>>>> [<ffffffffc0434f97>] amdgpu_gem_create_ioctl+0x97/0x120
>>>>>>>>> [amdgpu]
>>>>>>>>> [<ffffffffc037ddba>] drm_ioctl+0x1fa/0x480 [drm]
>>>>>>>>> [<ffffffffc041904f>] amdgpu_drm_ioctl+0x4f/0x90 [amdgpu]
>>>>>>>>> [<ffffffffbc23db33>] do_vfs_ioctl+0xa3/0x5f0
>>>>>>>>> [<ffffffffbc23e0f9>] SyS_ioctl+0x79/0x90
>>>>>>>>> [<ffffffffbc864ffb>] entry_SYSCALL_64_fastpath+0x1e/0xad
>>>>>>>>> [<ffffffffffffffff>] 0xffffffffffffffff
>>>>>>>>>
>>>>>>>>> Signed-off-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>>>>>>>> Acked-by: Christian König <christian.koenig at amd.com>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/amd/scheduler/gpu_scheduler.c | 7 ++++++-
>>>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>>>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>>>>> index 54eb77cffd9b..32a99e980d78 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>>>>>> @@ -220,22 +220,27 @@ void amd_sched_entity_fini(struct
>>>>>>>>> amd_gpu_scheduler *sched,
>>>>>>>>> amd_sched_entity_is_idle(entity));
>>>>>>>>> amd_sched_rq_remove_entity(rq, entity);
>>>>>>>>> if (r) {
>>>>>>>>> struct amd_sched_job *job;
>>>>>>>>> /* Park the kernel for a moment to make sure it isn't
>>>>>>>>> processing
>>>>>>>>> * our enity.
>>>>>>>>> */
>>>>>>>>> kthread_park(sched->thread);
>>>>>>>>> kthread_unpark(sched->thread);
>>>>>>>>> - while (kfifo_out(&entity->job_queue, &job, sizeof(job)))
>>>>>>>>> + while (kfifo_out(&entity->job_queue, &job,
>>>>>>>>> sizeof(job))) {
>>>>>>>>> + struct amd_sched_fence *s_fence = job->s_fence;
>>>>>>>>> + amd_sched_fence_scheduled(s_fence);
>>>>>>>> It would be really nice to have an error code set on
>>>>>>>> s_fence->finished before it is signaled, use
>>>>>>>> dma_fence_set_error() for this.
>>>>>>>>
>>>>>>>> Additional to that it would be nice to note in the subject line
>>>>>>>> that
>>>>>>>> this is a rather important bug fix.
>>>>>>>>
>>>>>>>> With that fixed the whole series is Reviewed-by: Christian König
>>>>>>>> <christian.koenig at amd.com>.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> + amd_sched_fence_finished(s_fence);
>>>>>>>>> + dma_fence_put(&s_fence->finished);
>>>>>>>>> sched->ops->free_job(job);
>>>>>>>>> + }
>>>>>>>>> }
>>>>>>>>> kfifo_free(&entity->job_queue);
>>>>>>>>> }
>>>>>>>>> static void amd_sched_entity_wakeup(struct dma_fence *f,
>>>>>>>>> struct
>>>>>>>>> dma_fence_cb *cb)
>>>>>>>>> {
>>>>>>>>> struct amd_sched_entity *entity =
>>>>>>>>> container_of(cb, struct amd_sched_entity, cb);
>>>>>>>>> entity->dependency = NULL;
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the amd-gfx
mailing list