On Mon, Jul 30, 2018 at 4:33 PM Christian König < ckoenig.leichtzumerken@gmail.com> wrote:

We removed the redundancy of having an extra scheduler field, so we can't set the rq to NULL any more or otherwise won't know which scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König christian.koenig@amd.com

Good catch.

Acked-by: Nayan Deshmukh nayan26deshmukh@gmail.com

---

drivers/gpu/drm/scheduler/gpu_scheduler.c | 35 +++++++------------------------ 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index f563e4fbb4b6..1b733229201e 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, } EXPORT_SYMBOL(drm_sched_entity_init);

-/**

• • drm_sched_entity_is_initialized - Query if entity is initialized
• • @sched: Pointer to scheduler instance
• • @entity: The pointer to a valid scheduler entity
• • return true if entity is initialized, false otherwise

-*/ -static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler *sched,

•                                       struct drm_sched_entity
  

*entity) -{

•   return entity->rq != NULL &&
  

•           entity->rq->sched == sched;
  

-}

/**

• drm_sched_entity_is_idle - Check if entity is idle

@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) { rmb();

•   if (!entity->rq || spsc_queue_peek(&entity->job_queue) == NULL)
  

•   if (list_empty(&entity->list) ||
  

•       spsc_queue_peek(&entity->job_queue) == NULL)
            return true;
  
    return false;
  

@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) long ret = timeout;

    sched = entity->rq->sched;

•   if (!drm_sched_entity_is_initialized(sched, entity))
  

•           return ret;
    /**
     * The client will not queue more IBs during this fini, consume
  

existing * queued IBs or discard them on SIGKILL @@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) last_user = cmpxchg(&entity->last_user, current->group_leader, NULL); if ((!last_user || last_user == current->group_leader) && (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))

•           drm_sched_entity_set_rq(entity, NULL);
  

•           drm_sched_rq_remove_entity(entity->rq, entity);
  
    return ret;
  

} @@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) struct drm_gpu_scheduler *sched;

    sched = entity->rq->sched;

•   drm_sched_entity_set_rq(entity, NULL);
  

•   drm_sched_rq_remove_entity(entity->rq, entity);
  
    /* Consumption of existing IBs wasn't completed. Forcefully
     * remove them here.
  

@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct drm_sched_entity *entity, if (entity->rq == rq) return;

•   spin_lock(&entity->rq_lock);
  

•   if (entity->rq)
  

•           drm_sched_rq_remove_entity(entity->rq, entity);
  

•   BUG_ON(!rq);
  
  

•   spin_lock(&entity->rq_lock);
  

•   drm_sched_rq_remove_entity(entity->rq, entity);
    entity->rq = rq;
  

•   if (rq)
  

•           drm_sched_rq_add_entity(rq, entity);
  

•   drm_sched_rq_add_entity(rq, entity);
    spin_unlock(&entity->rq_lock);
  

} EXPORT_SYMBOL(drm_sched_entity_set_rq); -- 2.14.1

Am 30.07.2018 um 22:51 schrieb Andrey Grodzovsky:

On 07/30/2018 09:30 AM, Nayan Deshmukh wrote:

...

On Mon, Jul 30, 2018 at 4:33 PM Christian König <ckoenig.leichtzumerken@gmail.com mailto:ckoenig.leichtzumerken@gmail.com> wrote:

We removed the redundancy of having an extra scheduler field, so we
can't set the rq to NULL any more or otherwise won't know which
scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König <christian.koenig@amd.com
<mailto:christian.koenig@amd.com>>

Good catch.

Acked-by: Nayan Deshmukh <nayan26deshmukh@gmail.com mailto:nayan26deshmukh@gmail.com>

---
 drivers/gpu/drm/scheduler/gpu_scheduler.c | 35
+++++++------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index f563e4fbb4b6..1b733229201e 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -198,21 +198,6 @@ int drm_sched_entity_init(struct
drm_sched_entity *entity,
 }
 EXPORT_SYMBOL(drm_sched_entity_init);

-/**
- * drm_sched_entity_is_initialized - Query if entity is initialized
- *
- * @sched: Pointer to scheduler instance
- * @entity: The pointer to a valid scheduler entity
- *
- * return true if entity is initialized, false otherwise
-*/
-static bool drm_sched_entity_is_initialized(struct
drm_gpu_scheduler *sched,
-                                           struct
drm_sched_entity *entity)
-{
-       return entity->rq != NULL &&
-               entity->rq->sched == sched;
-}
-
 /**
  * drm_sched_entity_is_idle - Check if entity is idle
  *
@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct
drm_sched_entity *entity)
 {
        rmb();

-       if (!entity->rq || spsc_queue_peek(&entity->job_queue) ==
NULL)
+       if (list_empty(&entity->list) ||
+           spsc_queue_peek(&entity->job_queue) == NULL)
                return true;

        return false;
@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct
drm_sched_entity *entity, long timeout)
        long ret = timeout;

        sched = entity->rq->sched;
-       if (!drm_sched_entity_is_initialized(sched, entity))
-               return ret;

Just to be clear, you remove this function because it's redundant to call it?

Yes, exactly.

Christian.

Andrey

...

/**
         * The client will not queue more IBs during this fini,
consume existing
         * queued IBs or discard them on SIGKILL
@@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct
drm_sched_entity *entity, long timeout)
        last_user = cmpxchg(&entity->last_user,
current->group_leader, NULL);
        if ((!last_user || last_user == current->group_leader) &&
            (current->flags & PF_EXITING) && (current->exit_code
== SIGKILL))
-               drm_sched_entity_set_rq(entity, NULL);
+               drm_sched_rq_remove_entity(entity->rq, entity);

        return ret;
 }
@@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct
drm_sched_entity *entity)
        struct drm_gpu_scheduler *sched;

        sched = entity->rq->sched;
-       drm_sched_entity_set_rq(entity, NULL);
+       drm_sched_rq_remove_entity(entity->rq, entity);

        /* Consumption of existing IBs wasn't completed. Forcefully
         * remove them here.
@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct
drm_sched_entity *entity,
        if (entity->rq == rq)
                return;

-       spin_lock(&entity->rq_lock);
-
-       if (entity->rq)
-               drm_sched_rq_remove_entity(entity->rq, entity);
+       BUG_ON(!rq);

+       spin_lock(&entity->rq_lock);
+       drm_sched_rq_remove_entity(entity->rq, entity);
        entity->rq = rq;
-       if (rq)
-               drm_sched_rq_add_entity(rq, entity);
-
+       drm_sched_rq_add_entity(rq, entity);
        spin_unlock(&entity->rq_lock);
 }
 EXPORT_SYMBOL(drm_sched_entity_set_rq);
-- 
2.14.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thinking again about this change and 53d5f1e drm/scheduler: move idle entities to scheduler with less load v2

Looks to me like use case for which fc9a539 drm/scheduler: add NULL pointer check for run queue (v2) was done

will not work anymore.

First of all in drm_sched_entity_push_job, 'if (!entity->rq)' will never be true any more since we stopped entity->rq to NULL in

drm_sched_entity_flush.

Second of all, even after we removed the entity from rq in drm_sched_entity_flush to terminate any subsequent submissions

to the queue the other thread doing push job can just acquire again a run queue

from drm_sched_entity_get_free_sched and continue submission so you can't substitute ' if (!entity->rq)' to 'if (list_empty(&entity->list))'.

What about adding a 'stopped' flag to drm_sched_entity to be set in drm_sched_entity_flush and in

drm_sched_entity_push_job check for  'if (!entity->stopped)' instead of  ' if (!entity->rq)' ?

Andrey

On 07/30/2018 07:03 AM, Christian König wrote:

We removed the redundancy of having an extra scheduler field, so we can't set the rq to NULL any more or otherwise won't know which scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/scheduler/gpu_scheduler.c | 35 +++++++------------------------ 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index f563e4fbb4b6..1b733229201e 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, } EXPORT_SYMBOL(drm_sched_entity_init);

-/**

• • drm_sched_entity_is_initialized - Query if entity is initialized
• • @sched: Pointer to scheduler instance
• • @entity: The pointer to a valid scheduler entity
• • return true if entity is initialized, false otherwise

-*/ -static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler *sched,

