[ndctl,0/2] cxl-list: Construct CXL topology graph images

Message

Fan Ni Dec. 20, 2022, 6:26 p.m. UTC

This patch series extends the `cxl list` subcommand to show the cxl
topology visually. Mattew Ho first worked on the code and provided an
initial patch as list below[1].

This patch series includes the following two patches,
1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
show which downstream port a component is attached. This attribute will be used
in patch 2 to generate the cxl topology graph.
2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
json format file or generate a graph showing the cxl topology. To use the
extended function, the option `-o output.suffix` is added. Acceptable output
suffixes include .jpeg, .jpg and .png for generating a graph and for other
suffix, it will dump the json-formatted cxl topology to the file, which
can be used the input file (with --input option) to generate the graph
later.

Patch 2 reuses the plotting functions in Matthew Ho's patch, which are updated
to work with parent_dport attribute in patch 1. Also, some bugs are
fixed. More detailed changes are listed in Patch 2's commit log.

The patch series is tested with the following different cxl topologies,
1) a single memdev attached the only root port of the single HB in the system;
2) two memdevs attached to the two root ports of the single HB in the system;
3) four memdevs attached to two HBs, each of which has two root ports;
4) four memdevs attached to the downstream ports of a cxl switch which is
attached to one of the two root ports of a HB in the system.


[1] Mattew Ho's patch:
https://lore.kernel.org/linux-cxl/[e-mail]/





Fan Ni (2):
  cxl-list: add `parent_dport` to cxl_port/memdev objects
  cxl-list: Construct CXL topology graph images

 Documentation/cxl/cxl-list.txt |  16 ++
 cxl/filter.c                   |  45 +++-
 cxl/filter.h                   |   5 +
 cxl/graph.c                    | 409 +++++++++++++++++++++++++++++++++
 cxl/graph.h                    |  20 ++
 cxl/json.c                     |  11 +
 cxl/lib/libcxl.c               |  43 ++++
 cxl/lib/libcxl.sym             |   6 +
 cxl/lib/private.h              |   1 +
 cxl/libcxl.h                   |   3 +
 cxl/list.c                     |  31 +++
 cxl/meson.build                |   2 +
 meson.build                    |   1 +
 util/json.c                    |   2 +-
 14 files changed, 590 insertions(+), 5 deletions(-)
 create mode 100644 cxl/graph.c
 create mode 100644 cxl/graph.h

Comments

Dan Williams Jan. 3, 2023, 7:05 p.m. UTC | #1

Fan Ni wrote:
> This patch series extends the `cxl list` subcommand to show the cxl
> topology visually. Mattew Ho first worked on the code and provided an
> initial patch as list below[1].

Thanks for picking this up, it looks like a useful addition to me.

> 
> This patch series includes the following two patches,
> 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> show which downstream port a component is attached. This attribute will be used
> in patch 2 to generate the cxl topology graph.

I had a similar patch to do this here:

http://lore.kernel.org/r/[e-mail]

The biggest difference I see is that your version adds 'parent_dport' to
memdevs where mine keeps it purely an 'port' attribute. So the listing
needs to have --endpoints to get the memdev to 'parent_dport'
assocation.

> 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> json format file or generate a graph showing the cxl topology. To use the
> extended function, the option `-o output.suffix` is added. Acceptable output
> suffixes include .jpeg, .jpg and .png for generating a graph and for other
> suffix, it will dump the json-formatted cxl topology to the file, which
> can be used the input file (with --input option) to generate the graph
> later.

I notice that the implementation enforces some implicit listing options,
disables --endpoints, and does some magic with looking at the suffix to
know what to write to the output file. That feels too restrictive and
makes the 'list' command a bit more difficult to reason about. How about
decoupling the support from 'list'? I am thinking of a scheme like this:

   cxl list -vvv | cxl graph -o png > cxl-topo.png

This has a few advantages:
- The graphviz build dependency can be made optional where the user can
  detect if the support is present in the build by the availability of
  the 'graph' subcommand.
- Listings can be created on one system and graphed on another which
  among other usages is useful for debugging topologies over email.
- The 'graph' subcommand can parse and optionally warn about the input
  json rather than implicit forcing of listing options.
- The json can go through additional filtering before being graphed:

    cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out

Thoughts?

Fan Ni Jan. 3, 2023, 9:43 p.m. UTC | #2

On Tue, Jan 03, 2023 at 11:05:22AM -0800, Dan Williams wrote:



