From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9BDF22BE057; Mon, 6 Jul 2026 08:46:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.97.179.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783327594; cv=none; b=FMOool29kGGtke7RBdemQkB2ggmSXPnT7qiqFLb0QtLJeP3TWUojMYYtgvrMy+rJjPrj85fU1JMJGvlYNyFqI9BGXARmZI5OhAdT7xQ2b3hrDBXqi/kFTHXs5bluBX9ReWeAifqcZQHwM9p/hS2C89w6hUAW3XMSkF/6iP/Wi4c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783327594; c=relaxed/simple; bh=0J5NWzgozKhj7iWw1lTDLJc1MZi1kjdy9yMrZRufbA0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Uw07pybYJYoPfShY61vCmI9c8k6NoyfiWv2R6vfwg03erN2V1RTWIIumX5ES74egw1ed1WFW/35A0GuEX/q1yWfjHR4Ezq7SoWRfsFfnM+5o73NG3O4lDRGSTOwCi3qq7WwmhT4WjMLA0C54gdeYt7KHhkdbX3uDxlGW5wxO5ck= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=igalia.com; spf=pass smtp.mailfrom=igalia.com; dkim=pass (2048-bit key) header.d=igalia.com header.i=@igalia.com header.b=M+Gs7SKK; arc=none smtp.client-ip=213.97.179.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=igalia.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=igalia.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=igalia.com header.i=@igalia.com header.b="M+Gs7SKK" 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=jknh75JH1P0CBTsivHhJoCFnbVm0gTLli2CnNEauNK8=; b=M+Gs7SKKHMqEtDOlgnQEd1iVQU HQvocK2D4tkQtvgMbMxs0c54ABt4LqrYEXu1b3DKAY98TgBFzwQVrVIzeR19iwhOVYDKSRDDMERzL ONlPhxXwIPOUhA10uBYE7fv2aAF/tXFCAgnene87JfjTVa3EupzKqZnywxNHjyf75mjvQAvma6hkb H+t0FRPFebqyf+9Xi+enJLXYf3WtcnxW8OYUIzlaVuUvUWc0d1WQjVKUXoM8I92u5ZjDtnXmtlj/K LK9O2QkHq3Mv9kAbGiuu3AxK9ai9KrcN8BphTHm4yjaDWAYqCEz1t4EzsE8sKvA06vjJZo/vpB5Gs IyeMqucQ==; Received: from [90.240.106.137] (helo=[192.168.0.116]) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_128_GCM:128) (Exim) id 1wgexk-009e5C-3f; Mon, 06 Jul 2026 10:46:00 +0200 Message-ID: Date: Mon, 6 Jul 2026 09:45:58 +0100 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/5] drm/sched: Protect entity->last_scheduled with spinlock To: phasta@kernel.org, Tvrtko Ursulin , Matthew Brost , Danilo Krummrich , =?UTF-8?Q?Christian_K=C3=B6nig?= , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Sumit Semwal , Marco Pagani , Boris Brezillon Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org References: <20260701085920.3253248-2-phasta@kernel.org> <20260701085920.3253248-3-phasta@kernel.org> <51fbde808bd059510d667d130afae179626303be.camel@mailbox.org> Content-Language: en-GB From: Tvrtko Ursulin In-Reply-To: <51fbde808bd059510d667d130afae179626303be.camel@mailbox.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 03/07/2026 15:47, Philipp Stanner wrote: > On Fri, 2026-07-03 at 12:27 +0100, Tvrtko Ursulin wrote: >> >> On 01/07/2026 09:59, Philipp Stanner wrote: >>> The entity->last_scheduled field has always been set and read with >>> special RCU functions in addition to memory barriers. >>> >>> This was added in >>> >>> commit 70102d77ff22 ("drm/scheduler: add drm_sched_entity_error and use rcu for last_scheduled") >>> >>> however, no proper justification for that mechanism was provided. There >>> seems to be no obvious reason, since the entity lock is available and >>> taken at all places that evaluate the last_scheduled field. The only >>> exception is drm_sched_entity_error(), which is not performance critical >>> in any way. >>> >>> Improve robustness, readability and maintainability by replacing RCU and >>> barriers with the lock. >> >> First thing, and regardless of other strands of discussion, I think it >> should be squashed with 3/5 instead of that one undoing the introduction >> of lock-unlock-lock-unlock. > > I agree that there should not be a do-undo pattern, but I don't want to > squash that, it's quite a distinctive action. One patch adds locks, the > other moves them. Hm, maybe it is a semantic discussion whether there is any real adding of the locks, when the effective end result is just widening of it's scope by pulling it out of the helper to caller. But okay, prep patch to move the lock out sounds like it could look acceptable. > But what I can do is move that patch before №1 here so that it becomes > understandable as a preparational commit. > >> >> For what the main topic is concerned, I really like the removal of all >> the rcu_dereference_check(, true) lines and the memory barriers. >> >> But I also think the commit message should explain better what code >> paths are now taking an extra lock - under which circumstances is the >> lock now taken for all scheduler users, and which amdgpu paths use >> drm_sched_entity_error() a lot so could be affected. I doubt it creates >> a measurable performance impact but it needs to be explained. > > I think it can detail which functions will now be locked; but > mentioning the users would be overkill and is uncommon for API reworks. Here I disagree quite strongly. Given the patch is making strong claims that the lockless access was added for no obvious reason, and that we have now established the lockless helper is in fact used on the submission paths, it is really required that those strong claims are backed by a concrete analysis instead of just saying "not performance critical in any way". >> I am also happy to give it a spin on the Steam Deck to see if I can >> observe anything. > > Could be interesting. Okay I'll try to do it in reasonable time. You can either respin or wait for it, I don't mind either way. >>> Signed-off-by: Philipp Stanner >>> --- >>>   drivers/gpu/drm/scheduler/sched_entity.c | 50 ++++++++++-------------- >>>   include/drm/gpu_scheduler.h              |  9 ++--- >>>   2 files changed, 25 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c >>> index c51101ec70c1..91aec20611ad 100644 >>> --- a/drivers/gpu/drm/scheduler/sched_entity.c >>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c >>> @@ -135,7 +135,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, >>>    entity->num_sched_list = num_sched_list; >>>    entity->sched_list = num_sched_list > 1 ? sched_list : NULL; >>>    entity->rq = &sched_list[0]->rq; >>> - RCU_INIT_POINTER(entity->last_scheduled, NULL); >>>    RB_CLEAR_NODE(&entity->rb_tree_node); >>>    init_completion(&entity->entity_idle); >>> >>> @@ -201,10 +200,10 @@ int drm_sched_entity_error(struct drm_sched_entity *entity) >>>    struct dma_fence *fence; >>>    int r; >>> >>> - rcu_read_lock(); >>> - fence = rcu_dereference(entity->last_scheduled); >>> + spin_lock(&entity->lock); >>> + fence = entity->last_scheduled; >>>    r = fence ? fence->error : 0; >>> - rcu_read_unlock(); >>> + spin_unlock(&entity->lock); >>> >>>    return r; >>>   } >>> @@ -287,9 +286,10 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity) >>>    /* Make sure this entity is not used by the scheduler at the moment */ >>>    wait_for_completion(&entity->entity_idle); >>> >>> - /* The entity is guaranteed to not be used by the scheduler */ >>> - prev = rcu_dereference_check(entity->last_scheduled, true); >>> + spin_lock(&entity->lock); >>> + prev = entity->last_scheduled; >>>    dma_fence_get(prev); >>> + spin_unlock(&entity->lock); >>>    while ((job = drm_sched_entity_queue_pop(entity))) { >>>    struct drm_sched_fence *s_fence = job->s_fence; >>> >>> @@ -381,8 +381,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) >>>    entity->dependency = NULL; >>>    } >>> >>> - dma_fence_put(rcu_dereference_check(entity->last_scheduled, true)); >>> - RCU_INIT_POINTER(entity->last_scheduled, NULL); >>> + dma_fence_put(entity->last_scheduled); >>>    drm_sched_entity_stats_put(entity->stats); >>>   } >>>   EXPORT_SYMBOL(drm_sched_entity_fini); >>> @@ -507,6 +506,10 @@ drm_sched_job_dependency(struct drm_sched_job *job, >>> >>>   struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) >>>   { >>> + /* Helper to avoid dropping the reference while the entity lock is held, >>> + * just to have some more robustness. >>> + */ >> >> I don't get this comment. Neither the placement or the content. > > It explains the purpose of the variable 'prev_last_scheduled', which > exists so that a reference does not drop under lock protection. Ah a helper _variable_, right, I was thrown off by the comment just below the function and did not even spot you added a new local. Regards, Tvrtko > > > P.