• 			    struct drm_sched_entity *entity)
  

-{

• return entity->rq != NULL &&

• entity->rq->sched == sched;
  

-}

• /**
  • drm_sched_entity_is_idle - Check if entity is idle

@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) { rmb();

• if (!entity->rq || spsc_queue_peek(&entity->job_queue) == NULL)

• if (list_empty(&entity->list) ||

•    spsc_queue_peek(&entity->job_queue) == NULL)
  

  return true;

  return false;

@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) long ret = timeout;

sched = entity->rq->sched;

• if (!drm_sched_entity_is_initialized(sched, entity))

• return ret;
  

  /**
  • The client will not queue more IBs during this fini, consume existing
  • queued IBs or discard them on SIGKILL

@@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) last_user = cmpxchg(&entity->last_user, current->group_leader, NULL); if ((!last_user || last_user == current->group_leader) && (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))

• drm_sched_entity_set_rq(entity, NULL);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  
  

  return ret; }

@@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) struct drm_gpu_scheduler *sched;

sched = entity->rq->sched;

• drm_sched_entity_set_rq(entity, NULL);

• drm_sched_rq_remove_entity(entity->rq, entity);

  /* Consumption of existing IBs wasn't completed. Forcefully

  • remove them here.

@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct drm_sched_entity *entity, if (entity->rq == rq) return;

• spin_lock(&entity->rq_lock);
• if (entity->rq)

• drm_sched_rq_remove_entity(entity->rq, entity);
  

• BUG_ON(!rq);

• spin_lock(&entity->rq_lock);

• drm_sched_rq_remove_entity(entity->rq, entity); entity->rq = rq;

• if (rq)

• drm_sched_rq_add_entity(rq, entity);
  

• drm_sched_rq_add_entity(rq, entity); spin_unlock(&entity->rq_lock); } EXPORT_SYMBOL(drm_sched_entity_set_rq);

On Thu, Aug 2, 2018 at 12:12 PM Christian König < ckoenig.leichtzumerken@gmail.com> wrote:

Am 02.08.2018 um 00:25 schrieb Andrey Grodzovsky:

...

Thinking again about this change and 53d5f1e drm/scheduler: move idle entities to scheduler with less load v2

Looks to me like use case for which fc9a539 drm/scheduler: add NULL pointer check for run queue (v2) was done

will not work anymore.

First of all in drm_sched_entity_push_job, 'if (!entity->rq)' will never be true any more since we stopped entity->rq to NULL in

drm_sched_entity_flush.

Good point, going to remove that.

...

Second of all, even after we removed the entity from rq in drm_sched_entity_flush to terminate any subsequent submissions

to the queue the other thread doing push job can just acquire again a run queue

from drm_sched_entity_get_free_sched and continue submission

Hi Christian

That is actually desired.

When another process is now using the entity to submit jobs adding it back to the rq is actually the right thing to do cause the entity is still in use.

I am not aware of the usecase so I might just be rambling. but if the thread/process that created the entity has called drm_sched_entity_flush then we shouldn't allow other processes to push jobs to that entity.

Nayan

Christian.

...

so you can't substitute ' if (!entity->rq)' to 'if (list_empty(&entity->list))'.

What about adding a 'stopped' flag to drm_sched_entity to be set in drm_sched_entity_flush and in

drm_sched_entity_push_job check for 'if (!entity->stopped)' instead of ' if (!entity->rq)' ?

Andrey

On 07/30/2018 07:03 AM, Christian König wrote:

...

We removed the redundancy of having an extra scheduler field, so we can't set the rq to NULL any more or otherwise won't know which scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/scheduler/gpu_scheduler.c | 35 +++++++------------------------ 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index f563e4fbb4b6..1b733229201e 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, } EXPORT_SYMBOL(drm_sched_entity_init); -/**

• • drm_sched_entity_is_initialized - Query if entity is initialized
• • @sched: Pointer to scheduler instance
• • @entity: The pointer to a valid scheduler entity
• • return true if entity is initialized, false otherwise

-*/ -static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler *sched,

