| Message ID | 08e8e7ec9a3413b91d51de39e385653494b1eed0.1701041440.git.alison.schofield@intel.com |
|---|---|
| State | Accepted |
| Commit | 5558b92e8d39e18aa19619be2ee37274e9592528 |
| Headers | show |
| Series | cxl/core: Hold region_rwsem during poison ops | expand |
On 11/26/23 17:09, alison.schofield@intel.com wrote: > From: Alison Schofield <alison.schofield@intel.com> > > A read of a device poison list is triggered via a sysfs attribute > and the results are logged as kernel trace events of type cxl_poison. > The work is managed by either: a) the region driver when one of more > regions map the device, or by b) the memdev driver when no regions > map the device. > > In the case of a) the region driver holds the region_rwsem while > reading the poison by committed endpoint decoder mappings and for > any unmapped resources. This makes sure that the cxl_poison trace > event trace reports valid region info. (Region name, HPA, and UUID). > > In the case of b) the memdev driver holds the dpa_rwsem preventing > new DPA resources from being attached to a region. However, it leaves > a gap between region attach and decoder commit actions. If a DPA in > the gap is in the poison list, the cxl_poison trace event will omit > the region info. > > Close the gap by holding the region_rwsem and the dpa_rwsem when > reading poison per memdev. Since both methods now hold both locks, > down_read both from the caller. Doing so also addresses the lockdep > assert that found this issue: > Commit 458ba8189cb4 ("cxl: Add cxl_decoders_committed() helper") > > Fixes: 7ff6ad107588 ("cxl/memdev: Add trigger_poison_list sysfs attribute") > Fixes: f0832a586396 ("cxl/region: Provide region info to the cxl_poison trace event") > Signed-off-by: Alison Schofield <alison.schofield@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/memdev.c | 9 ++++++++- > drivers/cxl/core/region.c | 5 ----- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index fc5c2b414793..5ad1b13e780a 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -227,10 +227,16 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd) > if (!port || !is_cxl_endpoint(port)) > return -EINVAL; > > - rc = down_read_interruptible(&cxl_dpa_rwsem); > + rc = down_read_interruptible(&cxl_region_rwsem); > if (rc) > return rc; > > + rc = down_read_interruptible(&cxl_dpa_rwsem); > + if (rc) { > + up_read(&cxl_region_rwsem); > + return rc; > + } > + > if (cxl_num_decoders_committed(port) == 0) { > /* No regions mapped to this memdev */ > rc = cxl_get_poison_by_memdev(cxlmd); > @@ -239,6 +245,7 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd) > rc = cxl_get_poison_by_endpoint(port); > } > up_read(&cxl_dpa_rwsem); > + up_read(&cxl_region_rwsem); > > return rc; > } > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 56e575c79bb4..3e817a6f94c6 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -2467,10 +2467,6 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) > struct cxl_poison_context ctx; > int rc = 0; > > - rc = down_read_interruptible(&cxl_region_rwsem); > - if (rc) > - return rc; > - > ctx = (struct cxl_poison_context) { > .port = port > }; > @@ -2480,7 +2476,6 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) > rc = cxl_get_poison_unmapped(to_cxl_memdev(port->uport_dev), > &ctx); > > - up_read(&cxl_region_rwsem); > return rc; > } >
On Sun, 26 Nov 2023, alison.schofield@intel.com wrote: >From: Alison Schofield <alison.schofield@intel.com> > >A read of a device poison list is triggered via a sysfs attribute >and the results are logged as kernel trace events of type cxl_poison. >The work is managed by either: a) the region driver when one of more >regions map the device, or by b) the memdev driver when no regions >map the device. > >In the case of a) the region driver holds the region_rwsem while >reading the poison by committed endpoint decoder mappings and for >any unmapped resources. This makes sure that the cxl_poison trace >event trace reports valid region info. (Region name, HPA, and UUID). > >In the case of b) the memdev driver holds the dpa_rwsem preventing >new DPA resources from being attached to a region. However, it leaves >a gap between region attach and decoder commit actions. If a DPA in >the gap is in the poison list, the cxl_poison trace event will omit >the region info. > >Close the gap by holding the region_rwsem and the dpa_rwsem when >reading poison per memdev. Since both methods now hold both locks, >down_read both from the caller. Doing so also addresses the lockdep >assert that found this issue: >Commit 458ba8189cb4 ("cxl: Add cxl_decoders_committed() helper") > >Fixes: 7ff6ad107588 ("cxl/memdev: Add trigger_poison_list sysfs attribute") >Fixes: f0832a586396 ("cxl/region: Provide region info to the cxl_poison trace event") >Signed-off-by: Alison Schofield <alison.schofield@intel.com> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > A read of a device poison list is triggered via a sysfs attribute > and the results are logged as kernel trace events of type cxl_poison. > The work is managed by either: a) the region driver when one of more > regions map the device, or by b) the memdev driver when no regions > map the device. > > In the case of a) the region driver holds the region_rwsem while > reading the poison by committed endpoint decoder mappings and for > any unmapped resources. This makes sure that the cxl_poison trace > event trace reports valid region info. (Region name, HPA, and UUID). > > In the case of b) the memdev driver holds the dpa_rwsem preventing > new DPA resources from being attached to a region. However, it leaves > a gap between region attach and decoder commit actions. If a DPA in > the gap is in the poison list, the cxl_poison trace event will omit > the region info. > > Close the gap by holding the region_rwsem and the dpa_rwsem when > reading poison per memdev. Since both methods now hold both locks, > down_read both from the caller. Doing so also addresses the lockdep > assert that found this issue: > Commit 458ba8189cb4 ("cxl: Add cxl_decoders_committed() helper") > > Fixes: 7ff6ad107588 ("cxl/memdev: Add trigger_poison_list sysfs attribute") I am going to drop this Fixes: tag. Every kernel that has this commit also has f0832a586396, and the there is nothing left to fix after backporting this new commit. > Fixes: f0832a586396 ("cxl/region: Provide region info to the cxl_poison trace event") > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/memdev.c | 9 ++++++++- > drivers/cxl/core/region.c | 5 ----- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c > index fc5c2b414793..5ad1b13e780a 100644 > --- a/drivers/cxl/core/memdev.c > +++ b/drivers/cxl/core/memdev.c > @@ -227,10 +227,16 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd) > if (!port || !is_cxl_endpoint(port)) > return -EINVAL; > > - rc = down_read_interruptible(&cxl_dpa_rwsem); > + rc = down_read_interruptible(&cxl_region_rwsem); > if (rc) > return rc; > > + rc = down_read_interruptible(&cxl_dpa_rwsem); > + if (rc) { > + up_read(&cxl_region_rwsem); > + return rc; I am going to leave this as is, but for future consideration I think it is ok reason that cxl_region_rwsem has a higher chance for contention. Compare how long cxl_region_rwsem is held vs cxl_dpa_rwsem. Once it is acquired, just commit to being uninterruptible with plain down_read() for cxl_dpa_rwsem.
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c index fc5c2b414793..5ad1b13e780a 100644 --- a/drivers/cxl/core/memdev.c +++ b/drivers/cxl/core/memdev.c @@ -227,10 +227,16 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd) if (!port || !is_cxl_endpoint(port)) return -EINVAL; - rc = down_read_interruptible(&cxl_dpa_rwsem); + rc = down_read_interruptible(&cxl_region_rwsem); if (rc) return rc; + rc = down_read_interruptible(&cxl_dpa_rwsem); + if (rc) { + up_read(&cxl_region_rwsem); + return rc; + } + if (cxl_num_decoders_committed(port) == 0) { /* No regions mapped to this memdev */ rc = cxl_get_poison_by_memdev(cxlmd); @@ -239,6 +245,7 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd) rc = cxl_get_poison_by_endpoint(port); } up_read(&cxl_dpa_rwsem); + up_read(&cxl_region_rwsem); return rc; } diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 56e575c79bb4..3e817a6f94c6 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -2467,10 +2467,6 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) struct cxl_poison_context ctx; int rc = 0; - rc = down_read_interruptible(&cxl_region_rwsem); - if (rc) - return rc; - ctx = (struct cxl_poison_context) { .port = port }; @@ -2480,7 +2476,6 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port) rc = cxl_get_poison_unmapped(to_cxl_memdev(port->uport_dev), &ctx); - up_read(&cxl_region_rwsem); return rc; }