diff mbox series

[v13,6/9] cxl/memdev: Add trigger_poison_list sysfs attribute

Message ID 1081cfdc8a349dc754779642d584707e56db26ba.1681838291.git.alison.schofield@intel.com
State Accepted
Commit 7ff6ad1075885fdc71f6fea94b95109a582dec29
Headers show
Series CXL Poison List Retrieval & Tracing | expand

Commit Message

Alison Schofield April 18, 2023, 5:39 p.m. UTC
From: Alison Schofield <alison.schofield@intel.com>

When a boolean 'true' is written to this attribute the memdev driver
retrieves the poison list from the device. The list consists of
addresses that are poisoned, or would result in poison if accessed,
and the source of the poison. This attribute is only visible for
devices supporting the capability. The retrieved errors are logged
as kernel events when cxl_poison event tracing is enabled.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl | 14 ++++++++
 drivers/cxl/core/memdev.c               | 43 +++++++++++++++++++++++++
 drivers/cxl/cxlmem.h                    |  3 +-
 drivers/cxl/mem.c                       | 43 +++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 1 deletion(-)

Comments

Davidlohr Bueso April 26, 2023, 2:38 a.m. UTC | #1
On Tue, 18 Apr 2023, alison.schofield@intel.com wrote:

>From: Alison Schofield <alison.schofield@intel.com>
>
>When a boolean 'true' is written to this attribute the memdev driver
>retrieves the poison list from the device. The list consists of
>addresses that are poisoned, or would result in poison if accessed,
>and the source of the poison. This attribute is only visible for
>devices supporting the capability. The retrieved errors are logged
>as kernel events when cxl_poison event tracing is enabled.

btw thinking of scan media, is there some interface you already
have in mind?

Afaict we could have a new 'trigger_poison_list_nocache' attribute,
or write "scan_overflow" or whatever to this very file.

Thanks,
Davidlohr
Alison Schofield April 27, 2023, 4:11 a.m. UTC | #2
On Tue, Apr 25, 2023 at 07:38:11PM -0700, Davidlohr Bueso wrote:
> On Tue, 18 Apr 2023, alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > When a boolean 'true' is written to this attribute the memdev driver
> > retrieves the poison list from the device. The list consists of
> > addresses that are poisoned, or would result in poison if accessed,
> > and the source of the poison. This attribute is only visible for
> > devices supporting the capability. The retrieved errors are logged
> > as kernel events when cxl_poison event tracing is enabled.
> 
> btw thinking of scan media, is there some interface you already
> have in mind?
> 
> Afaict we could have a new 'trigger_poison_list_nocache' attribute,
> or write "scan_overflow" or whatever to this very file.

At the moment, I have 'trigger_scan_media' that results in a spew of
scan media results to a trace file. I guess we'll chat more about 
presenting a _nocache, when I post for review.

I'm concerned that a  _nocache_ will lead user to believe they are
getting 'fresher' results, and to over-use it with that mindset.
Get Poison List gives accurate results and Scan Media is only necessary
when the poison list has overflowed. OK...here I go opening the can of
worms myself ;)


> 
> Thanks,
> Davidlohr
Davidlohr Bueso April 27, 2023, 3:39 p.m. UTC | #3
On Wed, 26 Apr 2023, Alison Schofield wrote:

>At the moment, I have 'trigger_scan_media' that results in a spew of
>scan media results to a trace file. I guess we'll chat more about
>presenting a _nocache, when I post for review.

I'm not crazy about calling the interface scan media straight how just
because it's very unintuitive - albeit users that care will probably
don't care.

>
>I'm concerned that a  _nocache_ will lead user to believe they are
>getting 'fresher' results, and to over-use it with that mindset.

Agreed.

>Get Poison List gives accurate results and Scan Media is only necessary
>when the poison list has overflowed. OK...here I go opening the can of
>worms myself ;)

trigger_poison_list_overflow?

If triggered and the list hasn't overflowed yet, just be a nop?