•                    struct drm_sched_entity *entity)
  

-{

• return entity->rq != NULL &&

•    entity->rq->sched == sched;
  

-}

• /**
  • drm_sched_entity_is_idle - Check if entity is idle

@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) { rmb();

• if (!entity->rq || spsc_queue_peek(&entity->job_queue) == NULL)

• if (list_empty(&entity->list) ||

•    spsc_queue_peek(&entity->job_queue) == NULL)
      return true;
    return false;
  

@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) long ret = timeout; sched = entity->rq->sched;

• if (!drm_sched_entity_is_initialized(sched, entity))

•    return ret;
  /**
   * The client will not queue more IBs during this fini, consume
  

existing * queued IBs or discard them on SIGKILL @@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) last_user = cmpxchg(&entity->last_user, current->group_leader, NULL); if ((!last_user || last_user == current->group_leader) && (current->flags & PF_EXITING) && (current->exit_code == SIGKILL))

•    drm_sched_entity_set_rq(entity, NULL);
  

•    drm_sched_rq_remove_entity(entity->rq, entity);
    return ret;
  

  }

@@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) struct drm_gpu_scheduler *sched; sched = entity->rq->sched;

• drm_sched_entity_set_rq(entity, NULL);

• drm_sched_rq_remove_entity(entity->rq, entity); /* Consumption of existing IBs wasn't completed. Forcefully
  • remove them here.

@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct drm_sched_entity *entity, if (entity->rq == rq) return;

• spin_lock(&entity->rq_lock);
• if (entity->rq)

•    drm_sched_rq_remove_entity(entity->rq, entity);
  

• BUG_ON(!rq);
  • spin_lock(&entity->rq_lock);
• drm_sched_rq_remove_entity(entity->rq, entity); entity->rq = rq;

• if (rq)

•    drm_sched_rq_add_entity(rq, entity);
  

• drm_sched_rq_add_entity(rq, entity); spin_unlock(&entity->rq_lock); } EXPORT_SYMBOL(drm_sched_entity_set_rq);

On 08/02/2018 02:47 AM, Nayan Deshmukh wrote:

On Thu, Aug 2, 2018 at 12:12 PM Christian König <ckoenig.leichtzumerken@gmail.com mailto:ckoenig.leichtzumerken@gmail.com> wrote:

Am 02.08.2018 um 00:25 schrieb Andrey Grodzovsky:
> Thinking again about this change and 53d5f1e drm/scheduler: move
idle
> entities to scheduler with less load v2
>
> Looks to me like use case for which fc9a539 drm/scheduler: add NULL
> pointer check for run queue (v2) was done
>
> will not work anymore.
>
> First of all in drm_sched_entity_push_job, 'if (!entity->rq)' will
> never be true any more since we stopped entity->rq to NULL in
>
> drm_sched_entity_flush.

Good point, going to remove that.

So basically we don't need that if (...){ return; } in drm_sched_entity_push_job any more ?

>
> Second of all, even after we removed the entity from rq in
> drm_sched_entity_flush to terminate any subsequent submissions
>
> to the queue the other thread doing push job can just acquire
again a
> run queue
>
> from drm_sched_entity_get_free_sched and continue submission

Hi Christian

That is actually desired.

When another process is now using the entity to submit jobs adding it
back to the rq is actually the right thing to do cause the entity is
still in use.

Yes, no problem if it's another process. But what about another thread from same process ? Is it a possible use case that 2 threads from same process submit to same entity concurrently ? If so and we specifically kill one, the other will not stop event if we want him to because current code makes him just require a rq for him self.

I am not aware of the usecase so I might just be rambling. but if the thread/process that created the entity has called drm_sched_entity_flush then we shouldn't allow other processes to push jobs to that entity.

Nayan

Christian.

We don't really know who created the entity, the creation could be by X itself and then it's passed to other process for use. Check 'drm/scheduler: only kill entity if last user is killed v2', the idea is that if by the time you want to kill this entity another process (not another thread from your process - i.e. current->group_leader != last_user in drm_sched_entity_flush) already started to use this entity just let it be.

Andrey

> so you can't substitute ' if (!entity->rq)' to 'if
> (list_empty(&entity->list))'.
>
> What about adding a 'stopped' flag to drm_sched_entity to be set in
> drm_sched_entity_flush and in
>
> drm_sched_entity_push_job check for  'if (!entity->stopped)'
instead
> of  ' if (!entity->rq)' ?
>
> Andrey
>
>
> On 07/30/2018 07:03 AM, Christian König wrote:
>> We removed the redundancy of having an extra scheduler field, so we
>> can't set the rq to NULL any more or otherwise won't know which
>> scheduler to use for the cleanup.
>>
>> Just remove the entity from the scheduling list instead.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com
<mailto:christian.koenig@amd.com>>
>> ---
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 35
>> +++++++------------------------
>>   1 file changed, 8 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index f563e4fbb4b6..1b733229201e 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct
>> drm_sched_entity *entity,
>>   }
>>   EXPORT_SYMBOL(drm_sched_entity_init);
>>   -/**
>> - * drm_sched_entity_is_initialized - Query if entity is
initialized
>> - *
>> - * @sched: Pointer to scheduler instance
>> - * @entity: The pointer to a valid scheduler entity
>> - *
>> - * return true if entity is initialized, false otherwise
>> -*/
>> -static bool drm_sched_entity_is_initialized(struct
drm_gpu_scheduler
>> *sched,
>> -                        struct drm_sched_entity *entity)
>> -{
>> -    return entity->rq != NULL &&
>> -        entity->rq->sched == sched;
>> -}
>> -
>>   /**
>>    * drm_sched_entity_is_idle - Check if entity is idle
>>    *
>> @@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct
>> drm_sched_entity *entity)
>>   {
>>       rmb();
>>   -    if (!entity->rq || spsc_queue_peek(&entity->job_queue)
== NULL)
>> +    if (list_empty(&entity->list) ||
>> +        spsc_queue_peek(&entity->job_queue) == NULL)
>>           return true;
>>         return false;
>> @@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct
>> drm_sched_entity *entity, long timeout)
>>       long ret = timeout;
>>         sched = entity->rq->sched;
>> -    if (!drm_sched_entity_is_initialized(sched, entity))
>> -        return ret;
>>       /**
>>        * The client will not queue more IBs during this fini,
consume
>> existing
>>        * queued IBs or discard them on SIGKILL
>> @@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct
>> drm_sched_entity *entity, long timeout)
>>       last_user = cmpxchg(&entity->last_user,
current->group_leader,
>> NULL);
>>       if ((!last_user || last_user == current->group_leader) &&
>>           (current->flags & PF_EXITING) && (current->exit_code ==
>> SIGKILL))
>> -        drm_sched_entity_set_rq(entity, NULL);
>> +        drm_sched_rq_remove_entity(entity->rq, entity);
>>         return ret;
>>   }
>> @@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct
>> drm_sched_entity *entity)
>>       struct drm_gpu_scheduler *sched;
>>         sched = entity->rq->sched;
>> -    drm_sched_entity_set_rq(entity, NULL);
>> +    drm_sched_rq_remove_entity(entity->rq, entity);
>>         /* Consumption of existing IBs wasn't completed. Forcefully
>>        * remove them here.
>> @@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct
>> drm_sched_entity *entity,
>>       if (entity->rq == rq)
>>           return;
>>   -    spin_lock(&entity->rq_lock);
>> -
>> -    if (entity->rq)
>> -        drm_sched_rq_remove_entity(entity->rq, entity);
>> +    BUG_ON(!rq);
>>   +    spin_lock(&entity->rq_lock);
>> +    drm_sched_rq_remove_entity(entity->rq, entity);
>>       entity->rq = rq;
>> -    if (rq)
>> -        drm_sched_rq_add_entity(rq, entity);
>> -
>> +    drm_sched_rq_add_entity(rq, entity);
>>       spin_unlock(&entity->rq_lock);
>>   }
>>   EXPORT_SYMBOL(drm_sched_entity_set_rq);
>

On 08/03/2018 10:06 AM, Christian König wrote:

Am 02.08.2018 um 16:11 schrieb Andrey Grodzovsky:

...

On 08/02/2018 02:47 AM, Nayan Deshmukh wrote:

...

On Thu, Aug 2, 2018 at 12:12 PM Christian König <ckoenig.leichtzumerken@gmail.com mailto:ckoenig.leichtzumerken@gmail.com> wrote:

Am 02.08.2018 um 00:25 schrieb Andrey Grodzovsky:
> Thinking again about this change and 53d5f1e drm/scheduler:
move idle
> entities to scheduler with less load v2
>
> Looks to me like use case for which fc9a539 drm/scheduler: add
NULL
> pointer check for run queue (v2) was done
>
> will not work anymore.
>
> First of all in drm_sched_entity_push_job, 'if (!entity->rq)'
will
> never be true any more since we stopped entity->rq to NULL in
>
> drm_sched_entity_flush.

Good point, going to remove that.

So basically we don't need that if (...){ return; } in drm_sched_entity_push_job any more ?

Yes, exactly.

...

...

>
> Second of all, even after we removed the entity from rq in
> drm_sched_entity_flush to terminate any subsequent submissions
>
> to the queue the other thread doing push job can just acquire
again a
> run queue
>
> from drm_sched_entity_get_free_sched and continue submission

Hi Christian

That is actually desired.

When another process is now using the entity to submit jobs
adding it
back to the rq is actually the right thing to do cause the
entity is
still in use.

Yes, no problem if it's another process. But what about another thread from same process ? Is it a possible use case that 2 threads from same process submit to same entity concurrently ? If so and we specifically kill one, the other will not stop event if we want him to because current code makes him just require a rq for him self.

Well you can't kill a single thread of a process (you can only interrupt it), a SIGKILL will always kill the whole process.

Is the following scenario possible and acceptable ? 2 threads from same process working on same queue where thread A currently in drm_sched_entity_flush->wait_event_timeout (the process getting shut down because of SIGKILL sent by user) while thread B still inside drm_sched_entity_push_job before 'if (reschedule)'. 'A' stopped waiting because queue became empty and then removes the entity queue from scheduler's run queue while B goes inside 'reschedule' because it evaluates to true ('first' is true and all the rest of the conditions), acquires new rq, and later adds it back to scheduler (different one maybe) and keeps submitting jobs as much as he likes and then can be stack for up to 'timeout' time  in his drm_sched_entity_flush waiting for them.

My understanding was that introduction of entity->last is to force immediate termination job submissions by any thread from the terminating process.

Andrey

...

...

I am not aware of the usecase so I might just be rambling. but if the thread/process that created the entity has called drm_sched_entity_flush then we shouldn't allow other processes to push jobs to that entity.

Nayan

Christian.

We don't really know who created the entity, the creation could be by X itself and then it's passed to other process for use. Check 'drm/scheduler: only kill entity if last user is killed v2', the idea is that if by the time you want to kill this entity another process (not another thread from your process - i.e. current->group_leader != last_user in drm_sched_entity_flush) already started to use this entity just let it be.

Yes, exactly that's the idea.

Christian.

...

Andrey

...

> so you can't substitute ' if (!entity->rq)' to 'if
> (list_empty(&entity->list))'.
>
> What about adding a 'stopped' flag to drm_sched_entity to be
set in
> drm_sched_entity_flush and in
>
> drm_sched_entity_push_job check for  'if (!entity->stopped)'
instead
> of  ' if (!entity->rq)' ?
>
> Andrey
>
>
> On 07/30/2018 07:03 AM, Christian König wrote:
>> We removed the redundancy of having an extra scheduler field,
so we
>> can't set the rq to NULL any more or otherwise won't know which
>> scheduler to use for the cleanup.
>>
>> Just remove the entity from the scheduling list instead.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com
<mailto:christian.koenig@amd.com>>
>> ---
>>   drivers/gpu/drm/scheduler/gpu_scheduler.c | 35
>> +++++++------------------------
>>   1 file changed, 8 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index f563e4fbb4b6..1b733229201e 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct
>> drm_sched_entity *entity,
>>   }
>>   EXPORT_SYMBOL(drm_sched_entity_init);
>>   -/**
>> - * drm_sched_entity_is_initialized - Query if entity is
initialized
>> - *
>> - * @sched: Pointer to scheduler instance
>> - * @entity: The pointer to a valid scheduler entity
>> - *
>> - * return true if entity is initialized, false otherwise
>> -*/
>> -static bool drm_sched_entity_is_initialized(struct
drm_gpu_scheduler
>> *sched,
>> -                        struct drm_sched_entity *entity)
>> -{
>> -    return entity->rq != NULL &&
>> -        entity->rq->sched == sched;
>> -}
>> -
>>   /**
>>    * drm_sched_entity_is_idle - Check if entity is idle
>>    *
>> @@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct
>> drm_sched_entity *entity)
>>   {
>>       rmb();
>>   -    if (!entity->rq || spsc_queue_peek(&entity->job_queue)
== NULL)
>> +    if (list_empty(&entity->list) ||
>> + spsc_queue_peek(&entity->job_queue) == NULL)
>>           return true;
>>         return false;
>> @@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct
>> drm_sched_entity *entity, long timeout)
>>       long ret = timeout;
>>         sched = entity->rq->sched;
>> -    if (!drm_sched_entity_is_initialized(sched, entity))
>> -        return ret;
>>       /**
>>        * The client will not queue more IBs during this fini,
consume
>> existing
>>        * queued IBs or discard them on SIGKILL
>> @@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct
>> drm_sched_entity *entity, long timeout)
>>       last_user = cmpxchg(&entity->last_user,
current->group_leader,
>> NULL);
>>       if ((!last_user || last_user == current->group_leader) &&
>>           (current->flags & PF_EXITING) &&
(current->exit_code ==
>> SIGKILL))
>> -        drm_sched_entity_set_rq(entity, NULL);
>> + drm_sched_rq_remove_entity(entity->rq, entity);
>>         return ret;
>>   }
>> @@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct
>> drm_sched_entity *entity)
>>       struct drm_gpu_scheduler *sched;
>>         sched = entity->rq->sched;
>> -    drm_sched_entity_set_rq(entity, NULL);
>> +    drm_sched_rq_remove_entity(entity->rq, entity);
>>         /* Consumption of existing IBs wasn't completed.
Forcefully
>>        * remove them here.
>> @@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct
>> drm_sched_entity *entity,
>>       if (entity->rq == rq)
>>           return;
>>   -    spin_lock(&entity->rq_lock);
>> -
>> -    if (entity->rq)
>> - drm_sched_rq_remove_entity(entity->rq, entity);
>> +    BUG_ON(!rq);
>>   +    spin_lock(&entity->rq_lock);
>> +    drm_sched_rq_remove_entity(entity->rq, entity);
>>       entity->rq = rq;
>> -    if (rq)
>> -        drm_sched_rq_add_entity(rq, entity);
>> -
>> +    drm_sched_rq_add_entity(rq, entity);
>>       spin_unlock(&entity->rq_lock);
>>   }
>>   EXPORT_SYMBOL(drm_sched_entity_set_rq);
>

Hi Andrey,

Good catch. I guess since both Christian and I were working on it parallelly we didn't catch this problem.

If it is only causing a problem in push_job then we can handle it something like this:

---------------------------------------------------------------------------------------------------------------------------- diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index 05dc6ecd4003..f344ce32f128 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -563,6 +563,11 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job, first = spsc_queue_count(&entity->job_queue) == 0; reschedule = idle && first && (entity->num_rq_list > 1);

+ if (first && list_empty(&entity->list)) { + DRM_ERROR("Trying to push to a killed entity\n"); + return; + } + if (reschedule) { rq = drm_sched_entity_get_free_sched(entity); spin_lock(&entity->rq_lock); @@ -580,11 +585,6 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job, if (first) { /* Add the entity to the run queue */ spin_lock(&entity->rq_lock); - if (!entity->rq) { - DRM_ERROR("Trying to push to a killed entity\n"); - spin_unlock(&entity->rq_lock); - return; - } drm_sched_rq_add_entity(entity->rq, entity); spin_unlock(&entity->rq_lock); drm_sched_wakeup(entity->rq->sched); -----------------------------------------------------------------------------------------------------------------------------