> Fan Ni wrote:
> > This patch series extends the `cxl list` subcommand to show the cxl
> > topology visually. Mattew Ho first worked on the code and provided an
> > initial patch as list below[1].
> 
> Thanks for picking this up, it looks like a useful addition to me.
> 
> > 
> > This patch series includes the following two patches,
> > 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> > show which downstream port a component is attached. This attribute will be used
> > in patch 2 to generate the cxl topology graph.
> 
> I had a similar patch to do this here:
> 
> https://urldefense.com/v3/__http://lore.kernel.org/r/[e-mail]__;!!EwVzqGoTKBqv-0DWAJBm!Xc_tWtX0GBqionoq7JRQxQ39MytJEW3jP-JcQnLrj7Op9_u5mMRg_hUoUvQabHNT_IXi3MbHF2d0TwCsiZckhvCToA$ 
> 
> The biggest difference I see is that your version adds 'parent_dport' to
> memdevs where mine keeps it purely an 'port' attribute. So the listing
> needs to have --endpoints to get the memdev to 'parent_dport'
> assocation.

Thanks for mentioning this, I missed the patch. It seems we can use this patch
to prepare the info for plotting the graph. Need to check more.

> 
> > 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> > json format file or generate a graph showing the cxl topology. To use the
> > extended function, the option `-o output.suffix` is added. Acceptable output
> > suffixes include .jpeg, .jpg and .png for generating a graph and for other
> > suffix, it will dump the json-formatted cxl topology to the file, which
> > can be used the input file (with --input option) to generate the graph
> > later.
> 
> I notice that the implementation enforces some implicit listing options,
> disables --endpoints, and does some magic with looking at the suffix to
> know what to write to the output file. That feels too restrictive and
> makes the 'list' command a bit more difficult to reason about. How about

Using file extention to decide whether to dump the output to a json-formatted
file or plot a cxl graph was suggested by Vishal in his previous
comments to Mattew's patch (
https://lore.kernel.org/all/[e-mail]/
). We are neutral to either way as long as there is a consensus.

About the enforcements placed on "cxl list" command option, it should be easy to
address. We should be able to remove them and fix in graph functions.

> decoupling the support from 'list'? I am thinking of a scheme like this:
> 
>    cxl list -vvv | cxl graph -o png > cxl-topo.png
> 

Currently, we can do similar things as you mentioned above.
cxl list -vvv > cxl.json (with some changes to remove the option enforcement.)
cxl list -input cxl.json -o cxl-topo.png

But I agree that using a separate subcommand may be a better idea as the
advantage you mentioned below.

> This has a few advantages:
> - The graphviz build dependency can be made optional where the user can
>   detect if the support is present in the build by the availability of
>   the 'graph' subcommand.

IMO, this is the main advantage to introduce a new subcommand.

> - Listings can be created on one system and graphed on another which
>   among other usages is useful for debugging topologies over email.

Our current approach can achieve similar goal with "-input" option.

> - The 'graph' subcommand can parse and optionally warn about the input
>   json rather than implicit forcing of listing options.

Agree.

> - The json can go through additional filtering before being graphed:
> 
>     cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out
> 
> Thoughts?

Thanks for the comments. So based on the above comments and my
understanding, I think we need to decide two things before acting
further.
1. whether we want to use suffix to determine plot behaviour or use
explicit arguments.
2. Whether we want to introduce an extra subcommand for the graph
function or stick to existing approach and make changes as needed to
achieve similar goal.

Verma, Vishal L Jan. 4, 2023, 6:59 a.m. UTC | #3