Thanks,
Davidlohr
Dan Williams April 27, 2023, 4:35 p.m. UTC | #4
Davidlohr Bueso wrote:
> On Wed, 26 Apr 2023, Alison Schofield wrote:
> 
> >At the moment, I have 'trigger_scan_media' that results in a spew of
> >scan media results to a trace file. I guess we'll chat more about
> >presenting a _nocache, when I post for review.
> 
> I'm not crazy about calling the interface scan media straight how just
> because it's very unintuitive - albeit users that care will probably
> don't care.
> 
> >
> >I'm concerned that a  _nocache_ will lead user to believe they are
> >getting 'fresher' results, and to over-use it with that mindset.
> 
> Agreed.
> 
> >Get Poison List gives accurate results and Scan Media is only necessary
> >when the poison list has overflowed. OK...here I go opening the can of
> >worms myself ;)
> 
> trigger_poison_list_overflow?
> 
> If triggered and the list hasn't overflowed yet, just be a nop?

What about a "poison_overflow" policy? Defaults to "report", but you can
write "scan" to have trigger_poison_list invocations fallback to
scan_media on overflow.
Alison Schofield April 27, 2023, 7:18 p.m. UTC | #5
On Thu, Apr 27, 2023 at 09:35:02AM -0700, Dan Williams wrote:
> Davidlohr Bueso wrote:
> > On Wed, 26 Apr 2023, Alison Schofield wrote:
> > 
> > >At the moment, I have 'trigger_scan_media' that results in a spew of
> > >scan media results to a trace file. I guess we'll chat more about
> > >presenting a _nocache, when I post for review.
> > 
> > I'm not crazy about calling the interface scan media straight how just
> > because it's very unintuitive - albeit users that care will probably
> > don't care.
> > 
> > >
> > >I'm concerned that a  _nocache_ will lead user to believe they are
> > >getting 'fresher' results, and to over-use it with that mindset.
> > 
> > Agreed.
> > 
> > >Get Poison List gives accurate results and Scan Media is only necessary
> > >when the poison list has overflowed. OK...here I go opening the can of
> > >worms myself ;)
> > 
> > trigger_poison_list_overflow?
> > 
> > If triggered and the list hasn't overflowed yet, just be a nop?
> 
> What about a "poison_overflow" policy? Defaults to "report", but you can
> write "scan" to have trigger_poison_list invocations fallback to
> scan_media on overflow.

With a policy like that, the driver may automatically do scan media
in response to overflow reports.

For users that want to schedule those scans (policy set to 'report'),
they can do so my doing trigger_scan_media at their leisure.

Would you expect the driver to retain overflow state, and warn those
users that they are issuing scan_media without prior overflow events?
Dan Williams April 27, 2023, 7:54 p.m. UTC | #6
Alison Schofield wrote:
> On Thu, Apr 27, 2023 at 09:35:02AM -0700, Dan Williams wrote:
> > Davidlohr Bueso wrote:
> > > On Wed, 26 Apr 2023, Alison Schofield wrote:
> > > 
> > > >At the moment, I have 'trigger_scan_media' that results in a spew of
> > > >scan media results to a trace file. I guess we'll chat more about
> > > >presenting a _nocache, when I post for review.
> > > 
> > > I'm not crazy about calling the interface scan media straight how just
> > > because it's very unintuitive - albeit users that care will probably
> > > don't care.
> > > 
> > > >
> > > >I'm concerned that a  _nocache_ will lead user to believe they are
> > > >getting 'fresher' results, and to over-use it with that mindset.
> > > 
> > > Agreed.
> > > 
> > > >Get Poison List gives accurate results and Scan Media is only necessary
> > > >when the poison list has overflowed. OK...here I go opening the can of
> > > >worms myself ;)
> > > 
> > > trigger_poison_list_overflow?
> > > 
> > > If triggered and the list hasn't overflowed yet, just be a nop?
> > 
> > What about a "poison_overflow" policy? Defaults to "report", but you can
> > write "scan" to have trigger_poison_list invocations fallback to
> > scan_media on overflow.
> 
> With a policy like that, the driver may automatically do scan media
> in response to overflow reports.

Sure that's what the policy enables.

> For users that want to schedule those scans (policy set to 'report'),
> they can do so my doing trigger_scan_media at their leisure.

It's not clear that scan media matters so much in practice that the
kernel needs to have separate controls for it.

One of the observations of the overflow threshold is that the failing
part is likely to be RMA'd before the poison list fills up. Scan Media
was more relevant in the PMEM days [1] before List Poison was introduced
in CXL that promised to always record poison on creation.