On Thu, Aug 2, 2018 at 3:55 AM Andrey Grodzovsky Andrey.Grodzovsky@amd.com wrote:

Thinking again about this change and 53d5f1e drm/scheduler: move idle entities to scheduler with less load v2

Looks to me like use case for which fc9a539 drm/scheduler: add NULL pointer check for run queue (v2) was done

will not work anymore.

First of all in drm_sched_entity_push_job, 'if (!entity->rq)' will never be true any more since we stopped entity->rq to NULL in

drm_sched_entity_flush.

Second of all, even after we removed the entity from rq in drm_sched_entity_flush to terminate any subsequent submissions

to the queue the other thread doing push job can just acquire again a run queue

from drm_sched_entity_get_free_sched and continue submission so you can't substitute ' if (!entity->rq)' to 'if (list_empty(&entity->list))'.

What about adding a 'stopped' flag to drm_sched_entity to be set in drm_sched_entity_flush and in

But it might be worth adding a stopped flag field if it is required at

other places as well.

Thanks, Nayan

drm_sched_entity_push_job check for 'if (!entity->stopped)' instead of ' if (!entity->rq)' ?

Andrey

On 07/30/2018 07:03 AM, Christian König wrote:

...

We removed the redundancy of having an extra scheduler field, so we can't set the rq to NULL any more or otherwise won't know which scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/scheduler/gpu_scheduler.c | 35

