Am 20.10.2017 um 18:51 schrieb Andrey Grodzovsky:
>
>
> On 2017-10-20 12:26 PM, Andres Rodriguez wrote:
>>
>>
>> On 2017-10-20 12:19 PM, Andrey Grodzovsky wrote:
>>>
>>>
>>> On 2017-10-20 11:59 AM, Andres Rodriguez wrote:
>>>>
>>>>
>>>> On 2017-10-20 09:32 AM, Andrey Grodzovsky wrote:
>>>>> Bug: amdgpu_job_free_cb was accessing s_job->s_entity when the
>>>>> allocated
>>>>> amdgpu_ctx (and the entity inside it) were already deallocated from
>>>>> amdgpu_cs_parser_fini.
>>>>>
>>>>> Fix: Save job's priority on it's creation instead of accessing it
>>>>> from
>>>>> s_entity later on.
>>>>
>>>> I'm not sure if this is the correct approach for a fix.
>>>>
>>>> Keeping s_entity as a dangling pointer could result in a similar
>>>> bugs being reintroduced. For example, there would still be a race
>>>> condition between amdgpu_cs_parser_fini() and amdgpu_job_dependency().
>>>
>>> .dependency hook is called only in once place
>>> amd_sched_entity_pop_job, amdgpu_cs_parser_fini will wait (from
>>> amd_sched_entity_fini) for wake_up(&sched->job_scheduled) from
>>> amd_sched_main so I don't see a race here.
>>>
>>>>
>>>>
>>>> Instead, it might be better for the job to grab a reference to the
>>>> context during job_init(), and put that reference on job free.
>>>
>>> Originally it was my thinkimg to, but I consulted Christian and he
>>> advised that quote - "it's not the best idea since
>>> the problem is that when we terminate a process we need to make sure
>>> that all resources are released or at least not hold for much
>>> longer. When we keep the ctx alive with the job we need to also keep
>>> the VM alive and that means we need to keep all the VM page tables
>>> alive".
>>>
>>
>> That makes sense.
>>
>> Since s_entity is tied to the context reference held by the parser,
>> can you set it to NULL when you drop the context reference there?
>
> O am not sure i understand - you want to send s_job->s_entity to NULL
> in amd_sched_entity_fini for each remaining job in the queue ? But all
> the jobs remaining in the queue are destroyed there anyway.
I think what Andres means here is exactly what we planned to do anyway.
Set job->s_entity to NULL as soon as we know that the entity is not used
any more and might be released.
In the long term we should target towards making s_job->s_entity as well
as job->vm superfluous. This way we could even push remaining jobs on a
graveyard entity when we destroy one and timeout.
Alternatively we could look into why wait_event_killable is sometimes
not killable as the name says :)
Maybe we can get to a point where we can finally reboot the system
cleanly even when the GPU is stuck.
Regards,
Christian.
>
> Thanks,
> Andrey
>
>>
>> At least that way we can easily detect misuse of s_entity after it
>> enters a "possibly deleted" state.
>>
>> Regards,
>> Andres
>>
>>> Thanks,
>>> Andrey
>>>
>>>>
>>>> Regards,
>>>> Andres
>>>>
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
>>>>> ---
>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c       | 3 +--
>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c      | 5 ++---
>>>>> Â drivers/gpu/drm/amd/scheduler/gpu_scheduler.c |Â 1 +
>>>>> Â drivers/gpu/drm/amd/scheduler/gpu_scheduler.h | 32
>>>>> ++++++++++++---------------
>>>>> Â 4 files changed, 18 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index f7fceb6..a760b6e 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -1192,8 +1192,7 @@ static int amdgpu_cs_submit(struct
>>>>> amdgpu_cs_parser *p,
>>>>> Â Â Â Â Â job->uf_sequence = seq;
>>>>> Â Â Â Â Â Â Â amdgpu_job_free_resources(job);
>>>>> -Â Â Â amdgpu_ring_priority_get(job->ring,
>>>>> - amd_sched_get_job_priority(&job->base));
>>>>> +Â Â Â amdgpu_ring_priority_get(job->ring, job->base.s_priority);
>>>>> Â Â Â Â Â Â Â trace_amdgpu_cs_ioctl(job);
>>>>> Â Â Â Â Â amd_sched_entity_push_job(&job->base);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> index 0cfc68d..a58e3c5 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> @@ -104,7 +104,7 @@ static void amdgpu_job_free_cb(struct
>>>>> amd_sched_job *s_job)
>>>>> Â {
>>>>> Â Â Â Â Â struct amdgpu_job *job = container_of(s_job, struct
>>>>> amdgpu_job, base);
>>>>> Â -Â Â Â amdgpu_ring_priority_put(job->ring,
>>>>> amd_sched_get_job_priority(s_job));
>>>>> +Â Â Â amdgpu_ring_priority_put(job->ring, s_job->s_priority);
>>>>> Â Â Â Â Â dma_fence_put(job->fence);
>>>>> Â Â Â Â Â amdgpu_sync_free(&job->sync);
>>>>> Â Â Â Â Â amdgpu_sync_free(&job->dep_sync);
>>>>> @@ -141,8 +141,7 @@ int amdgpu_job_submit(struct amdgpu_job *job,
>>>>> struct amdgpu_ring *ring,
>>>>> Â Â Â Â Â job->fence_ctx = entity->fence_context;
>>>>> Â Â Â Â Â *f = dma_fence_get(&job->base.s_fence->finished);
>>>>> Â Â Â Â Â amdgpu_job_free_resources(job);
>>>>> -Â Â Â amdgpu_ring_priority_get(job->ring,
>>>>> - amd_sched_get_job_priority(&job->base));
>>>>> +Â Â Â amdgpu_ring_priority_get(job->ring, job->base.s_priority);
>>>>> Â Â Â Â Â amd_sched_entity_push_job(&job->base);
>>>>> Â Â Â Â Â Â Â return 0;
>>>>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>> index e4d3b4e..1bbbce2 100644
>>>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.c
>>>>> @@ -529,6 +529,7 @@ int amd_sched_job_init(struct amd_sched_job *job,
>>>>> Â {
>>>>> Â Â Â Â Â job->sched = sched;
>>>>> Â Â Â Â Â job->s_entity = entity;
>>>>> +Â Â Â job->s_priority = entity->rq - sched->sched_rq;
>>>>> Â Â Â Â Â job->s_fence = amd_sched_fence_create(entity, owner);
>>>>> Â Â Â Â Â if (!job->s_fence)
>>>>> Â Â Â Â Â Â Â Â Â return -ENOMEM;
>>>>> diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>> b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>> index 52c8e54..3f75b45 100644
>>>>> --- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>> +++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
>>>>> @@ -30,6 +30,19 @@
>>>>> Â struct amd_gpu_scheduler;
>>>>> Â struct amd_sched_rq;
>>>>> Â +enum amd_sched_priority {
>>>>> +Â Â Â AMD_SCHED_PRIORITY_MIN,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_LOW = AMD_SCHED_PRIORITY_MIN,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_NORMAL,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_HIGH_SW,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_HIGH_HW,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_KERNEL,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_MAX,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_INVALID = -1,
>>>>> +Â Â Â AMD_SCHED_PRIORITY_UNSET = -2
>>>>> +};
>>>>> +
>>>>> +
>>>>> Â /**
>>>>> Â Â * A scheduler entity is a wrapper around a job queue or a group
>>>>> Â Â * of other entities. Entities take turns emitting jobs from their
>>>>> @@ -83,6 +96,7 @@ struct amd_sched_job {
>>>>>      struct delayed_work       work_tdr;
>>>>>      uint64_t           id;
>>>>> Â Â Â Â Â atomic_t karma;
>>>>> +Â Â Â enum amd_sched_priority s_priority;
>>>>> Â };
>>>>> Â Â Â extern const struct dma_fence_ops amd_sched_fence_ops_scheduled;
>>>>> @@ -114,18 +128,6 @@ struct amd_sched_backend_ops {
>>>>> Â Â Â Â Â void (*free_job)(struct amd_sched_job *sched_job);
>>>>> Â };
>>>>> Â -enum amd_sched_priority {
>>>>> -Â Â Â AMD_SCHED_PRIORITY_MIN,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_LOW = AMD_SCHED_PRIORITY_MIN,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_NORMAL,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_HIGH_SW,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_HIGH_HW,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_KERNEL,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_MAX,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_INVALID = -1,
>>>>> -Â Â Â AMD_SCHED_PRIORITY_UNSET = -2
>>>>> -};
>>>>> -
>>>>> Â /**
>>>>> Â Â * One scheduler is implemented for each hardware ring
>>>>> Â */
>>>>> @@ -176,10 +178,4 @@ bool amd_sched_dependency_optimized(struct
>>>>> dma_fence* fence,
>>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct amd_sched_entity *entity);
>>>>> Â void amd_sched_job_kickout(struct amd_sched_job *s_job);
>>>>> Â -static inline enum amd_sched_priority
>>>>> -amd_sched_get_job_priority(struct amd_sched_job *job)
>>>>> -{
>>>>> -Â Â Â return (job->s_entity->rq - job->sched->sched_rq);
>>>>> -}
>>>>> -
>>>>> Â #endif
>>>>>
>>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
