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 7CD36CEACEF for ; Mon, 17 Nov 2025 11:07:16 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D6C7810E03A; Mon, 17 Nov 2025 11:07:15 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 2F55A10E08F for ; Mon, 17 Nov 2025 11:07:15 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 23101FEC; Mon, 17 Nov 2025 03:07:07 -0800 (PST) Received: from [10.57.69.30] (unknown [10.57.69.30]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B50EF3F66E; Mon, 17 Nov 2025 03:07:13 -0800 (PST) Message-ID: <1cfa2075-a229-44f2-b078-e46334c328fb@arm.com> Date: Mon, 17 Nov 2025 11:07:11 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset() To: Boris Brezillon Cc: Liviu Dudau , =?UTF-8?Q?Adri=C3=A1n_Larumbe?= , dri-devel@lists.freedesktop.org, Lars-Ivar Hesselberg Simonsen , kernel@collabora.com References: <20251031154818.821054-1-boris.brezillon@collabora.com> <20251031154818.821054-5-boris.brezillon@collabora.com> <717fea87-6927-484a-b373-cb1917185c0f@arm.com> <20251112134725.160734c4@fedora> <285e5435-7dca-4539-a5ab-aae82df4ef7b@arm.com> <20251117112634.0542ee63@fedora> From: Steven Price Content-Language: en-GB In-Reply-To: <20251117112634.0542ee63@fedora> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 17/11/2025 10:26, Boris Brezillon wrote: > On Mon, 17 Nov 2025 09:49:27 +0000 > Steven Price wrote: > >> On 12/11/2025 12:47, Boris Brezillon wrote: >>> On Fri, 7 Nov 2025 16:40:53 +0000 >>> Steven Price wrote: >>> >>>> On 31/10/2025 15:48, Boris Brezillon wrote: >>>>> A group can become runnable even after reset.in_progress has >>>>> been set to true and panthor_sched_suspend() has been called, >>>>> because the drm_sched queues are still running at that point, >>>>> and ::run_job() might call group_schedule_locked() which moves >>>>> the group to the runnable list. And that's fine, because we're >>>>> moving those groups to the stopped list anyway when we call >>>>> panthor_group_stop(), so just drop the misleading WARN_ON(). >>>> >>>> If we've got another thread mutating the runnable list between >>>> panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that >>>> make the list iterator unsafe? (_safe only protects against deleting the >>>> current item, not against concurrent access). >>> >>> I'm not too sure actually. There's an >>> atomic_read(&sched->reset.in_progress) to check if we're about to reset >>> in group_schedule_locked() and cancel the insertion into the runnable >>> list in that case, meaning we're sure nothing new will be inserted after >>> we've set the in_progress=true in panthor_sched_pre_reset(). >> >> I was mostly going on your commit message: >> >>> A group can become runnable even after reset.in_progress has >>> been set to true and panthor_sched_suspend() has been called >> >> if that is indeed happening then we have a problem (and removing the >> WARN_ON is just papering over it). I haven't actually followed through >> the logic. > > Sorry, it's not exactly that. The problem is that a group might be > inserted in the runnable list before we've had a chance to set > reset.in_progress=true (earlier in this function), and there's nothing > removing those groups from the runnable list between this assignment and > the loop stopping the groups. Ok, so the commit message is misleading... My understanding is that the tick and sync_upd is what moves things onto the runnable list. So we set the "reset.in_progress" flag and cancel (and sync) with the two work queues which could move things onto runnable. panthor_sched_suspend() then deals with anything actually running, but it does appear we're missing anything to deal with the runnable list, hence the WARN_ON needs dropping. >> >>>> >>>> It feels to me like we should be holding the sched mutex - at least >>>> while iterating. I agree the WARN_ON is unnecessary, and will need >>>> removing if we simply guard the iteration - the alternative is to >>>> recolour panthor_sched_suspend() to assume the lock is held (and take >>>> the lock in panthor_sched_pre_reset), but I suspect that's a more ugly >>>> change. >>> >>> I'd rather ensure that nothing new is inserted in the runnable/idle >>> lists after sched->reset.in_progress is set to true. Note that >>> sched->reset.in_progress is set to true with the sched lock held, >>> meaning any path modifying the sched lists (must be done with the sched >>> lock held) should complete before we set this to true. As long as those >>> paths also skip the list insertion, or, for things happening in a work >>> context (thinking of the tick work here), as long as the work is not >>> rescheduled until we get a chance to disable this work, we should be >>> good, no? >> >> Yes that design can work. But atomics can be difficult to reason about, >> so unless there's a good reason I think we'd generally be better >> sticking with (simple) locks > > Locks alone won't prevent groups from being moved around after the > stop_all_groups loop though. It's the lock plus the fact groups can't > have their state changed while a reset is in progress that gives this > guarantee, at which point I guess checking reset.in_progress with or > without the lock held is the same, no? We do change the > reset.in_progress state with the sched.reset.lock held though, to make > sure any path that could move the group to a different list is out of > the way when we exit the locked section. That means that new threads > entering such paths (path changing the group state) will see the new > value and bail out. No you're right - the lock by itself won't work because we need to drop all locks before returning from pre_reset() and we need to keep that state until post_reset(). >> on the slow paths, then we get the benefits >> of lockdep etc checking we haven't messed up. > > I don't mind taking the sched lock in this slow path, but I don't think > taking/releasing it inside panthor_sched_pre_reset() is giving any more > safeness, because what we want is a guarantee that groups won't be > moved around between panthor_sched_pre_reset() and > panthor_sched_post_reset(). If that's really a change we want to push > (reworking the locking in the reset sequence), I'd rather do that in > its own patchset, if you don't mind. I think the real issue was the commit message misleading me. I'd assumed from the commit message that you'd observed the situation of a group being put on the runnable list between the call to panthor_sched_suspend() and the drm_WARN_ON check. If that really did happen then there's a much deeper bug somewhere that needs fixing. If I understand correctly what you've actually observed is a group being put on the runnable list *before* panthor_sched_suspend() and we're not currently handling that[1]. I agree that simply dropping the drm_WARN_ON() would be the logic fix for that. TLDR; The commit message needs some work ;) Thanks, Steve [1] Well we are, but with a big WARN_ON splat ;)