+++++++------------------------

...

1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c

b/drivers/gpu/drm/scheduler/gpu_scheduler.c

...

index f563e4fbb4b6..1b733229201e 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct drm_sched_entity

*entity,

...

} EXPORT_SYMBOL(drm_sched_entity_init);

-/**

• • drm_sched_entity_is_initialized - Query if entity is initialized
• • @sched: Pointer to scheduler instance
• • @entity: The pointer to a valid scheduler entity
• • return true if entity is initialized, false otherwise

-*/ -static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler

*sched,

...

•                                     struct drm_sched_entity
  

*entity)

...

-{

• return entity->rq != NULL &&
  

•         entity->rq->sched == sched;
  

-}

• /**
  • drm_sched_entity_is_idle - Check if entity is idle

@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct

drm_sched_entity *entity)

...

{ rmb();

• if (!entity->rq || spsc_queue_peek(&entity->job_queue) == NULL)
  

• if (list_empty(&entity->list) ||
  

•     spsc_queue_peek(&entity->job_queue) == NULL)
          return true;
  
  return false;
  

@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct drm_sched_entity

*entity, long timeout)

...

  long ret = timeout;

  sched = entity->rq->sched;

• if (!drm_sched_entity_is_initialized(sched, entity))
  

•         return ret;
  /**
   * The client will not queue more IBs during this fini, consume
  

existing

...

   * queued IBs or discard them on SIGKILL

@@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct drm_sched_entity

*entity, long timeout)

...

  last_user = cmpxchg(&entity->last_user, current->group_leader,

NULL);

...

  if ((!last_user || last_user == current->group_leader) &&
      (current->flags & PF_EXITING) && (current->exit_code ==

SIGKILL))

...

•         drm_sched_entity_set_rq(entity, NULL);
  

•         drm_sched_rq_remove_entity(entity->rq, entity);
  
  return ret;
  

  }

@@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct drm_sched_entity

*entity)

...

  struct drm_gpu_scheduler *sched;

  sched = entity->rq->sched;

• drm_sched_entity_set_rq(entity, NULL);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  
  /* Consumption of existing IBs wasn't completed. Forcefully
   * remove them here.
  

@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct

drm_sched_entity *entity,

...

  if (entity->rq == rq)
          return;

• spin_lock(&entity->rq_lock);
  

• if (entity->rq)
  

•         drm_sched_rq_remove_entity(entity->rq, entity);
  

• BUG_ON(!rq);
  
  

• spin_lock(&entity->rq_lock);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  entity->rq = rq;
  

• if (rq)
  

•         drm_sched_rq_add_entity(rq, entity);
  

• drm_sched_rq_add_entity(rq, entity);
  spin_unlock(&entity->rq_lock);
  

  } EXPORT_SYMBOL(drm_sched_entity_set_rq);

On Thu, Aug 2, 2018, 8:09 PM Andrey Grodzovsky Andrey.Grodzovsky@amd.com wrote:

On 08/02/2018 02:43 AM, Nayan Deshmukh wrote:

Hi Andrey,

Good catch. I guess since both Christian and I were working on it parallelly we didn't catch this problem.

If it is only causing a problem in push_job then we can handle it something like this:

---

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index 05dc6ecd4003..f344ce32f128 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -563,6 +563,11 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job, first = spsc_queue_count(&entity->job_queue) == 0; reschedule = idle && first && (entity->num_rq_list > 1);

•   if (first && list_empty(&entity->list)) {
  
  
  

first might be false if the other process interrupted by SIGKILL in middle of wait_event_killable in drm_sched_entity_flush when there were still item in queue. I don't see a good way besides adding a 'stopped' flag to drm_sched_enitity.

Sorry I missed this mail. This case might happen but this was also not being handled previously.

Nayan

Andrey

•           DRM_ERROR("Trying to push to a killed entity\n");
  

•           return;
  

•   }
  

•   if (reschedule) {
            rq = drm_sched_entity_get_free_sched(entity);
            spin_lock(&entity->rq_lock);
  

@@ -580,11 +585,6 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job, if (first) { /* Add the entity to the run queue */ spin_lock(&entity->rq_lock);

