From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9E5ADC3ABDA for ; Wed, 14 May 2025 08:46:10 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 41FCD10E218; Wed, 14 May 2025 08:46:10 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=igalia.com header.i=@igalia.com header.b="XZ94Gkbn"; dkim-atps=neutral Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) by gabe.freedesktop.org (Postfix) with ESMTPS id 995B210E148; Wed, 14 May 2025 08:46:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=aiGJMJ+H0PhVMrW5qhCGY7ZHfFuvBjMjJikZvkA7Mr0=; b=XZ94Gkbn3GC3u+7RWk83k5SrlL OQY7YXGnVOQ9wip8SPqnEGuxHX8dybibmeu+aO4K7FOoLcmnXuaJtTeo+IeRxy0M1QDCiSsF2KCGl 0sPNQ2imqdU7vf5o8K00g3uWmkCWjVHw+qjtUfurEquaEWF3tmeiUw4HgU1l+QgntxlZXDAuXXdan YisSXYvoY4P+k8OMXOZCVABSpOrHsKzTsfXO2DKtXQmMLUR5fQsRx4dAHNyk+qB2A4egM4Ahq+wz1 Y+gY06Z4h7DA+hePDJMTD2oeFCNF4Ps1EsPCgQg6TmvJLJeCCxi6idY8kSfz+cHL3k9cB1h+DWh7t njq/pbGQ==; Received: from [81.79.92.254] (helo=[192.168.0.101]) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_128_GCM:128) (Exim) id 1uF7f3-0083aw-PE; Wed, 14 May 2025 10:46:04 +0200 Message-ID: <2de0c446-e521-408c-a0e2-1c1b1d85f830@igalia.com> Date: Wed, 14 May 2025 09:46:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC v4 04/16] drm/sched: Avoid double re-lock on the job free path To: phasta@kernel.org, amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: kernel-dev@igalia.com, =?UTF-8?Q?Christian_K=C3=B6nig?= , Danilo Krummrich , Matthew Brost References: <20250425102034.85133-1-tvrtko.ursulin@igalia.com> <20250425102034.85133-5-tvrtko.ursulin@igalia.com> <657c053d7cd443ff310dfff19d03ab11e0f17289.camel@mailbox.org> Content-Language: en-GB From: Tvrtko Ursulin In-Reply-To: <657c053d7cd443ff310dfff19d03ab11e0f17289.camel@mailbox.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" On 12/05/2025 13:49, Philipp Stanner wrote: > On Fri, 2025-04-25 at 11:20 +0100, Tvrtko Ursulin wrote: >> Currently the job free work item will lock sched->job_list_lock first >> time >> to see if there are any jobs, free a single job, and then lock again >> to >> decide whether to re-queue itself if there are more finished jobs. >> >> Since drm_sched_get_finished_job() already looks at the second job in >> the >> queue we can simply add the signaled check and have it return the >> presence >> of more jobs to free to the caller. That way the work item does not >> have >> to lock the list again and repeat the signaled check. > > Are you convinced that this is worth it? I cannot see a reason for the lazy code which re-locks only to get the same boolean state it already peeked at so yes, I am. Maybe CPU vendors don't mind burning extra cycles to sell us faster chips, I don't know. :D More interesting angle is that the patch removes the potential opportunistic signaling from the fence worker (the bad old evil dma_fence_is_signaled). > I'm torn. It's rare that one returns a status through a boolean by > reference. > > Independently from that, this is a candidate which certainly can be > branched out from this series, to make the series completely about the > new scheduling policy, not general other improvements. If I get an r-b I can easily send it standalone. Until then I let it simmer. Regards, Tvrtko >> Signed-off-by: Tvrtko Ursulin >> Cc: Christian König >> Cc: Danilo Krummrich >> Cc: Matthew Brost >> Cc: Philipp Stanner >> --- >>  drivers/gpu/drm/scheduler/sched_main.c | 39 +++++++++++------------- >> -- >>  1 file changed, 16 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/gpu/drm/scheduler/sched_main.c >> b/drivers/gpu/drm/scheduler/sched_main.c >> index 86e40157b09b..a45b02fd2af3 100644 >> --- a/drivers/gpu/drm/scheduler/sched_main.c >> +++ b/drivers/gpu/drm/scheduler/sched_main.c >> @@ -365,22 +365,6 @@ static void __drm_sched_run_free_queue(struct >> drm_gpu_scheduler *sched) >>   queue_work(sched->submit_wq, &sched->work_free_job); >>  } >> >> -/** >> - * drm_sched_run_free_queue - enqueue free-job work if ready >> - * @sched: scheduler instance >> - */ >> -static void drm_sched_run_free_queue(struct drm_gpu_scheduler >> *sched) >> -{ >> - struct drm_sched_job *job; >> - >> - spin_lock(&sched->job_list_lock); >> - job = list_first_entry_or_null(&sched->pending_list, >> -        struct drm_sched_job, list); >> - if (job && dma_fence_is_signaled(&job->s_fence->finished)) >> - __drm_sched_run_free_queue(sched); >> - spin_unlock(&sched->job_list_lock); >> -} >> - >>  /** >>   * drm_sched_job_done - complete a job >>   * @s_job: pointer to the job which is done >> @@ -1097,12 +1081,13 @@ drm_sched_select_entity(struct >> drm_gpu_scheduler *sched) >>   * drm_sched_get_finished_job - fetch the next finished job to be >> destroyed >>   * >>   * @sched: scheduler instance >> + * @have_more: are there more finished jobs on the list >>   * >>   * Returns the next finished job from the pending list (if there is >> one) >>   * ready for it to be destroyed. >>   */ >>  static struct drm_sched_job * >> -drm_sched_get_finished_job(struct drm_gpu_scheduler *sched) >> +drm_sched_get_finished_job(struct drm_gpu_scheduler *sched, bool >> *have_more) >>  { >>   struct drm_sched_job *job, *next; >> >> @@ -1110,22 +1095,27 @@ drm_sched_get_finished_job(struct >> drm_gpu_scheduler *sched) >> >>   job = list_first_entry_or_null(&sched->pending_list, >>          struct drm_sched_job, list); >> - >>   if (job && dma_fence_is_signaled(&job->s_fence->finished)) { >>   /* remove job from pending_list */ >>   list_del_init(&job->list); >> >>   /* cancel this job's TO timer */ >>   cancel_delayed_work(&sched->work_tdr); >> - /* make the scheduled timestamp more accurate */ >> + >> + *have_more = false; >>   next = list_first_entry_or_null(&sched- >>> pending_list, >>   typeof(*next), >> list); >> - >>   if (next) { >> + /* make the scheduled timestamp more >> accurate */ >>   if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, >>        &next->s_fence- >>> scheduled.flags)) >>   next->s_fence->scheduled.timestamp = >>   dma_fence_timestamp(&job- >>> s_fence->finished); >> + >> + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, >> +      &next->s_fence- >>> finished.flags)) >> + *have_more = true; >> + >>   /* start TO timer for next job */ >>   drm_sched_start_timeout(sched); >>   } >> @@ -1184,12 +1174,15 @@ static void drm_sched_free_job_work(struct >> work_struct *w) >>   struct drm_gpu_scheduler *sched = >>   container_of(w, struct drm_gpu_scheduler, >> work_free_job); >>   struct drm_sched_job *job; >> + bool have_more; >> >> - job = drm_sched_get_finished_job(sched); >> - if (job) >> + job = drm_sched_get_finished_job(sched, &have_more); >> + if (job) { >>   sched->ops->free_job(job); >> + if (have_more) >> + __drm_sched_run_free_queue(sched); >> + } >> >> - drm_sched_run_free_queue(sched); >>   drm_sched_run_job_queue(sched); >>  } >> >