From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 A908918A6A9 for ; Mon, 27 Jan 2025 18:59:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738004354; cv=none; b=TyOycrGnUEf+0A2Tm8b7X6Amb/PegEjQf46x2brsL4SoESTVCsg4f9kMcX/P/NZLYu7f+uc5n0ZH2NQfuFVa74gu1BEdIcF6cI9JzeAmcEejgbzpnPja9MdieFvdiOLDtMnaP8qprXtGn70rgWpoifNUXCl85xHbyZctIHDc13o= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738004354; c=relaxed/simple; bh=G2Ig/jSUvjTNe2kQvkEK5vkjVqqyBJDuLul6ueUHank=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=omqA/AGSQfEEtMqV+82p6TawihYsI0rnLQY95dhP3U6snEQIpdsOPFadaQc93s2LTKFV5RA+4jOs/HoPaUllV/TGxDnIOTuDlLhEBO9NiVdXn9UYRqL7JLr5XSZV43+dDw/6xVYuc3R9IiGIIGoF//ORaTrxWsS7ncuUSTTzt04= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=RIOs/uDu; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="RIOs/uDu" Received: by smtp.kernel.org (Postfix) with ESMTPSA id EFD83C4CED2; Mon, 27 Jan 2025 18:59:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1738004354; bh=G2Ig/jSUvjTNe2kQvkEK5vkjVqqyBJDuLul6ueUHank=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RIOs/uDuklvkzTefPuw2f0yi8A+EARsGULtGFt8JExTC5grUOY8GYJSkmTFYGTlhB YWUaZmKHjj1EMYFc1LiTV7EYah3qcG8VLrqX50NmM1JrT1n8C3TQXb84DalhIWUSk2 kXCAefJBuoeASEbthYXdx26+imcg9YeFfMq4MdDiwCEVTkvPhzNdcLRLD+6Z6Tdccg X07+BWoMTvfS09cTLmgU5lornvbvnjFAjjNWCOSJRbollX9yy0FtGI2v7g69njLLWj tpSgtXrrt2pRSWK8RzOHdY+rU2ZQeIwcSqsrzmrZZZR6ZQ4LFaOK//kIsxs41N5t0G q5Di99l2Nj8fQ== Date: Mon, 27 Jan 2025 08:59:12 -1000 From: Tejun Heo To: Andrea Righi Cc: David Vernet , Changwoo Min , linux-kernel@vger.kernel.org Subject: Re: [PATCH v4] sched_ext: Fix lock imbalance in dispatch_to_local_dsq() Message-ID: References: <20250125091657.203445-1-arighi@nvidia.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250125091657.203445-1-arighi@nvidia.com> Hello, On Sat, Jan 25, 2025 at 10:16:57AM +0100, Andrea Righi wrote: ... > @@ -2253,9 +2253,11 @@ static void move_local_task_to_local_dsq(struct task_struct *p, u64 enq_flags, > * @dst_rq: rq to move the task into, locked on return > * > * Move @p which is currently on @src_rq to @dst_rq's local DSQ. > + * > + * Return the rq where @p has been moved. > */ > -static void move_remote_task_to_local_dsq(struct task_struct *p, u64 enq_flags, > - struct rq *src_rq, struct rq *dst_rq) > +static struct rq *move_remote_task_to_local_dsq(struct task_struct *p, u64 enq_flags, > + struct rq *src_rq, struct rq *dst_rq) > { > lockdep_assert_rq_held(src_rq); > > @@ -2277,6 +2279,8 @@ static void move_remote_task_to_local_dsq(struct task_struct *p, u64 enq_flags, > dst_rq->scx.extra_enq_flags = enq_flags; > activate_task(dst_rq, p, 0); > dst_rq->scx.extra_enq_flags = 0; > + > + return dst_rq; The returned dst_rq always matches the input param, right? Let's please not do this. The return value can mislead users to assume that the returned value may be different from the input. e.g. kobj_get() does similar identity return for convenience and that led people to assume that kobj_get() does zero-ref testing before inc'ing which led to a group of bugs. Just follow up the call statement with an explicit assignment if necessary. Nothing meaningful is achieved by merging that into the call statement. > +/** > + * dispatch_to_local_dsq - Dispatch a task to a local dsq > + * @rq: current rq which is locked > + * @dst_dsq: destination DSQ > + * @p: task to dispatch > + * @enq_flags: %SCX_ENQ_* > + * > + * We're holding @rq lock and want to dispatch @p to @dst_dsq which is a local > + * DSQ. This function performs all the synchronization dancing needed because > + * local DSQs are protected with rq locks. > + * > + * The caller must have exclusive ownership of @p (e.g. through > + * %SCX_OPSS_DISPATCHING). > + */ > +static void dispatch_to_local_dsq(struct rq *rq, struct scx_dispatch_q *dst_dsq, > + struct task_struct *p, u64 enq_flags) > +{ > + struct rq *src_rq = task_rq(p); > + struct rq *dst_rq = container_of(dst_dsq, struct rq, scx.local_dsq); > + > + /* > + * We're synchronized against dequeue through DISPATCHING. As @p can't > + * be dequeued, its task_rq and cpus_allowed are stable too. > + * > + * If dispatching to @rq that @p is already on, no lock dancing needed. > + */ > + if (rq == src_rq && rq == dst_rq) { > + dispatch_enqueue(dst_dsq, p, enq_flags | SCX_ENQ_CLEAR_OPSS); > + return; > + } > + > + move_to_remote_dsq(rq, src_rq, dst_rq, p, enq_flags); I'm not sure the refactoring is adding much, but even if it does: - All the changes the patch makes and why should be explained in the description. - The refactoring is obscuring the actual fix. If refactoring is desirable, please put it in a separate patch. Thanks. -- tejun