On 5/26/25 11:34, Philipp Stanner wrote:
> On Mon, 2025-05-26 at 11:25 +0200, Christian König wrote:
>> On 5/23/25 16:16, Danilo Krummrich wrote:
>>> On Fri, May 23, 2025 at 04:11:39PM +0200, Danilo Krummrich wrote:
>>>> On Fri, May 23, 2025 at 02:56:40PM +0200, Christian König wrote:
>>>>> It turned out that we can actually massively optimize here.
>>>>>
>>>>> The previous code was horrible inefficient since it constantly
>>>>> released
>>>>> and re-acquired the lock of the xarray and started each
>>>>> iteration from the
>>>>> base of the array to avoid concurrent modification which in our
>>>>> case
>>>>> doesn't exist.
>>>>>
>>>>> Additional to that the xas_find() and xas_store() functions are
>>>>> explicitly
>>>>> made in a way so that you can efficiently check entries and if
>>>>> you don't
>>>>> find a match store a new one at the end or replace existing
>>>>> ones.
>>>>>
>>>>> So use xas_for_each()/xa_store() instead of
>>>>> xa_for_each()/xa_alloc().
>>>>> It's a bit more code, but should be much faster in the end.
>>>>
>>>> This commit message does neither explain the motivation of the
>>>> commit nor what it
>>>> does. It describes what instead belongs into the changelog
>>>> between versions.
>>>
>>> Sorry, this is wrong. I got confused, the commit message is
>>> perfectly fine. :)
>>>
>>> The rest still applies though.
>>>
>>>> Speaking of versioning of the patch series, AFAIK there were
>>>> previous versions,
>>>> but this series was sent as a whole new series -- why?
>>>>
>>>> Please resend with a proper commit message, version and
>>>> changelog. Thanks!
>>
>>
>> Well Philip asked to remove the changelog. I'm happy to bring it
>> back, but yeah...
>
> No no no no :D
>
> Philipp asked for the changelog to be removed *from the git commit
> message*; because it doesn't belong / isn't useful there.
>
> If there's a cover letter, the changelog should be in the cover letter.
> If there's no cover letter, it should be between the --- separators:
I can live with that, just clearly state what you want.
For DRM the ask is often to keep the changelog in the commit message or remove
it entirely.
Regards,
Christian.
>
>
> Signed-off-by: Gordon Freeman <free...@blackmesa.org>
> Reviewed-by: Alyx Vance <a...@vance.edu>
> ---
> Changes in v2:
> - Provide more docu for crowbar-alloc-function.
> - Use NULL pointers for reserved xarray entries
> ---
> <DIFF>
>
>
> P.
>
>
>>
>> Regards,
>> Christian.
>>
>>>>
>>>>> Signed-off-by: Christian König <christian.koe...@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/scheduler/sched_main.c | 29
>>>>> ++++++++++++++++++--------
>>>>> 1 file changed, 20 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index f7118497e47a..cf200b1b643e 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -871,10 +871,8 @@ EXPORT_SYMBOL(drm_sched_job_arm);
>>>>> int drm_sched_job_add_dependency(struct drm_sched_job *job,
>>>>> struct dma_fence *fence)
>>>>> {
>>>>> + XA_STATE(xas, &job->dependencies, 0);
>>>>> struct dma_fence *entry;
>>>>> - unsigned long index;
>>>>> - u32 id = 0;
>>>>> - int ret;
>>>>>
>>>>> if (!fence)
>>>>> return 0;
>>>>> @@ -883,24 +881,37 @@ int drm_sched_job_add_dependency(struct
>>>>> drm_sched_job *job,
>>>>> * This lets the size of the array of deps scale with
>>>>> the number of
>>>>> * engines involved, rather than the number of BOs.
>>>>> */
>>>>> - xa_for_each(&job->dependencies, index, entry) {
>>>>> + xa_lock(&job->dependencies);
>>>>> + xas_for_each(&xas, entry, ULONG_MAX) {
>>>>> if (entry->context != fence->context)
>>>>> continue;
>>>>>
>>>>> if (dma_fence_is_later(fence, entry)) {
>>>>> dma_fence_put(entry);
>>>>> - xa_store(&job->dependencies, index,
>>>>> fence, GFP_KERNEL);
>>>>> + xas_store(&xas, fence);
>>>>> } else {
>>>>> dma_fence_put(fence);
>>>>> }
>>>>> - return 0;
>>>>> + xa_unlock(&job->dependencies);
>>>>> + return xas_error(&xas);
>>>>> }
>>>>>
>>>>> - ret = xa_alloc(&job->dependencies, &id, fence,
>>>>> xa_limit_32b, GFP_KERNEL);
>>>>> - if (ret != 0)
>>>>> +retry:
>>>>> + entry = xas_store(&xas, fence);
>>>>> + xa_unlock(&job->dependencies);
>>>>> +
>>>>> + /* There shouldn't be any concurrent add, so no need
>>>>> to loop again */
>>>>
>>>> Concurrency shouldn't matter, xas_nomem() stores the pre-
>>>> allocated memory in the
>>>> XA_STATE not the xarray. Hence, I think we should remove the
>>>> comment.
>>>>
>>>>> + if (xas_nomem(&xas, GFP_KERNEL)) {
>>>>> + xa_lock(&job->dependencies);
>>>>> + goto retry;
>>>>
>>>> Please don't use a goto here, if we would have failed to allocate
>>>> memory here,
>>>> this would be an endless loop until we succeed eventually. It
>>>> would be equal to:
>>>>
>>>> while (!ptr) {
>>>> ptr = kmalloc();
>>>> }
>>>>
>>>> Instead just take the lock and call xas_store() again.
>>>>
>>>>> + }
>>>>> +
>>>>> + if (xas_error(&xas))
>>>>> dma_fence_put(fence);
>>>>> + else
>>>>> + WARN_ON(entry);
>>>>
>>>> Please don't call WARN_ON() here, this isn't fatal, we only need
>>>> to return the
>>>> error code.
>>
>