On Tue, 2023-01-03 at 21:43 +0000, Fan Ni wrote:
> On Tue, Jan 03, 2023 at 11:05:22AM -0800, Dan Williams wrote:
> > Fan Ni wrote:
> > > This patch series extends the `cxl list` subcommand to show the cxl
> > > topology visually. Mattew Ho first worked on the code and provided an
> > > initial patch as list below[1].
> > 
> > Thanks for picking this up, it looks like a useful addition to me.
> > 
> > > 
> > > This patch series includes the following two patches,
> > > 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> > > show which downstream port a component is attached. This attribute will be used
> > > in patch 2 to generate the cxl topology graph.
> > 
> > I had a similar patch to do this here:
> > 
> > https://urldefense.com/v3/__http://lore.kernel.org/r/[e-mail]__;!!EwVzqGoTKBqv-0DWAJBm!Xc_tWtX0GBqionoq7JRQxQ39MytJEW3jP-JcQnLrj7Op9_u5mMRg_hUoUvQabHNT_IXi3MbHF2d0TwCsiZckhvCToA$ 
> > 
> > The biggest difference I see is that your version adds 'parent_dport' to
> > memdevs where mine keeps it purely an 'port' attribute. So the listing
> > needs to have --endpoints to get the memdev to 'parent_dport'
> > assocation.
> 
> Thanks for mentioning this, I missed the patch. It seems we can use this patch
> to prepare the info for plotting the graph. Need to check more.
> 
> > 
> > > 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> > > json format file or generate a graph showing the cxl topology. To use the
> > > extended function, the option `-o output.suffix` is added. Acceptable output
> > > suffixes include .jpeg, .jpg and .png for generating a graph and for other
> > > suffix, it will dump the json-formatted cxl topology to the file, which
> > > can be used the input file (with --input option) to generate the graph
> > > later.
> > 
> > I notice that the implementation enforces some implicit listing options,
> > disables --endpoints, and does some magic with looking at the suffix to
> > know what to write to the output file. That feels too restrictive and
> > makes the 'list' command a bit more difficult to reason about. How about
> 
> Using file extention to decide whether to dump the output to a json-formatted
> file or plot a cxl graph was suggested by Vishal in his previous
> comments to Mattew's patch (
> https://lore.kernel.org/all/[e-mail]/
> ). We are neutral to either way as long as there is a consensus.
> 
> About the enforcements placed on "cxl list" command option, it should be easy to
> address. We should be able to remove them and fix in graph functions.
> 
> > decoupling the support from 'list'? I am thinking of a scheme like this:
> > 
> >    cxl list -vvv | cxl graph -o png > cxl-topo.png
> > 
> 
> Currently, we can do similar things as you mentioned above.
> cxl list -vvv > cxl.json (with some changes to remove the option enforcement.)
> cxl list -input cxl.json -o cxl-topo.png
> 
> But I agree that using a separate subcommand may be a better idea as the
> advantage you mentioned below.
> 
> > This has a few advantages:
> > - The graphviz build dependency can be made optional where the user can
> >   detect if the support is present in the build by the availability of
> >   the 'graph' subcommand.
> 
> IMO, this is the main advantage to introduce a new subcommand.
> 
> > - Listings can be created on one system and graphed on another which
> >   among other usages is useful for debugging topologies over email.
> 
> Our current approach can achieve similar goal with "-input" option.
> 
> > - The 'graph' subcommand can parse and optionally warn about the input
> >   json rather than implicit forcing of listing options.
> 
> Agree.
> 
> > - The json can go through additional filtering before being graphed:
> > 
> >     cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out
> > 
> > Thoughts?
> 
> Thanks for the comments. So based on the above comments and my
> understanding, I think we need to decide two things before acting
> further.

> 1. whether we want to use suffix to determine plot behaviour or use
> explicit arguments.

I think I was taking inspiration from pandoc when I suggested using
file extensions to deduce the output / input formats. However, I think
it can be valuable to have an explicit override too.

So maybe something like:
  -o topo.png : outputs as a png (deduced from extension)
  -o topo -f png : also outputs as a png
                   (specified explicitly by -f / --format)
  -o topo.png -f jpg : outputs as a jpg - i.e. -f overrides extension

> 2. Whether we want to introduce an extra subcommand for the graph
> function or stick to existing approach and make changes as needed to
> achieve similar goal.

I think the flexibility and decoupling gained from splitting this out
to a new command is quite valuable. Especially considering the ability
to pipe through jq filters. Thanks for this suggestion Dan!

Dan Williams Jan. 4, 2023, 8:34 a.m. UTC | #4