•           if (!entity->rq) {
  

•                   DRM_ERROR("Trying to push to a killed entity\n");
  

•                   spin_unlock(&entity->rq_lock);
  

•                   return;
  

•           }
            drm_sched_rq_add_entity(entity->rq, entity);
            spin_unlock(&entity->rq_lock);
            drm_sched_wakeup(entity->rq->sched);
  
  

---

On Thu, Aug 2, 2018 at 3:55 AM Andrey Grodzovsky < Andrey.Grodzovsky@amd.com> wrote:

...

Thinking again about this change and 53d5f1e drm/scheduler: move idle entities to scheduler with less load v2

Looks to me like use case for which fc9a539 drm/scheduler: add NULL pointer check for run queue (v2) was done

will not work anymore.

First of all in drm_sched_entity_push_job, 'if (!entity->rq)' will never be true any more since we stopped entity->rq to NULL in

drm_sched_entity_flush.

Second of all, even after we removed the entity from rq in drm_sched_entity_flush to terminate any subsequent submissions

to the queue the other thread doing push job can just acquire again a run queue

from drm_sched_entity_get_free_sched and continue submission so you can't substitute ' if (!entity->rq)' to 'if (list_empty(&entity->list))'.

What about adding a 'stopped' flag to drm_sched_entity to be set in drm_sched_entity_flush and in

But it might be worth adding a stopped flag field if it is required at

other places as well.

Thanks, Nayan

...

drm_sched_entity_push_job check for 'if (!entity->stopped)' instead of ' if (!entity->rq)' ?

Andrey

On 07/30/2018 07:03 AM, Christian König wrote:

...

We removed the redundancy of having an extra scheduler field, so we can't set the rq to NULL any more or otherwise won't know which scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/scheduler/gpu_scheduler.c | 35

+++++++------------------------

...

1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c

b/drivers/gpu/drm/scheduler/gpu_scheduler.c

...

index f563e4fbb4b6..1b733229201e 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct drm_sched_entity

*entity,

...

} EXPORT_SYMBOL(drm_sched_entity_init);

-/**

• • drm_sched_entity_is_initialized - Query if entity is initialized
• • @sched: Pointer to scheduler instance
• • @entity: The pointer to a valid scheduler entity
• • return true if entity is initialized, false otherwise

-*/ -static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler

*sched,

...

•                                     struct drm_sched_entity
  

*entity)

...

-{

• return entity->rq != NULL &&
  

•         entity->rq->sched == sched;
  

-}

