| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <687558d0897cc_2ead1002e@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 14 Jul 2025 12:21:52 -0700
From: <dan.j.williams@...el.com>
To: Shiju Jose <shiju.jose@...wei.com>, Dan Williams
<dan.j.williams@...el.com>, "linux-cxl@...r.kernel.org"
<linux-cxl@...r.kernel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "David
Lechner" <dlechner@...libre.com>, Peter Zijlstra <peterz@...radead.org>,
"Linus Torvalds" <torvalds@...ux-foundation.org>, Ingo Molnar
<mingo@...nel.org>, "Fabio M. De Francesco"
<fabio.m.de.francesco@...ux.intel.com>, "Davidlohr Bueso"
<dave@...olabs.net>, Jonathan Cameron <jonathan.cameron@...wei.com>, Dave
Jiang <dave.jiang@...el.com>, Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, "Ira Weiny" <ira.weiny@...el.com>
Subject: RE: [PATCH v3 8/8] cxl: Convert to ACQUIRE() for conditional rwsem
locking
Shiju Jose wrote:
> >-----Original Message-----
> >From: Dan Williams <dan.j.williams@...el.com>
> >Sent: 12 July 2025 00:50
> >To: linux-cxl@...r.kernel.org
> >Cc: linux-kernel@...r.kernel.org; David Lechner <dlechner@...libre.com>;
> >Peter Zijlstra <peterz@...radead.org>; Linus Torvalds <torvalds@...ux-
> >foundation.org>; Ingo Molnar <mingo@...nel.org>; Fabio M. De Francesco
> ><fabio.m.de.francesco@...ux.intel.com>; Davidlohr Bueso <dave@...olabs.net>;
> >Jonathan Cameron <jonathan.cameron@...wei.com>; Dave Jiang
> ><dave.jiang@...el.com>; Alison Schofield <alison.schofield@...el.com>; Vishal
> >Verma <vishal.l.verma@...el.com>; Ira Weiny <ira.weiny@...el.com>; Shiju Jose
> ><shiju.jose@...wei.com>
> >Subject: [PATCH v3 8/8] cxl: Convert to ACQUIRE() for conditional rwsem locking
> >
> >Use ACQUIRE() to cleanup conditional locking paths in the CXL driver The
> >ACQUIRE() macro and its associated ACQUIRE_ERR() helpers, like
> >scoped_cond_guard(), arrange for scoped-based conditional locking. Unlike
> >scoped_cond_guard(), these macros arrange for an ERR_PTR() to be retrieved
> >representing the state of the conditional lock.
> >
> >The goal of this conversion is to complete the removal of all explicit unlock calls
> >in the subsystem. I.e. the methods to acquire a lock are solely via guard(),
> >scoped_guard() (for limited cases), or ACQUIRE(). All unlock is implicit / scope-
> >based. In order to make sure all lock sites are converted, the existing rwsem's
> >are consolidated and renamed in 'struct cxl_rwsem'. While that makes the patch
> >noisier it gives a clean cut-off between old-world (explicit unlock allowed), and
> >new world (explicit unlock deleted).
> >
> >Cc: David Lechner <dlechner@...libre.com>
> >Cc: Peter Zijlstra <peterz@...radead.org>
> >Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> >Cc: Ingo Molnar <mingo@...nel.org>
> >Cc: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>
> >Cc: Davidlohr Bueso <dave@...olabs.net>
> >Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
> >Cc: Dave Jiang <dave.jiang@...el.com>
> >Cc: Alison Schofield <alison.schofield@...el.com>
> >Cc: Vishal Verma <vishal.l.verma@...el.com>
> >Cc: Ira Weiny <ira.weiny@...el.com>
> >Cc: Shiju Jose <shiju.jose@...wei.com>
> >Acked-by: "Peter Zijlstra (Intel)" <peterz@...radead.org>
> >Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>
> Hi Dan,
>
> For changes in CXL EDAC (drivers/cxl/core/edac.c),
> Tested-by: Shiju Jose <shiju.jose@...wei.com>
>
> >---
> > drivers/cxl/core/cdat.c | 6 +-
> > drivers/cxl/core/core.h | 17 ++-
> > drivers/cxl/core/edac.c | 44 +++---
> > drivers/cxl/core/hdm.c | 41 +++---
> > drivers/cxl/core/mbox.c | 6 +-
> > drivers/cxl/core/memdev.c | 50 +++----
> > drivers/cxl/core/port.c | 18 +--
> > drivers/cxl/core/region.c | 295 ++++++++++++++++----------------------
> > drivers/cxl/cxl.h | 13 +-
> > include/linux/rwsem.h | 1 +
> > 10 files changed, 212 insertions(+), 279 deletions(-)
> >
> [...]
> >diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c index
> >2cbc664e5d62..f1ebdbe222c8 100644
> >--- a/drivers/cxl/core/edac.c
> >+++ b/drivers/cxl/core/edac.c
> >@@ -115,10 +115,9 @@ static int cxl_scrub_get_attrbs(struct
> >cxl_patrol_scrub_context *cxl_ps_ctx,
> > flags, min_cycle);
> > }
> >
> >- struct rw_semaphore *region_lock __free(rwsem_read_release) =
> >- rwsem_read_intr_acquire(&cxl_region_rwsem);
> >- if (!region_lock)
> >- return -EINTR;
> >+ ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
> >+ if ((ret = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
> Checkpatch is giving error here and in other places with similar coding style,
> "ERROR: do not use assignment in if condition"
Yes, the proposal is that the rule be relaxed for ACQUIRE_ERR()
compactness similar to how the C99 variable declaration expectations
were relaxed for __free().
Prior discussion with Jonathan [1] and Alison [2] about this.
[1]: http://lore.kernel.org/6870833aa1344_588c100dd@dwillia2-xfh.jf.intel.com.notmuch
[2]: http://lore.kernel.org/aGXDMZB6omShJpoj@aschofie-mobl2.lan
In general, checkpatch also needs to catch up with cleanup macros like
DEFINE_FREE. So, I think that "checkpatch updates for cleanup.h" is a
worthwhile discussion as these helpers get more use.
> [...]
> > p = &cxlr->params;
> >@@ -2215,18 +2173,18 @@ int cxl_decoder_detach(struct cxl_region *cxlr,
> > struct cxl_region *detach;
> >
> > /* when the decoder is being destroyed lock unconditionally */
> >- if (mode == DETACH_INVALIDATE)
> >- down_write(&cxl_region_rwsem);
> >- else {
> >- int rc = down_write_killable(&cxl_region_rwsem);
> >+ if (mode == DETACH_INVALIDATE) {
> >+ guard(rwsem_write)(&cxl_rwsem.region);
> >+ detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
> >+ } else {
> >+ int rc;
> >
> >- if (rc)
> >+ ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> >+ if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
> > return rc;
> >+ detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
> > }
> May be detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
> add outside if ... else as before?
Recall that the lock is only held within its bracketed scope. So this...
if (mode == DETACH_INVALIDATE) {
guard(rwsem_write)(&cxl_rwsem.region);
} else {
int rc;
ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
return rc;
}
detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
...would be equivalent to:
if (mode == DETACH_INVALIDATE) {
down_write(&cxl_rwsem.region);
up_write(&cxl_rwsem.region);
} else {
int rc = down_write_killable(&cxl_rwsem.region);
if (rc)
return rc;
up_write(&cxl_rwsem.region);
}
detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
Powered by blists - more mailing lists