Message ID | 170905254443.2268463.935306988251313983.stgit@dwillia2-xfh.jf.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-83650-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:a81b:b0:108:e6aa:91d0 with SMTP id bq27csp2822649dyb; Tue, 27 Feb 2024 08:50:07 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCUfkfEGVK1MC+v87wN7Egg5JTi3GUXZEG1+kH56RF7pDNs06nDawfeu3rxaj4M1fELPtlQ+Sk9bANQu9ncRZJ2LjVfP4g== X-Google-Smtp-Source: AGHT+IGfDhFU9MF81y+RvYIBVy+HdmIPiC0Oc94sosiO7nhZVs17rZgR4uoXJll+1hj1ffGGIY92 X-Received: by 2002:a9d:7cc4:0:b0:6e4:80c3:af76 with SMTP id r4-20020a9d7cc4000000b006e480c3af76mr10259774otn.17.1709052606755; Tue, 27 Feb 2024 08:50:06 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709052606; cv=pass; d=google.com; s=arc-20160816; b=KT/0BNjQfI7KtyiX4ylspb62hmCNHVsfagk7OraiEh+Y4EphvB+mEodb+9X/0hr8t/ qs2Zm+IPJvkrCTc7abvnO/Reb8h8thnf/5mMSaBh8TW32g6UEQdSNQXd7zdz/jPW9RrC QF81f3B8pHV/IaeSfzqmr/x4LVqDYsT9lcbyyWRdvrPIvy1YKVGLt7GHGyfyjTksHq0v 6nkWEy++0vvBnFl+zIdqbJ04+VjHWUd+R6cn3ol6Jh9iHYPH+ZwkY6SnNxcGecWFC/9o Fqh0NNOYq59fUZyNtOJ5Dghiuiy/wDo/kbHFfvLKQuUPo4uos7x2gcxGVpucZP9763h9 n90g== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:user-agent:references:in-reply-to :message-id:date:cc:to:from:subject:dkim-signature; bh=QxkknGacMFhDHeGPAHuUP/JH0dbE4m2kP6y6jjyYCsY=; fh=J+y8fwVqf69pkg13Fh9pTaAnYpohIzxQcHuTScC561I=; b=xDwv7d4JhtwcVl2gbyG6UZXtuoUFaTf1uM2lFEnXHSGgcFSUoJpyOf7GTyvzHQ+aqE RyvmbS+T5QQ/Rh8a9Yj5Rna+7VZhfRrZCsTlCGlUoXrTzg9BCcMkKBCJQiIPw7yhtT02 FCer8PuIcx1i9bDlMC3LXJhZFr7WNJ/uH1eQrdOoahPPEPYvIuSGx8ILzdGtENbad97N +nqGPltl8XMe8WNWxIeWyV89c/pcfouk9rO3Ht10WvaF//sdq8s7bHiDJCdtszvwcGUX Tj/lSWOjiMnx+IywSlJNkhXH2zusYYGpqWOi5a1zANzQYBhYzMT7zFNLBUOTOIWm9wKH sa7Q==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="mixHy/oz"; arc=pass (i=1 spf=pass spfdomain=intel.com dkim=pass dkdomain=intel.com dmarc=pass fromdomain=intel.com); spf=pass (google.com: domain of linux-kernel+bounces-83650-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-83650-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id 14-20020a63174e000000b005d66232593dsi5640544pgx.868.2024.02.27.08.50.06 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Feb 2024 08:50:06 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-83650-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="mixHy/oz"; arc=pass (i=1 spf=pass spfdomain=intel.com dkim=pass dkdomain=intel.com dmarc=pass fromdomain=intel.com); spf=pass (google.com: domain of linux-kernel+bounces-83650-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-83650-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 505BE2861AC for <ouuuleilei@gmail.com>; Tue, 27 Feb 2024 16:50:06 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 802F94EB4F; Tue, 27 Feb 2024 16:49:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="mixHy/oz" Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.8]) (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 73DD74EB43; Tue, 27 Feb 2024 16:49:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.8 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709052547; cv=none; b=lpMbV7OdAbvYSC1kAB1s7tUbgOVphC2gWcM3M+Cx+81Tnw3CZPb17U6PEOtzR6iMC60Clb8DwXcmFSiHKcENBHHbzBBa9neX9L1Kcxhg+vV1MU1FHgbnGOwsvsc7kbEsuFsLzgrJPZwUN9xuiWEIz4Z0VVIa0rZrC8iQ2kIP270= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709052547; c=relaxed/simple; bh=uM/i0Rm9q3ig1FjMY9O1+2SHrbYFfbfJXmzB9UyHcMg=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=rxatextoHO4lzHWBCV9l/r4EIDXFx345nJ+nfF696rpp/f8JvUbaaX9nhQ/qlIQoVtPzYED9UDFPaEAxB71WQoJ4s0KmpMv4UAWy/7c904aQcjRnIhyIA0heI8rhpEhai+GytETYMwbvaSGUZgg3k1C/RA8tDC/rQB+AwnmXA4E= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=mixHy/oz; arc=none smtp.client-ip=192.198.163.8 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1709052545; x=1740588545; h=subject:from:to:cc:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=uM/i0Rm9q3ig1FjMY9O1+2SHrbYFfbfJXmzB9UyHcMg=; b=mixHy/oz9ftJbGz9Kc9p3G02My0GRO1kXjM/b1mU2bKlklqCvnYUYlNN jRB1VSZ6+8V7IGdg09KIFfPG4O6b0jQ0CIrWtdy4S+SZqNYq8Lxp2hCXU VDDR7WEyAUEa1jJXGEukffFRP7WsfIwClDLkeQEamjui21uoJ7RyDTTkH nlS5Gex+bV2NYEck0uW6wQdQQUUGqk2HPAvGcla4JzJz5BAjUbJbpaK40 osxy5l+jwYyO8gyHNC6gMudixGJFzvZfadfWHuz4UY6YzzcQt+nsTEEPW wFSUVi4xFc93aOVAUNcCPlylyV35+YbMdh+Va4sOM4vkt5+zF5ZKNiCu+ Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10996"; a="20948840" X-IronPort-AV: E=Sophos;i="6.06,188,1705392000"; d="scan'208";a="20948840" Received: from orviesa006.jf.intel.com ([10.64.159.146]) by fmvoesa102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Feb 2024 08:49:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,188,1705392000"; d="scan'208";a="7536117" Received: from sshaik-mobl1.amr.corp.intel.com (HELO dwillia2-xfh.jf.intel.com) ([10.209.88.67]) by orviesa006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Feb 2024 08:49:05 -0800 Subject: [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN() From: Dan Williams <dan.j.williams@intel.com> To: torvalds@linux-foundation.org, peterz@infradead.org, gregkh@linuxfoundation.org Cc: Ira Weiny <ira.weiny@intel.com>, Dave Jiang <dave.jiang@intel.com>, Ira Weiny <ira.weiny@intel.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>, linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org Date: Tue, 27 Feb 2024 08:49:04 -0800 Message-ID: <170905254443.2268463.935306988251313983.stgit@dwillia2-xfh.jf.intel.com> In-Reply-To: <170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com> References: <170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com> User-Agent: StGit/0.18-3-g996c Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792071546374792413 X-GMAIL-MSGID: 1792071546374792413 |
Series |
cleanup: A couple extensions for conditional resource management
|
|
Commit Message
Dan Williams
Feb. 27, 2024, 4:49 p.m. UTC
From: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com> Use cond_guard() in show_target() to not open code an up_read() in an 'out' block. If the down_read_interruptible() fails, the statement passed to the second argument of cond_guard() returns -EINTR. Cc: Peter Zijlstra <peterz@infradead.org> Suggested-by: Dan Williams <dan.j.williams@intel.com> Suggested-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/region.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
Comments
On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote: > > - rc = down_read_interruptible(&cxl_region_rwsem); > - if (rc) > - return rc; > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); Yeah, this is an example of how NOT to do things. If you can't make the syntax be something clean and sane like if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem)) return -EINTR; then this code should simply not be converted to guards AT ALL. Note that we have a perfectly fine way to do conditional lock guarding by simply using helper functions, which actually makes code MORE READABLE: if (!down_read_interruptible(&cxl_region_rwsem)) return -EINTR; rc = do_locked_function(); up_read(&cxl_region_rwsem); return rc; and notice how there are no special cases, no multiple unlocks, no NOTHING. And the syntax is clean. Honestly, if people are going to use 'guard' to write crap code, we need to really stop that in its tracks. There is no upside to making up new interfaces that only generate garbage. This is final. I'm not willing to even entertain this kind of crap. Linus
Linus Torvalds wrote: > On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote: > > > > - rc = down_read_interruptible(&cxl_region_rwsem); > > - if (rc) > > - return rc; > > + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); > > Yeah, this is an example of how NOT to do things. > > If you can't make the syntax be something clean and sane like > > if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem)) > return -EINTR; > > then this code should simply not be converted to guards AT ALL. > > Note that we have a perfectly fine way to do conditional lock guarding > by simply using helper functions, which actually makes code MORE > READABLE: > > if (!down_read_interruptible(&cxl_region_rwsem)) > return -EINTR; > rc = do_locked_function(); > up_read(&cxl_region_rwsem); > return rc; > > and notice how there are no special cases, no multiple unlocks, no > NOTHING. And the syntax is clean. Ok, I took the wrong lessons from scoped_cond_guard(). Consider it dropped. > Honestly, if people are going to use 'guard' to write crap code, we > need to really stop that in its tracks. > > There is no upside to making up new interfaces that only generate garbage. > > This is final. I'm not willing to even entertain this kind of crap. I will also note that these last 3 statements, nuking the proposal from space, I find excessive. Yes, on the internet no one can hear you being subtle, but the "MORE READABLE" and "NOTHING" were pretty darn unequivocal, especially coming from the person who has absolute final say on what enters his project. I have been around a while so I take this as par for the course, and I appreciate blunt feedback. I have had dance teachers tell me my "dancing is shit", and sometimes that level of bluntness is needed, but that was also from somebody I have worked with for years. Fabio has not been around that long, and nothing about what Fabio did was crap, he carried through on an idea that I asked him to explore and it did not work out. So Fabio, keep going, thank you for patiently working through this investigation and my takeaway is that we have successfully discovered one way this mission to cleanup usage of goto in the CXL subsystem can not proceed. Back to the drawing board.
On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@intel.com> wrote: > > I will also note that these last 3 statements, nuking the proposal from > space, I find excessive. Yes, on the internet no one can hear you being > subtle, but the "MORE READABLE" and "NOTHING" were pretty darn > unequivocal, especially coming from the person who has absolute final > say on what enters his project. Heh. It's not just " one can hear you being subtle", sometimes it's also "people don't take hints". It can be hard to tell.. Anyway, it's not that I hate the guard things in general. But I do think they need to be used carefully, and I do think it's very important that they have clean interfaces. The current setup came about after quite long discussions about getting reasonable syntax, and I'm still a bit worried even about the current simpler ones. And by "simpler ones" I don't mean our current scoped_cond_guard() thing. We have exactly one user of it, and I have considered getting rid of that one user because I think it's absolutely horrid. I haven't figured out a better syntax for it. For the non-scoped version, I actually think there *would* be a better syntax - putting the error case after the macro (the way we put the success case after the macro for the scoped one). In fact, maybe the solution is to make the scoped and non-scoped versions act very similar: we could do something like this: [scoped_]cond_guard(name, args) { success } else { fail }; and that syntax feels much more C-line to me. So maybe something like the attached (TOTALLY UNTESTED!!) patch for the scoped version, and then the non-scoped version would have the same syntax (except it would have to generate that __UNIQUE_ID() thing, of course). I haven't thought much about this. But I think this would be more acceptable to me, and also solve some of the ugliness with the current pre-existing scoped_cond_guard(). I dunno. PeterZ did the existing stuff, but he's offlined due to shoulder problems so not likely to chip in. Linus
Linus Torvalds wrote: > On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@intel.com> wrote: > > > > I will also note that these last 3 statements, nuking the proposal from > > space, I find excessive. Yes, on the internet no one can hear you being > > subtle, but the "MORE READABLE" and "NOTHING" were pretty darn > > unequivocal, especially coming from the person who has absolute final > > say on what enters his project. > > Heh. It's not just " one can hear you being subtle", sometimes it's > also "people don't take hints". It can be hard to tell.. I appreciate that. It is difficult to judge what size clue bat to carry from one thread to the next. > Anyway, it's not that I hate the guard things in general. But I do > think they need to be used carefully, and I do think it's very > important that they have clean interfaces. > > The current setup came about after quite long discussions about > getting reasonable syntax, and I'm still a bit worried even about the > current simpler ones. > > And by "simpler ones" I don't mean our current scoped_cond_guard() > thing. We have exactly one user of it, and I have considered getting > rid of that one user because I think it's absolutely horrid. I haven't > figured out a better syntax for it. > > For the non-scoped version, I actually think there *would* be a better > syntax - putting the error case after the macro (the way we put the > success case after the macro for the scoped one). > > In fact, maybe the solution is to make the scoped and non-scoped > versions act very similar: we could do something like this: > > [scoped_]cond_guard(name, args) { success } else { fail }; > > and that syntax feels much more C-line to me. > > So maybe something like the attached (TOTALLY UNTESTED!!) patch for > the scoped version, and then the non-scoped version would have the > same syntax (except it would have to generate that __UNIQUE_ID() > thing, of course). This would have definitely saved me from thinking that passing a "return" statement to a macro was an idea worth copying. I like that it puts the onus on the caller to understand "this is a conditional" you are responsible for handling the conditions, the macro is only handling releasing the lock at the end of the scope". > I haven't thought much about this. But I think this would be more > acceptable to me, and also solve some of the ugliness with the current > pre-existing scoped_cond_guard(). > > I dunno. PeterZ did the existing stuff, but he's offlined due to > shoulder problems so not likely to chip in. Ah, ok, yeah has been quiet on this thread for a while. I will take some inspiration from this proposal and huddle again with Fabio. Thanks for the nudge.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index ce0e2d82bb2b..704102f75c14 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -666,28 +666,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) { struct cxl_region_params *p = &cxlr->params; struct cxl_endpoint_decoder *cxled; - int rc; - rc = down_read_interruptible(&cxl_region_rwsem); - if (rc) - return rc; + cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem); if (pos >= p->interleave_ways) { dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos, p->interleave_ways); - rc = -ENXIO; - goto out; + return -ENXIO; } cxled = p->targets[pos]; if (!cxled) - rc = sysfs_emit(buf, "\n"); - else - rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); -out: - up_read(&cxl_region_rwsem); + return sysfs_emit(buf, "\n"); - return rc; + return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev)); } static int match_free_decoder(struct device *dev, void *data)