• /**
  • drm_sched_entity_is_idle - Check if entity is idle

@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct

drm_sched_entity *entity)

...

{ rmb();

• if (!entity->rq || spsc_queue_peek(&entity->job_queue) == NULL)
  

• if (list_empty(&entity->list) ||
  

•     spsc_queue_peek(&entity->job_queue) == NULL)
          return true;
  
  return false;
  

@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct drm_sched_entity

*entity, long timeout)

...

  long ret = timeout;

  sched = entity->rq->sched;

• if (!drm_sched_entity_is_initialized(sched, entity))
  

•         return ret;
  /**
   * The client will not queue more IBs during this fini, consume
  

existing

...

   * queued IBs or discard them on SIGKILL

@@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct drm_sched_entity

*entity, long timeout)

...

  last_user = cmpxchg(&entity->last_user, current->group_leader,

NULL);

...

  if ((!last_user || last_user == current->group_leader) &&
      (current->flags & PF_EXITING) && (current->exit_code ==

SIGKILL))

...

•         drm_sched_entity_set_rq(entity, NULL);
  

•         drm_sched_rq_remove_entity(entity->rq, entity);
  
  return ret;
  

  }

@@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct drm_sched_entity

*entity)

...

  struct drm_gpu_scheduler *sched;

  sched = entity->rq->sched;

• drm_sched_entity_set_rq(entity, NULL);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  
  /* Consumption of existing IBs wasn't completed. Forcefully
   * remove them here.
  

@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct

drm_sched_entity *entity,

...

  if (entity->rq == rq)
          return;

• spin_lock(&entity->rq_lock);
  

• if (entity->rq)
  

•         drm_sched_rq_remove_entity(entity->rq, entity);
  

• BUG_ON(!rq);
  
  

• spin_lock(&entity->rq_lock);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  entity->rq = rq;
  

• if (rq)
  

•         drm_sched_rq_add_entity(rq, entity);
  

• drm_sched_rq_add_entity(rq, entity);
  spin_unlock(&entity->rq_lock);
  

  } EXPORT_SYMBOL(drm_sched_entity_set_rq);

On Fri, Aug 3, 2018 at 7:17 PM Andrey Grodzovsky Andrey.Grodzovsky@amd.com wrote:

On 08/03/2018 08:12 AM, Nayan Deshmukh wrote:

On Thu, Aug 2, 2018, 8:09 PM Andrey Grodzovsky Andrey.Grodzovsky@amd.com wrote:

...

On 08/02/2018 02:43 AM, Nayan Deshmukh wrote:

Hi Andrey,

Good catch. I guess since both Christian and I were working on it parallelly we didn't catch this problem.

If it is only causing a problem in push_job then we can handle it something like this:

---

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index 05dc6ecd4003..f344ce32f128 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -563,6 +563,11 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job, first = spsc_queue_count(&entity->job_queue) == 0; reschedule = idle && first && (entity->num_rq_list > 1);

•   if (first && list_empty(&entity->list)) {
  
  
  

first might be false if the other process interrupted by SIGKILL in middle of wait_event_killable in drm_sched_entity_flush when there were still item in queue. I don't see a good way besides adding a 'stopped' flag to drm_sched_enitity.

Sorry I missed this mail. This case might happen but this was also not being handled previously.

Nayan

The original code before 'drm/scheduler: stop setting rq to NULL' was , because you didn't use the queue's emptiness as a criteria for aborting your next push to queue but rather checking for entity->rq != NULL.

But (entity->rq != NULL) check was inside the body of if (first) so it was also dependent on the queue's emptiness earlier. Well in any case it doesn't matter now as with Christian's patch we can remove the if condition altogether.

Nayan

Andrey

...

Andrey

•           DRM_ERROR("Trying to push to a killed entity\n");
  

•           return;
  

•   }
  

•   if (reschedule) {
            rq = drm_sched_entity_get_free_sched(entity);
            spin_lock(&entity->rq_lock);
  

@@ -580,11 +585,6 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job, if (first) { /* Add the entity to the run queue */ spin_lock(&entity->rq_lock);

•           if (!entity->rq) {
  

•                   DRM_ERROR("Trying to push to a killed entity\n");
  

•                   spin_unlock(&entity->rq_lock);
  

•                   return;
  

•           }
            drm_sched_rq_add_entity(entity->rq, entity);
            spin_unlock(&entity->rq_lock);
            drm_sched_wakeup(entity->rq->sched);
  
  

---

On Thu, Aug 2, 2018 at 3:55 AM Andrey Grodzovsky < Andrey.Grodzovsky@amd.com> wrote:

...

Thinking again about this change and 53d5f1e drm/scheduler: move idle entities to scheduler with less load v2

Looks to me like use case for which fc9a539 drm/scheduler: add NULL pointer check for run queue (v2) was done

will not work anymore.

First of all in drm_sched_entity_push_job, 'if (!entity->rq)' will never be true any more since we stopped entity->rq to NULL in

drm_sched_entity_flush.

Second of all, even after we removed the entity from rq in drm_sched_entity_flush to terminate any subsequent submissions

to the queue the other thread doing push job can just acquire again a run queue

from drm_sched_entity_get_free_sched and continue submission so you can't substitute ' if (!entity->rq)' to 'if (list_empty(&entity->list))'.

What about adding a 'stopped' flag to drm_sched_entity to be set in drm_sched_entity_flush and in

But it might be worth adding a stopped flag field if it is required at

other places as well.

Thanks, Nayan

...

drm_sched_entity_push_job check for 'if (!entity->stopped)' instead of ' if (!entity->rq)' ?

Andrey

On 07/30/2018 07:03 AM, Christian König wrote:

...

We removed the redundancy of having an extra scheduler field, so we can't set the rq to NULL any more or otherwise won't know which scheduler to use for the cleanup.

Just remove the entity from the scheduling list instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/scheduler/gpu_scheduler.c | 35

+++++++------------------------

...

1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c

b/drivers/gpu/drm/scheduler/gpu_scheduler.c

...

index f563e4fbb4b6..1b733229201e 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -198,21 +198,6 @@ int drm_sched_entity_init(struct drm_sched_entity

*entity,

...

} EXPORT_SYMBOL(drm_sched_entity_init);