If Scan Media is an operation that is rarely used, does the kernel need
to carry a trigger_scan_media attribute? I.e. the kernel only needs one
way to kick off a poison collection event and the depth of that
collection is an optional policy setting. Whether that policy needs to
be configurable is also an argument that needs evidence.

> Would you expect the driver to retain overflow state, and warn those
> users that they are issuing scan_media without prior overflow events?

I don't follow the use of plural "users". Only the single platform owner
cares about the health of the hardware.

[1]: https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V3.0.pdf
Alison Schofield April 27, 2023, 9:35 p.m. UTC | #7
On Thu, Apr 27, 2023 at 12:54:38PM -0700, Dan Williams wrote:
> Alison Schofield wrote:
> > On Thu, Apr 27, 2023 at 09:35:02AM -0700, Dan Williams wrote:
> > > Davidlohr Bueso wrote:
> > > > On Wed, 26 Apr 2023, Alison Schofield wrote:
> > > > 
> > > > >At the moment, I have 'trigger_scan_media' that results in a spew of
> > > > >scan media results to a trace file. I guess we'll chat more about
> > > > >presenting a _nocache, when I post for review.
> > > > 
> > > > I'm not crazy about calling the interface scan media straight how just
> > > > because it's very unintuitive - albeit users that care will probably
> > > > don't care.
> > > > 
> > > > >
> > > > >I'm concerned that a  _nocache_ will lead user to believe they are
> > > > >getting 'fresher' results, and to over-use it with that mindset.
> > > > 
> > > > Agreed.
> > > > 
> > > > >Get Poison List gives accurate results and Scan Media is only necessary
> > > > >when the poison list has overflowed. OK...here I go opening the can of
> > > > >worms myself ;)
> > > > 
> > > > trigger_poison_list_overflow?
> > > > 
> > > > If triggered and the list hasn't overflowed yet, just be a nop?
> > > 
> > > What about a "poison_overflow" policy? Defaults to "report", but you can
> > > write "scan" to have trigger_poison_list invocations fallback to
> > > scan_media on overflow.
> > 
> > With a policy like that, the driver may automatically do scan media
> > in response to overflow reports.
> 
> Sure that's what the policy enables.
> 
> > For users that want to schedule those scans (policy set to 'report'),
> > they can do so my doing trigger_scan_media at their leisure.
> 
> It's not clear that scan media matters so much in practice that the
> kernel needs to have separate controls for it.
> 
> One of the observations of the overflow threshold is that the failing
> part is likely to be RMA'd before the poison list fills up. Scan Media
> was more relevant in the PMEM days [1] before List Poison was introduced
> in CXL that promised to always record poison on creation.
> 
> If Scan Media is an operation that is rarely used, does the kernel need
> to carry a trigger_scan_media attribute? I.e. the kernel only needs one
> way to kick off a poison collection event and the depth of that
> collection is an optional policy setting. Whether that policy needs to
> be configurable is also an argument that needs evidence.
> 
> > Would you expect the driver to retain overflow state, and warn those
> > users that they are issuing scan_media without prior overflow events?
> 
> I don't follow the use of plural "users". Only the single platform owner
> cares about the health of the hardware.

This conversation isn't new or a surprise. The no user access was discussed
previously, off-list. Let's pick it back up when I post the scan support
patchset - so we can get more eyes on it, and keep the discussion in one
place.

Alison

> 
> [1]: https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V3.0.pdf
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index 3acf2f17a73f..48ac0d911801 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -415,3 +415,17 @@  Description:
 		1), and checks that the hardware accepts the commit request.
 		Reading this value indicates whether the region is committed or
 		not.
+
+
+What:		/sys/bus/cxl/devices/memX/trigger_poison_list
+Date:		April, 2023
+KernelVersion:	v6.4
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(WO) When a boolean 'true' is written to this attribute the
+		memdev driver retrieves the poison list from the device. The
+		list consists of addresses that are poisoned, or would result
+		in poison if accessed, and the source of the poison. This
+		attribute is only visible for devices supporting the
+		capability. The retrieved errors are logged as kernel
+		events when cxl_poison event tracing is enabled.
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 0af8856936dc..71f5ec6ba6c1 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -106,6 +106,49 @@  static ssize_t numa_node_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(numa_node);
 
+static int cxl_get_poison_by_memdev(struct cxl_memdev *cxlmd)
+{
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	u64 offset, length;
+	int rc = 0;
+
+	/* CXL 3.0 Spec 8.2.9.8.4.1 Separate pmem and ram poison requests */
+	if (resource_size(&cxlds->pmem_res)) {
+		offset = cxlds->pmem_res.start;
+		length = resource_size(&cxlds->pmem_res);
+		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
+		if (rc)
+			return rc;
+	}
+	if (resource_size(&cxlds->ram_res)) {
+		offset = cxlds->ram_res.start;
+		length = resource_size(&cxlds->ram_res);
+		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
+		/*
+		 * Invalid Physical Address is not an error for
+		 * volatile addresses. Device support is optional.
+		 */
+		if (rc == -EFAULT)
+			rc = 0;
+	}
+	return rc;
+}
+
+int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
+{
+	int rc;
+
+	rc = down_read_interruptible(&cxl_dpa_rwsem);
+	if (rc)
+		return rc;
+
+	rc = cxl_get_poison_by_memdev(cxlmd);
+	up_read(&cxl_dpa_rwsem);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_trigger_poison_list, CXL);
+
 static struct attribute *cxl_memdev_attributes[] = {
 	&dev_attr_serial.attr,
 	&dev_attr_firmware_version.attr,
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 07775ab5af4e..68b9db545aae 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -145,7 +145,7 @@  struct cxl_mbox_cmd {
 	C(FWROLLBACK, -ENXIO, "rolled back to the previous active FW"),         \
 	C(FWRESET, -ENXIO, "FW failed to activate, needs cold reset"),		\
 	C(HANDLE, -ENXIO, "one or more Event Record Handles were invalid"),     \
-	C(PADDR, -ENXIO, "physical address specified is invalid"),		\
+	C(PADDR, -EFAULT, "physical address specified is invalid"),		\
 	C(POISONLMT, -ENXIO, "poison injection limit has been reached"),        \
 	C(MEDIAFAILURE, -ENXIO, "permanent issue with the media"),		\
 	C(ABORT, -ENXIO, "background cmd was aborted by device"),               \
@@ -688,6 +688,7 @@  int cxl_set_timestamp(struct cxl_dev_state *cxlds);
 int cxl_poison_state_init(struct cxl_dev_state *cxlds);
 int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		       struct cxl_region *cxlr);
+int cxl_trigger_poison_list(struct cxl_memdev *cxlmd);
 
 #ifdef CONFIG_CXL_SUSPEND
 void cxl_mem_active_inc(void);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 39c4b54f0715..b6a413facbd7 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -176,10 +176,53 @@  static int cxl_mem_probe(struct device *dev)
 	return devm_add_action_or_reset(dev, enable_suspend, NULL);
 }
 
+static ssize_t trigger_poison_list_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	bool trigger;
+	int rc;
+
+	if (kstrtobool(buf, &trigger) || !trigger)
+		return -EINVAL;
+
+	rc = cxl_trigger_poison_list(to_cxl_memdev(dev));
+
+	return rc ? rc : len;
+}
+static DEVICE_ATTR_WO(trigger_poison_list);
+
+static umode_t cxl_mem_visible(struct kobject *kobj, struct attribute *a, int n)
+{
+	if (a == &dev_attr_trigger_poison_list.attr) {
+		struct device *dev = kobj_to_dev(kobj);
+
+		if (!test_bit(CXL_POISON_ENABLED_LIST,
+			      to_cxl_memdev(dev)->cxlds->poison.enabled_cmds))
+			return 0;
+	}
+	return a->mode;
+}
+
+static struct attribute *cxl_mem_attrs[] = {
+	&dev_attr_trigger_poison_list.attr,
+	NULL
+};
+
+static struct attribute_group cxl_mem_group = {
+	.attrs = cxl_mem_attrs,
+	.is_visible = cxl_mem_visible,
+};
+
+__ATTRIBUTE_GROUPS(cxl_mem);
+
 static struct cxl_driver cxl_mem_driver = {
 	.name = "cxl_mem",
 	.probe = cxl_mem_probe,
 	.id = CXL_DEVICE_MEMORY_EXPANDER,
+	.drv = {
+		.dev_groups = cxl_mem_groups,
+	},
 };
 
 module_cxl_driver(cxl_mem_driver);