[6/8] drm/sched: Re-order struct drm_sched_rq members for clarity

Commit Message

Tvrtko Ursulin Sept. 9, 2024, 5:19 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Lets re-order the members to make it clear which are protected by the lock
and at the same time document it via kerneldoc.

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: Christian König <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: Luben Tuikov <[e-mail]>
Cc: Matthew Brost <[e-mail]>
Cc: Philipp Stanner <[e-mail]>
---
 include/drm/gpu_scheduler.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Christian König Sept. 10, 2024, 8:42 a.m. UTC | #1

Am 09.09.24 um 19:19 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <[e-mail]>
>
> Lets re-order the members to make it clear which are protected by the lock
> and at the same time document it via kerneldoc.
>
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Luben Tuikov <[e-mail]>
> Cc: Matthew Brost <[e-mail]>
> Cc: Philipp Stanner <[e-mail]>
> ---
>   include/drm/gpu_scheduler.h | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index a06753987d93..d4a3ba333568 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -243,10 +243,10 @@ struct drm_sched_entity {
>   /**
>    * struct drm_sched_rq - queue of entities to be scheduled.
>    *
> - * @lock: to modify the entities list.
>    * @sched: the scheduler to which this rq belongs to.
> - * @entities: list of the entities to be scheduled.
> + * @lock: protects the list, tree and current entity.
>    * @current_entity: the entity which is to be scheduled.
> + * @entities: list of the entities to be scheduled.
>    * @rb_tree_root: root of time based priory queue of entities for FIFO scheduling
>    *
>    * Run queue is a set of entities scheduling command submissions for
> @@ -254,10 +254,12 @@ struct drm_sched_entity {
>    * the next entity to emit commands from.
>    */
>   struct drm_sched_rq {
> -	spinlock_t			lock;
>   	struct drm_gpu_scheduler	*sched;
> -	struct list_head		entities;
> +
> +	spinlock_t			lock;
> +	/* Following members are protected by the @lock: */
>   	struct drm_sched_entity		*current_entity;
> +	struct list_head		entities;
>   	struct rb_root_cached		rb_tree_root;

Just suggestion for further improvement:

struct {
     struct drm_sched_entity    *last_selected;
     struct list_head                  entities;
} rr;
struct {
     struct rb_root_cached        rb_tree_root;
} fifo;

But even without that the patch is Reviewed-by: Christian König 
<[e-mail]>

Regards,
Christian.

>   };
>

Philipp Stanner Sept. 10, 2024, 10:05 a.m. UTC | #2

On Mon, 2024-09-09 at 18:19 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[e-mail]>
> 
> Lets re-order the members to make it clear which are protected by the
> lock
> and at the same time document it via kerneldoc.

I'd prefer if commit messages follow the idiomatic kernel style of that
order:
   1. Describe the current situation
   2. State why it's bad or undesirable
   3. (describe the solution)
   4. Conclude commit message through sentences in imperative stating
      what the commit does.

In this case I would go for:
"struct drm_sched_rq contains a spinlock that protects several struct
members. The current documentation incorrectly states that this lock
only guards the entities list. In truth, it guards that list, the
rb_tree and the current entity.

Document what the lock actually guards. Rearrange struct members so
that this becomes even more visible."

> 
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Cc: Christian König <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Luben Tuikov <[e-mail]>
> Cc: Matthew Brost <[e-mail]>
> Cc: Philipp Stanner <[e-mail]>
> ---
>  include/drm/gpu_scheduler.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/gpu_scheduler.h
> b/include/drm/gpu_scheduler.h
> index a06753987d93..d4a3ba333568 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -243,10 +243,10 @@ struct drm_sched_entity {
>  /**
>   * struct drm_sched_rq - queue of entities to be scheduled.
>   *
> - * @lock: to modify the entities list.
>   * @sched: the scheduler to which this rq belongs to.
> - * @entities: list of the entities to be scheduled.
> + * @lock: protects the list, tree and current entity.

Would be more consistent with the below comment if you'd address them
with their full name, aka "protects @entities, @rb_tree_root and
@current_entity".


Thanks,
P.


>   * @current_entity: the entity which is to be scheduled.
> + * @entities: list of the entities to be scheduled.
>   * @rb_tree_root: root of time based priory queue of entities for
> FIFO scheduling
>   *
>   * Run queue is a set of entities scheduling command submissions for
> @@ -254,10 +254,12 @@ struct drm_sched_entity {
>   * the next entity to emit commands from.
>   */
>  struct drm_sched_rq {
> - spinlock_t lock;
>   struct drm_gpu_scheduler *sched;
> - struct list_head entities;
> +
> + spinlock_t lock;
> + /* Following members are protected by the @lock: */
>   struct drm_sched_entity *current_entity;
> + struct list_head entities;
>   struct rb_root_cached rb_tree_root;
>  };
>

Tvrtko Ursulin Sept. 10, 2024, 10:42 a.m. UTC | #3

On 10/09/2024 11:05, Philipp Stanner wrote:
> On Mon, 2024-09-09 at 18:19 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> Lets re-order the members to make it clear which are protected by the
>> lock
>> and at the same time document it via kerneldoc.
> 
> I'd prefer if commit messages follow the idiomatic kernel style of that
> order:
>     1. Describe the current situation
>     2. State why it's bad or undesirable
>     3. (describe the solution)
>     4. Conclude commit message through sentences in imperative stating
>        what the commit does.
> 
> In this case I would go for:
> "struct drm_sched_rq contains a spinlock that protects several struct
> members. The current documentation incorrectly states that this lock
> only guards the entities list. In truth, it guards that list, the
> rb_tree and the current entity.
> 
> Document what the lock actually guards. Rearrange struct members so
> that this becomes even more visible."

IMO a bit much to ask for a text book format, for a trivial patch, when 
all points are already implicitly obvious. That is "lets make it clear" 
= current situation is not clear -> obviously bad with no need to 
explain; "and the same time document" = means it is currently not 
documented -> again obviously not desirable.

But okay, since I agree with the point below (*), I can explode the text 
for maximum redundancy.

>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>> Cc: Christian König <[e-mail]>
>> Cc: Alex Deucher <[e-mail]>
>> Cc: Luben Tuikov <[e-mail]>
>> Cc: Matthew Brost <[e-mail]>
>> Cc: Philipp Stanner <[e-mail]>
>> ---
>>   include/drm/gpu_scheduler.h | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/drm/gpu_scheduler.h
>> b/include/drm/gpu_scheduler.h
>> index a06753987d93..d4a3ba333568 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -243,10 +243,10 @@ struct drm_sched_entity {
>>   /**
>>    * struct drm_sched_rq - queue of entities to be scheduled.
>>    *
>> - * @lock: to modify the entities list.
>>    * @sched: the scheduler to which this rq belongs to.
>> - * @entities: list of the entities to be scheduled.
>> + * @lock: protects the list, tree and current entity.
> 
> Would be more consistent with the below comment if you'd address them
> with their full name, aka "protects @entities, @rb_tree_root and
> @current_entity".

*) this one I agree with.

Regards,

Tvrtko

> 
> Thanks,
> P.
> 
> 
>>    * @current_entity: the entity which is to be scheduled.
>> + * @entities: list of the entities to be scheduled.
>>    * @rb_tree_root: root of time based priory queue of entities for
>> FIFO scheduling
>>    *
>>    * Run queue is a set of entities scheduling command submissions for
>> @@ -254,10 +254,12 @@ struct drm_sched_entity {
>>    * the next entity to emit commands from.
>>    */
>>   struct drm_sched_rq {
>> - spinlock_t lock;
>>    struct drm_gpu_scheduler *sched;
>> - struct list_head entities;
>> +
>> + spinlock_t lock;
>> + /* Following members are protected by the @lock: */
>>    struct drm_sched_entity *current_entity;
>> + struct list_head entities;
>>    struct rb_root_cached rb_tree_root;
>>   };
>>   
>

Philipp Stanner Sept. 10, 2024, 11:24 a.m. UTC | #4

On Tue, 2024-09-10 at 11:42 +0100, Tvrtko Ursulin wrote:
> 
> On 10/09/2024 11:05, Philipp Stanner wrote:
> > On Mon, 2024-09-09 at 18:19 +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <[e-mail]>
> > > 
> > > Lets re-order the members to make it clear which are protected by
> > > the
> > > lock
> > > and at the same time document it via kerneldoc.
> > 
> > I'd prefer if commit messages follow the idiomatic kernel style of
> > that
> > order:
> >     1. Describe the current situation
> >     2. State why it's bad or undesirable
> >     3. (describe the solution)
> >     4. Conclude commit message through sentences in imperative
> > stating
> >        what the commit does.
> > 
> > In this case I would go for:
> > "struct drm_sched_rq contains a spinlock that protects several
> > struct
> > members. The current documentation incorrectly states that this
> > lock
> > only guards the entities list. In truth, it guards that list, the
> > rb_tree and the current entity.
> > 
> > Document what the lock actually guards. Rearrange struct members so
> > that this becomes even more visible."
> 
> IMO a bit much to ask for a text book format, for a trivial patch,
> when 
> all points are already implicitly obvious. That is "lets make it
> clear" 
> = current situation is not clear -> obviously bad with no need to 
> explain; "and the same time document" = means it is currently not 
> documented -> again obviously not desirable.
> 
> But okay, since I agree with the point below (*), I can explode the
> text 
> for maximum redundancy.

I agree that for very short / trivial changes one can keep it short.
But the line separating what is obvious for oneself and for others is
often thin.

P.

> 
> > > Signed-off-by: Tvrtko Ursulin <[e-mail]>
> > > Cc: Christian König <[e-mail]>
> > > Cc: Alex Deucher <[e-mail]>
> > > Cc: Luben Tuikov <[e-mail]>
> > > Cc: Matthew Brost <[e-mail]>
> > > Cc: Philipp Stanner <[e-mail]>
> > > ---
> > >   include/drm/gpu_scheduler.h | 10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/drm/gpu_scheduler.h
> > > b/include/drm/gpu_scheduler.h
> > > index a06753987d93..d4a3ba333568 100644
> > > --- a/include/drm/gpu_scheduler.h
> > > +++ b/include/drm/gpu_scheduler.h
> > > @@ -243,10 +243,10 @@ struct drm_sched_entity {
> > >   /**
> > >    * struct drm_sched_rq - queue of entities to be scheduled.
> > >    *
> > > - * @lock: to modify the entities list.
> > >    * @sched: the scheduler to which this rq belongs to.
> > > - * @entities: list of the entities to be scheduled.
> > > + * @lock: protects the list, tree and current entity.
> > 
> > Would be more consistent with the below comment if you'd address
> > them
> > with their full name, aka "protects @entities, @rb_tree_root and
> > @current_entity".
> 
> *) this one I agree with.
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Thanks,
> > P.
> > 
> > 
> > >    * @current_entity: the entity which is to be scheduled.
> > > + * @entities: list of the entities to be scheduled.
> > >    * @rb_tree_root: root of time based priory queue of entities
> > > for
> > > FIFO scheduling
> > >    *
> > >    * Run queue is a set of entities scheduling command
> > > submissions for
> > > @@ -254,10 +254,12 @@ struct drm_sched_entity {
> > >    * the next entity to emit commands from.
> > >    */
> > >   struct drm_sched_rq {
> > > - spinlock_t lock;
> > >    struct drm_gpu_scheduler *sched;
> > > - struct list_head entities;
> > > +
> > > + spinlock_t lock;
> > > + /* Following members are protected by the @lock: */
> > >    struct drm_sched_entity *current_entity;
> > > + struct list_head entities;
> > >    struct rb_root_cached rb_tree_root;
> > >   };
> > >   
> > 
>

Patch

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index a06753987d93..d4a3ba333568 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -243,10 +243,10 @@  struct drm_sched_entity {
 /**
  * struct drm_sched_rq - queue of entities to be scheduled.
  *
- * @lock: to modify the entities list.
  * @sched: the scheduler to which this rq belongs to.
- * @entities: list of the entities to be scheduled.
+ * @lock: protects the list, tree and current entity.
  * @current_entity: the entity which is to be scheduled.
+ * @entities: list of the entities to be scheduled.
  * @rb_tree_root: root of time based priory queue of entities for FIFO scheduling
  *
  * Run queue is a set of entities scheduling command submissions for
@@ -254,10 +254,12 @@  struct drm_sched_entity {
  * the next entity to emit commands from.
  */
 struct drm_sched_rq {
-	spinlock_t			lock;
 	struct drm_gpu_scheduler	*sched;
-	struct list_head		entities;
+
+	spinlock_t			lock;
+	/* Following members are protected by the @lock: */
 	struct drm_sched_entity		*current_entity;
+	struct list_head		entities;
 	struct rb_root_cached		rb_tree_root;
 };