-/**

• • drm_sched_entity_is_initialized - Query if entity is initialized
• • @sched: Pointer to scheduler instance
• • @entity: The pointer to a valid scheduler entity
• • return true if entity is initialized, false otherwise

-*/ -static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler

*sched,

...

•                                     struct drm_sched_entity
  

*entity)

...

-{

• return entity->rq != NULL &&
  

•         entity->rq->sched == sched;
  

-}

• /**
  • drm_sched_entity_is_idle - Check if entity is idle

@@ -224,7 +209,8 @@ static bool drm_sched_entity_is_idle(struct

drm_sched_entity *entity)

...

{ rmb();

• if (!entity->rq || spsc_queue_peek(&entity->job_queue) == NULL)
  

• if (list_empty(&entity->list) ||
  

•     spsc_queue_peek(&entity->job_queue) == NULL)
          return true;
  
  return false;
  

@@ -279,8 +265,6 @@ long drm_sched_entity_flush(struct

drm_sched_entity *entity, long timeout)

...

  long ret = timeout;

  sched = entity->rq->sched;

• if (!drm_sched_entity_is_initialized(sched, entity))
  

•         return ret;
  /**
   * The client will not queue more IBs during this fini, consume
  

existing

...

   * queued IBs or discard them on SIGKILL

@@ -299,7 +283,7 @@ long drm_sched_entity_flush(struct

drm_sched_entity *entity, long timeout)

...

  last_user = cmpxchg(&entity->last_user, current->group_leader,

NULL);

...

  if ((!last_user || last_user == current->group_leader) &&
      (current->flags & PF_EXITING) && (current->exit_code ==

SIGKILL))

...

•         drm_sched_entity_set_rq(entity, NULL);
  

•         drm_sched_rq_remove_entity(entity->rq, entity);
  
  return ret;
  

  }

@@ -320,7 +304,7 @@ void drm_sched_entity_fini(struct drm_sched_entity

*entity)

...

  struct drm_gpu_scheduler *sched;

  sched = entity->rq->sched;

• drm_sched_entity_set_rq(entity, NULL);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  
  /* Consumption of existing IBs wasn't completed. Forcefully
   * remove them here.
  

@@ -416,15 +400,12 @@ void drm_sched_entity_set_rq(struct

drm_sched_entity *entity,

...

  if (entity->rq == rq)
          return;

• spin_lock(&entity->rq_lock);
  

• if (entity->rq)
  

•         drm_sched_rq_remove_entity(entity->rq, entity);
  

• BUG_ON(!rq);
  
  

• spin_lock(&entity->rq_lock);
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  entity->rq = rq;
  

• if (rq)
  

•         drm_sched_rq_add_entity(rq, entity);
  

• drm_sched_rq_add_entity(rq, entity);
  spin_unlock(&entity->rq_lock);
  

  } EXPORT_SYMBOL(drm_sched_entity_set_rq);

I believe that in this case

if (!entity->rq) {

DRM_ERROR...

return;

}

clause will take place.

P.S I remember we planned to actually propagate the error back to the caller so i guess we should take care of this sooner or later.

The change is Reviewed-by: Andrey Grodzovsky andrey.grodzovsky@amd.com

Andrey

On 07/30/2018 09:34 AM, Nayan Deshmukh wrote:

Hi Christian,

The code looks good to me. But I was just wondering what will happen when the last user is killed and some other user tries to push to the entity.

Regards, Nayan Deshmukh

On Mon, Jul 30, 2018 at 4:33 PM Christian König <ckoenig.leichtzumerken@gmail.com mailto:ckoenig.leichtzumerken@gmail.com> wrote:

Note which task is using the entity and only kill it if the last
user of
the entity is killed. This should prevent problems when entities
are leaked to
child processes.

v2: add missing kernel doc

Signed-off-by: Christian König <christian.koenig@amd.com
<mailto:christian.koenig@amd.com>>
---
 drivers/gpu/drm/scheduler/gpu_scheduler.c | 6 +++++-
 include/drm/gpu_scheduler.h               | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
b/drivers/gpu/drm/scheduler/gpu_scheduler.c
index 3f2fc5e8242a..f563e4fbb4b6 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
@@ -275,6 +275,7 @@ static void
drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
 long drm_sched_entity_flush(struct drm_sched_entity *entity, long
timeout)
 {
        struct drm_gpu_scheduler *sched;
+       struct task_struct *last_user;
        long ret = timeout;

        sched = entity->rq->sched;
@@ -295,7 +296,9 @@ long drm_sched_entity_flush(struct
drm_sched_entity *entity, long timeout)


        /* For killed process disable any more IBs enqueue right
now */
-       if ((current->flags & PF_EXITING) && (current->exit_code
== SIGKILL))
+       last_user = cmpxchg(&entity->last_user,
current->group_leader, NULL);
+       if ((!last_user || last_user == current->group_leader) &&
+           (current->flags & PF_EXITING) && (current->exit_code
== SIGKILL))
                drm_sched_entity_set_rq(entity, NULL);

        return ret;
@@ -541,6 +544,7 @@ void drm_sched_entity_push_job(struct
drm_sched_job *sched_job,

        trace_drm_sched_job(sched_job, entity);

+       WRITE_ONCE(entity->last_user, current->group_leader);
        first = spsc_queue_push(&entity->job_queue,
&sched_job->queue_node);

        /* first job wakes up scheduler */
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 091b9afcd184..21c648b0b2a1 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -66,6 +66,7 @@ enum drm_sched_priority {
  * @guilty: points to ctx's guilty.
  * @fini_status: contains the exit status in case the process was
signalled.
  * @last_scheduled: points to the finished fence of the last
scheduled job.
+ * @last_user: last group leader pushing a job into the entity.
  *
  * Entities will emit jobs in order to their corresponding hardware
  * ring, and the scheduler will alternate between entities based on
@@ -85,6 +86,7 @@ struct drm_sched_entity {
        struct dma_fence_cb             cb;
        atomic_t                        *guilty;
        struct dma_fence                *last_scheduled;
+       struct task_struct              *last_user;
 };

 /**
-- 
2.14.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel