From: Alison Schofield <alison.schofield@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Wonjae Lee <wj28.lee@samsung.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Hojin Nam <hj96.nam@samsung.com>,
"nvdimm@lists.linux.dev" <nvdimm@lists.linux.dev>,
"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>
Subject: Re: [ndctl PATCH v11 6/7] cxl/list: add --media-errors option to cxl list
Date: Thu, 18 Apr 2024 13:12:39 -0700 [thread overview]
Message-ID: <ZiF+t0sVPOdE0X1K@aschofie-mobl2> (raw)
In-Reply-To: <ZgR3/AWytkKwX9u4@aschofie-mobl2>
Hi Dan,
Here's where I believe we last left off.
I thought we had closure on the json format of the media error records,
and on the fact that those objects are appended to memdev or region
objects.
The open is on how to use 'cxl list' to view the poison records.
Can we pick up that discussion below in this v11 thread?
The v12 that I refer to below is here:
https://lore.kernel.org/cover.1711519822.git.alison.schofield@intel.com/
-- Alison
On Wed, Mar 27, 2024 at 12:48:12PM -0700, Alison Schofield wrote:
> On Thu, Mar 14, 2024 at 08:35:01PM -0700, Dan Williams wrote:
> > Alison Schofield wrote:
> > > On Fri, Mar 15, 2024 at 10:09:44AM +0900, Wonjae Lee wrote:
> > > > alison.schofield@intel.com wrote:
> > > > > From: Alison Schofield <alison.schofield@intel.com>
> > > > >
> > > > > The --media-errors option to 'cxl list' retrieves poison lists from
> > > > > memory devices supporting the capability and displays the returned
> > > > > media_error records in the cxl list json. This option can apply to
> > > > > memdevs or regions.
> > > > >
> > > > > Include media-errors in the -vvv verbose option.
> > > > >
> > > > > Example usage in the Documentation/cxl/cxl-list.txt update.
> > > > >
> > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > > > ---
> > > > > Documentation/cxl/cxl-list.txt 62 +++++++++++++++++++++++++++++++++-
> > > > > cxl/filter.h 3 ++
> > > > > cxl/list.c 3 ++
> > > > > 3 files changed, 67 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> > > > > index 838de4086678..6d3ef92c29e8 100644
> > > > > --- a/Documentation/cxl/cxl-list.txt
> > > > > +++ b/Documentation/cxl/cxl-list.txt
> > > >
> > > > [snip]
> > > >
> > > > +----
> > > > +In the above example, region mappings can be found using:
> > > > +"cxl list -p mem9 --decoders"
> > > > +----
> > > >
> > > > Hi, isn't it '-m mem9' instead of -p? FYI, it's also on patch's
> > > > cover letter, too.
> > >
> > > Thanks for the review! I went with -p because it gives only
> > > the endpoint decoder while -m gives all the decoders up to
> > > the root - more than needed to discover the region.
> >
> > The first thing that comes to mind to list memory devices with their
> > decoders is:
> >
> > cxl list -MD -d endpoint
> >
> > ...however the problem is that endpoint ports connect memdevs to their
> > parent port, so the above results in:
> >
> > Warning: no matching devices found
> >
> > I think I want to special case "-d endpoint" when both -M and -D are
> > specified to also imply -E, "endpoint ports". However that also seems to
> > have a bug at present:
> >
> > # cxl list -EDM -d endpoint -iu
> > {
> > "endpoint":"endpoint2",
> > "host":"mem0",
> > "parent_dport":"0000:34:00.0",
> > "depth":2
> > }
> >
> > That needs to be fixed up to merge:
>
> What's to fix up? Doesn't filtering by '-d endpoint' exclude the
> objects you specified in -EDM. It becomes the equivalent of
> of 'cxl list -E'
>
> >
> > # cxl list -ED -d endpoint -iu
> > {
> > "endpoint":"endpoint2",
> > "host":"mem0",
> > "parent_dport":"0000:34:00.0",
> > "depth":2,
> > "decoders:endpoint2":[
> > {
> > "decoder":"decoder2.0",
> > "interleave_ways":1,
> > "state":"disabled"
> > }
> > ]
> > }
> >
> > ...and:
> >
> > # cxl list -EMu
> > {
> > "endpoint":"endpoint2",
> > "host":"mem0",
> > "parent_dport":"0000:34:00.0",
> > "depth":2,
> > "memdev":{
> > "memdev":"mem0",
> > "pmem_size":"512.00 MiB (536.87 MB)",
> > "serial":"0",
> > "host":"0000:35:00.0"
> > }
> > }
> >
>
> Some of the examples above that use "-d endpoint", filtering on endpoint
> decoders, and so are, by design, excluding memdev info. Filtering on
> endpoint ports, ie -p endpoint, supports a listing of the endpoint
> memdevs and decoders.
>
> > ...so that one can get a nice listing of just endpoint ports, their
> > decoders (with media errors) and their memdevs.
> >
>
> Dissecting the above sentence:
> "of just endpoint ports" --> -p endpoint
> "their decoders" --> -DE
> "their memdevs" --> -M
> "(with media errors)" --media-errors
>
> Yields this query:
> cxl list -p endpoint -DEM --media-errors
>
> You wrote (with media errors) after 'decoders' and that is of concern,
> but maybe just a typo? ATM --media-errors applies to memdev or region
> objects, not to decoder objects.
>
> > The reason that "cxl list -p mem9 -D" works is subtle because it filters
> > the endpoint decoders by an endpoint port filter, but I think most users
> > would expect to not need to enable endpoint-port listings to see their
> > decoders the natural key to filter endpoint decoders is by memdev.
>
> Not following this subtle comment. I find it to be an exacting filter
> targeting exactly a memdev that may be of interest and supplying
> the decoder and region mappings. It would be best suggested in one
> step, and that's is an update in the v12 man page:
> cxl list -p mem9 -DEM --media-errors
>
> I don't understand the desire to use endpoint decoders as a filter when
> using endpoint ports which have memdevs and endpoint decoders as
> children works, and flows with the whole top down cxl list filtering
> design. I also don't see a need to special case, and 'imply' endpoint
> ports, when use can explicitly add -p endpoint to their query.
> (the special case seems like it would add confusion to the cxl list
> usage)
>
> I'm following this w a v12 that does update the man page suggestions.
> Let's continue this conversation there.
>
> Thanks,
> Alison
>
>
>
>
>
next prev parent reply other threads:[~2024-04-18 20:12 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-14 4:05 [ndctl PATCH v11 0/7] Support poison list retrieval alison.schofield
2024-03-14 4:05 ` [ndctl PATCH v11 1/7] libcxl: add interfaces for GET_POISON_LIST mailbox commands alison.schofield
2024-03-18 17:51 ` fan
2024-03-18 20:11 ` Alison Schofield
2024-03-18 21:01 ` Dan Williams
2024-03-19 16:43 ` Alison Schofield
2024-03-14 4:05 ` [ndctl PATCH v11 2/7] cxl/event_trace: add an optional pid check to event parsing alison.schofield
2024-03-14 4:05 ` [ndctl PATCH v11 3/7] cxl/event_trace: support poison context in " alison.schofield
2024-03-14 4:05 ` [ndctl PATCH v11 4/7] cxl/event_trace: add helpers to retrieve tep fields by type alison.schofield
2024-03-15 15:44 ` Dave Jiang
2024-03-15 17:39 ` Dan Williams
2024-03-18 17:28 ` Alison Schofield
2024-03-18 21:21 ` fan
2024-03-14 4:05 ` [ndctl PATCH v11 5/7] cxl/list: collect and parse media_error records alison.schofield
2024-03-15 16:16 ` Dave Jiang
2024-03-20 20:24 ` Alison Schofield
2024-03-14 4:05 ` [ndctl PATCH v11 6/7] cxl/list: add --media-errors option to cxl list alison.schofield
2024-03-15 16:41 ` Dave Jiang
2024-03-14 4:05 ` [ndctl PATCH v11 7/7] cxl/test: add cxl-poison.sh unit test alison.schofield
2024-03-15 17:03 ` Dave Jiang
[not found] ` <CGME20240314040548epcas2p3698bf9d1463a1d2255dc95ac506d3ae8@epcms2p4>
2024-03-15 1:09 ` [ndctl PATCH v11 6/7] cxl/list: add --media-errors option to cxl list Wonjae Lee
2024-03-15 2:36 ` Alison Schofield
2024-03-15 3:35 ` Dan Williams
2024-03-20 20:40 ` Alison Schofield
2024-03-27 19:48 ` Alison Schofield
2024-04-18 20:12 ` Alison Schofield [this message]
[not found] ` <CGME20240314040551epcas2p40829b16b09f439519a692070fb460242@epcms2p1>
2024-03-15 23:03 ` [ndctl PATCH v11 7/7] cxl/test: add cxl-poison.sh unit test Wonjae Lee
2024-03-18 17:17 ` Alison Schofield
2024-03-20 20:42 ` [ndctl PATCH v11 0/7] Support poison list retrieval Alison Schofield
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZiF+t0sVPOdE0X1K@aschofie-mobl2 \
--to=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=hj96.nam@samsung.com \
--cc=linux-cxl@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
--cc=vishal.l.verma@intel.com \
--cc=wj28.lee@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).