Verma, Vishal L wrote:
> On Tue, 2023-01-03 at 21:43 +0000, Fan Ni wrote:
> > On Tue, Jan 03, 2023 at 11:05:22AM -0800, Dan Williams wrote:
> > > Fan Ni wrote:
> > > > This patch series extends the `cxl list` subcommand to show the cxl
> > > > topology visually. Mattew Ho first worked on the code and provided an
> > > > initial patch as list below[1].
> > > 
> > > Thanks for picking this up, it looks like a useful addition to me.
> > > 
> > > > 
> > > > This patch series includes the following two patches,
> > > > 1) Patch 1 adds a parent_dport attribute to ports and type 3 memory devices to
> > > > show which downstream port a component is attached. This attribute will be used
> > > > in patch 2 to generate the cxl topology graph.
> > > 
> > > I had a similar patch to do this here:
> > > 
> > > https://urldefense.com/v3/__http://lore.kernel.org/r/[e-mail]__;!!EwVzqGoTKBqv-0DWAJBm!Xc_tWtX0GBqionoq7JRQxQ39MytJEW3jP-JcQnLrj7Op9_u5mMRg_hUoUvQabHNT_IXi3MbHF2d0TwCsiZckhvCToA$ 
> > > 
> > > The biggest difference I see is that your version adds 'parent_dport' to
> > > memdevs where mine keeps it purely an 'port' attribute. So the listing
> > > needs to have --endpoints to get the memdev to 'parent_dport'
> > > assocation.
> > 
> > Thanks for mentioning this, I missed the patch. It seems we can use this patch
> > to prepare the info for plotting the graph. Need to check more.
> > 
> > > 
> > > > 2) Patch 2 extends the `cxl list` subcommand to dump the cxl topology to a
> > > > json format file or generate a graph showing the cxl topology. To use the
> > > > extended function, the option `-o output.suffix` is added. Acceptable output
> > > > suffixes include .jpeg, .jpg and .png for generating a graph and for other
> > > > suffix, it will dump the json-formatted cxl topology to the file, which
> > > > can be used the input file (with --input option) to generate the graph
> > > > later.
> > > 
> > > I notice that the implementation enforces some implicit listing options,
> > > disables --endpoints, and does some magic with looking at the suffix to
> > > know what to write to the output file. That feels too restrictive and
> > > makes the 'list' command a bit more difficult to reason about. How about
> > 
> > Using file extention to decide whether to dump the output to a json-formatted
> > file or plot a cxl graph was suggested by Vishal in his previous
> > comments to Mattew's patch (
> > https://lore.kernel.org/all/[e-mail]/
> > ). We are neutral to either way as long as there is a consensus.
> > 
> > About the enforcements placed on "cxl list" command option, it should be easy to
> > address. We should be able to remove them and fix in graph functions.
> > 
> > > decoupling the support from 'list'? I am thinking of a scheme like this:
> > > 
> > >    cxl list -vvv | cxl graph -o png > cxl-topo.png
> > > 
> > 
> > Currently, we can do similar things as you mentioned above.
> > cxl list -vvv > cxl.json (with some changes to remove the option enforcement.)
> > cxl list -input cxl.json -o cxl-topo.png
> > 
> > But I agree that using a separate subcommand may be a better idea as the
> > advantage you mentioned below.
> > 
> > > This has a few advantages:
> > > - The graphviz build dependency can be made optional where the user can
> > >   detect if the support is present in the build by the availability of
> > >   the 'graph' subcommand.
> > 
> > IMO, this is the main advantage to introduce a new subcommand.
> > 
> > > - Listings can be created on one system and graphed on another which
> > >   among other usages is useful for debugging topologies over email.
> > 
> > Our current approach can achieve similar goal with "-input" option.
> > 
> > > - The 'graph' subcommand can parse and optionally warn about the input
> > >   json rather than implicit forcing of listing options.
> > 
> > Agree.
> > 
> > > - The json can go through additional filtering before being graphed:
> > > 
> > >     cxl list -vvv $basic_filters | jq $advanced_filters | cxl graph -o png > out
> > > 
> > > Thoughts?
> > 
> > Thanks for the comments. So based on the above comments and my
> > understanding, I think we need to decide two things before acting
> > further.
> 
> > 1. whether we want to use suffix to determine plot behaviour or use
> > explicit arguments.
> 
> I think I was taking inspiration from pandoc when I suggested using
> file extensions to deduce the output / input formats. However, I think
> it can be valuable to have an explicit override too.
> 
> So maybe something like:
>   -o topo.png : outputs as a png (deduced from extension)
>   -o topo -f png : also outputs as a png
>                    (specified explicitly by -f / --format)

Yeah, that seems fine. I was more worried about the implications of 'cxl
list' filter options being silently changed just because someone changed
the output filename from $f.txt to $f.jpg.

>   -o topo.png -f jpg : outputs as a jpg - i.e. -f overrides extension

I think -f can be saved for later.

> > 2. Whether we want to introduce an extra subcommand for the graph
> > function or stick to existing approach and make changes as needed to
> > achieve similar goal.
> 
> I think the flexibility and decoupling gained from splitting this out
> to a new command is quite valuable. Especially considering the ability
> to pipe through jq filters. Thanks for this suggestion Dan!
> 

Yeah, the separation of concerns feels more unix-y.

Davidlohr Bueso Jan. 4, 2023, 4:21 p.m. UTC | #5

On Wed, 04 Jan 2023, Verma, Vishal L wrote:

>So maybe something like:
>  -o topo.png : outputs as a png (deduced from extension)
>  -o topo -f png : also outputs as a png
>                   (specified explicitly by -f / --format)
>  -o topo.png -f jpg : outputs as a jpg - i.e. -f overrides extension

Makes sense to me.