| Message ID | 20240909171937.51550-7-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM scheduler fixes, or not, or incorrect kind | expand |
Am 09.09.24 um 19:19 schrieb Tvrtko Ursulin: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > 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 <tvrtko.ursulin@igalia.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Luben Tuikov <ltuikov89@gmail.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Philipp Stanner <pstanner@redhat.com> > --- > 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 <christian.koenig@amd.com> Regards, Christian. > }; >
On Mon, 2024-09-09 at 18:19 +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > 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 <tvrtko.ursulin@igalia.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Luben Tuikov <ltuikov89@gmail.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Philipp Stanner <pstanner@redhat.com> > --- > 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; > }; >
On 10/09/2024 11:05, Philipp Stanner wrote: > On Mon, 2024-09-09 at 18:19 +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> >> 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 <tvrtko.ursulin@igalia.com> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: Luben Tuikov <ltuikov89@gmail.com> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: Philipp Stanner <pstanner@redhat.com> >> --- >> 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; >> }; >> >
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 <tvrtko.ursulin@igalia.com> > > > > > > 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 <tvrtko.ursulin@igalia.com> > > > Cc: Christian König <christian.koenig@amd.com> > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > > Cc: Luben Tuikov <ltuikov89@gmail.com> > > > Cc: Matthew Brost <matthew.brost@intel.com> > > > Cc: Philipp Stanner <pstanner@redhat.com> > > > --- > > > 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; > > > }; > > > > > >
On Mon, Sep 09, 2024 at 06:19:35PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > 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 <tvrtko.ursulin@igalia.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Luben Tuikov <ltuikov89@gmail.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Philipp Stanner <pstanner@redhat.com> > --- > 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: */ Bit a bikeshed, but I think the moment you feel like you need to sprinkle additional comments around you should switch over to the in-line documentation style, where you can have all the space to add everything. For structures in general I think the top comment style for members isn't a good idea except for really trivial structs. But yeah I know it's a bit of work to shuffle stuff around, so maybe next time .. -Sima > struct drm_sched_entity *current_entity; > + struct list_head entities; > struct rb_root_cached rb_tree_root; > }; > > -- > 2.46.0 >
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; };