[PATCH 1/5] drm/sched: Optimise drm_sched_entity_push_job

[Tvrtko Ursulin]

On 15/10/2024 08:11, Philipp Stanner wrote:
> On Mon, 2024-10-14 at 13:07 +0100, Tvrtko Ursulin wrote:
>>
>> On 14/10/2024 12:32, Philipp Stanner wrote:
>>> Hi,
>>>
>>> On Mon, 2024-10-14 at 11:46 +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>
>>>> In FIFO mode We can avoid dropping the lock only to immediately
>>>> re-
>>>> acquire
>>>> by adding a new drm_sched_rq_update_fifo_locked() helper.
>>>>
>>>
>>> Please write detailed commit messages, as described here [1].
>>>      1. Describe the problem: current state and why it's bad.
>>>      2. Then, describe in imperative (present tense) form what the
>>> commit
>>>         does about the problem.
>>
>> Both pieces of info are already there:
>>
>> 1. Drops the lock to immediately re-acquire it.
>> 2. We avoid that by by adding a locked helper.
>>> Optionally, in between can be information about why it's solved
>>> this
>>> way and not another etc.
>>>
>>> Applies to the other patches, too.
>>>
>>>
>>> [1]
>>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>>
>> Thanks I am new here and did not know this.
>>
>> Seriosuly, lets not be too blindly strict about this because it can
>> get
>> IMO ridiculous.
>>
>> One example when I previously accomodated your request is patch 3/5
>> from
>> this series:
>>
>> """
>> Current kerneldoc for struct drm_sched_rq incompletely documents what
>> fields are protected by the lock.
>>
>> This is not good because it is misleading.
>>
>> Lets fix it by listing all the elements which are protected by the
>> lock.
>> """
>>
>> While this was the original commit text you weren't happy with:
>>
>> """
>> drm/sched: Re-order struct drm_sched_rq members for clarity
>>
>> 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 maintain the original text was passable.
>>
>> On top, this was just a respin to accomodate the merge process. All
>> approvals were done and dusted couple weeks or so ago so asking for
>> yet
>> another respin for such trivial objections is not great.
> 
> I understand that you're unhappy, but please understand the position
> I'm coming from. As you know, since you sent these patches within a
> different series (and, thus, since I reviewed them), I was trusted with
> co-maintaining this piece of shared infrastructure.
> 
> And since you've worked on it a bit now, I suppose you also know that
> the GPU Scheduler is arguably in quite a bad shape, has far too little
> documentation, has leaks, maybe race conditions, parts *where the
> locking rules are unclear* and is probably only fully understood by a
> small hand full of people. I also argue that this is a *very*
> complicated piece of software.

We already went over that and agreed. Not least I agreed the base is 
shaky since few years  ago. :)

Btw if things align, I hope you will at some point see a follow up 
series from me which makes some significant simplifications and 
improvements at the same time.
> So I might be or appear to be a bit pedantic, but I'm not doing that to
> terrorize you, but because I want this thing to become well documented,
> understandable, and bisectable. Working towards a canonical, idiot-
> proof commit style is one measure that will help with that.
> 
> I want to offer you the following: I can be more relaxed with things
> universally recognized as trivial (comment changes, struct member
> reordering) – but when something like a lock is touched in any way, we
> shall document that in the commit message as canonically as possible,
> so someone who's less experienced and just bisected the commit
> immediately understands what has been done (or rather: was supposed to
> be done).

So how would you suggest to expand this commit text so it doesn't read 
too self-repeating?

Regards,

Tvrtko