| Message ID | 20230330164556.31533-1-Jonathan.Cameron@huawei.com |
|---|---|
| Headers | show |
| Series | CXL 3.0 Performance Monitoring Unit support | expand |
Jonathan Cameron wrote: > Changes since v3: Kan Liang gave feedback on v2 incorporated here. > - All updates in patch 4, see details there. > > Updated cover letter. > > The CXL rev 3.0 specification introduces a CXL Performance Monitoring > Unit definition. CXL components may have any number of these blocks. The > definition is highly flexible, but that does bring complexity in the > driver. > > Notes. > > 1) The QEMU model against which this was developed needs tidying up. > Latest tree at https://gitlab.com/jic23/qemu branch cxl-2023-02-28 > It's backed up behind other series that I plan to upstream first. > 2) There are quite a lot of corner cases that will need working through > with variants of the model, or I'll have to design a pathological > set of CPMUs to hit all the corner cases in one go. So whilst I believe > the driver should be fine for what it supports we may find issues > as those corners of what is allowed are explored. > 3) I'm not sure it makes sense to hang this of the cxl/pci driver but > couldn't really figure out where else in the current structure we could > make it fit cleanly. > 4) Driver location. In past perf maintainers have requested perf drivers > for PCI etc be under drivers/perf. That would require moving some > CXL headers to be more generally visible, but is certainly possible > if there is agreement between CXL and perf maintainers on the correct > location. I am ok the bulk of the logic living under drivers/perf/ Are drivers/perf/ folks ok with a: CFLAGS_cxl.o := -I$(srctree)/drivers/cxl/ ...or similar in their Makefile, because I don't think this by itself is a reason to push CXL headers to include/. I say this without having looked at the code yet and whether this driver will need new exports from the cxl/core etc. > 5) Patch 1 led to a discussion of how to handle the self describing > and extensible nature of the CPMU counters. That is likely to involve > a description in the "caps" sysfs directory and perf tool code that is > aware of the different event combinations that make sense and can > establish which sets are available on a given device. > That is likely to take a while to converge on, so as the driver is useful > in the current state, I'm looking to upstream this first and deal with > the more complex handling later. What's "this" in this last sentence, a canned set of base counters? > 6) There is a lot of other functionality that can be added in future > include allowing for simpler hardware implementations that may not > support the minimum level of features currently required by the driver > (freeze, overflow interrupts etc). Curious if the the minimal set determined by the specification, like the minimal features a CXL Memory Expander device must implement, or by what you decided was fit to be emulated in QEMU? > > CXL rev 3.0 specification available from https://www.computeexpresslink.org > > > Jonathan Cameron (5): > cxl: Add function to count regblocks of a given type > perf: Allow a PMU to have a parent > cxl/pci: Find and register CXL PMU devices > cxl: CXL Performance Monitoring Unit driver > docs: perf: Minimal introduction the the CXL PMU device and driver > > Documentation/admin-guide/perf/cxl.rst | 65 ++ > Documentation/admin-guide/perf/index.rst | 1 + > drivers/cxl/Kconfig | 13 + > drivers/cxl/Makefile | 1 + > drivers/cxl/core/Makefile | 1 + > drivers/cxl/core/core.h | 1 + > drivers/cxl/core/cpmu.c | 72 ++ > drivers/cxl/core/pci.c | 2 +- > drivers/cxl/core/port.c | 4 +- > drivers/cxl/core/regs.c | 50 +- > drivers/cxl/cpmu.c | 940 +++++++++++++++++++++++ > drivers/cxl/cpmu.h | 56 ++ > drivers/cxl/cxl.h | 17 +- > drivers/cxl/cxlpci.h | 1 + > drivers/cxl/pci.c | 27 +- > include/linux/perf_event.h | 1 + > kernel/events/core.c | 1 + > 17 files changed, 1245 insertions(+), 8 deletions(-) > create mode 100644 Documentation/admin-guide/perf/cxl.rst > create mode 100644 drivers/cxl/core/cpmu.c > create mode 100644 drivers/cxl/cpmu.c > create mode 100644 drivers/cxl/cpmu.h > > -- > 2.37.2 >
On Mon, 3 Apr 2023 20:55:35 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > Jonathan Cameron wrote: > > Changes since v3: Kan Liang gave feedback on v2 incorporated here. > > - All updates in patch 4, see details there. > > > > Updated cover letter. > > > > The CXL rev 3.0 specification introduces a CXL Performance Monitoring > > Unit definition. CXL components may have any number of these blocks. The > > definition is highly flexible, but that does bring complexity in the > > driver. > > > > Notes. > > > > 1) The QEMU model against which this was developed needs tidying up. > > Latest tree at https://gitlab.com/jic23/qemu branch cxl-2023-02-28 > > It's backed up behind other series that I plan to upstream first. > > 2) There are quite a lot of corner cases that will need working through > > with variants of the model, or I'll have to design a pathological > > set of CPMUs to hit all the corner cases in one go. So whilst I believe > > the driver should be fine for what it supports we may find issues > > as those corners of what is allowed are explored. > > 3) I'm not sure it makes sense to hang this of the cxl/pci driver but > > couldn't really figure out where else in the current structure we could > > make it fit cleanly. > > 4) Driver location. In past perf maintainers have requested perf drivers > > for PCI etc be under drivers/perf. That would require moving some > > CXL headers to be more generally visible, but is certainly possible > > if there is agreement between CXL and perf maintainers on the correct > > location. > > I am ok the bulk of the logic living under drivers/perf/ > > Are drivers/perf/ folks ok with a: > > CFLAGS_cxl.o := -I$(srctree)/drivers/cxl/ > > ...or similar in their Makefile, because I don't think this by itself is > a reason to push CXL headers to include/. For now I've gone with ../cxl/ which doesn't look too bad. If cflags stuff is preferred can change that. > > I say this without having looked at the code yet and whether this driver > will need new exports from the cxl/core etc. > > > 5) Patch 1 led to a discussion of how to handle the self describing > > and extensible nature of the CPMU counters. That is likely to involve > > a description in the "caps" sysfs directory and perf tool code that is > > aware of the different event combinations that make sense and can > > establish which sets are available on a given device. > > That is likely to take a while to converge on, so as the driver is useful > > in the current state, I'm looking to upstream this first and deal with > > the more complex handling later. > > What's "this" in this last sentence, a canned set of base counters? All the counters defined in the CXL spec itself without any summing of counters (so you can't do all 'reads' for example without 'guessing' the magic numbers). > > > 6) There is a lot of other functionality that can be added in future > > include allowing for simpler hardware implementations that may not > > support the minimum level of features currently required by the driver > > (freeze, overflow interrupts etc). > > Curious if the the minimal set determined by the specification, like the > minimal features a CXL Memory Expander device must implement, or by what > you decided was fit to be emulated in QEMU? There is no specification requirement to implement any particular subset of counters or features for those counters. I think that's considered out of scope for the CXL spec itself. Of course particularly customers or industry bodies may define a minimal set of requirements. I don't think anyone has done so yet. Current set of counters is all the ones in the specification itself at their finest granularity (summed cases for the configurable counters 'work' but aren't advertised - working out sensible choices is a job for perftool + an enhanced driver that advertises what combinations it can sum). Counter features / types include the most generic, fully featured and flexible versions. In many cases it's harder to support the less complex hardware because a configuration dance can be needed. I'd imagine the remaining 'likely' hardware support will be added over the next kernel cycle or two. Jonathan > > > > > CXL rev 3.0 specification available from https://www.computeexpresslink.org > > > > > > Jonathan Cameron (5): > > cxl: Add function to count regblocks of a given type > > perf: Allow a PMU to have a parent > > cxl/pci: Find and register CXL PMU devices > > cxl: CXL Performance Monitoring Unit driver > > docs: perf: Minimal introduction the the CXL PMU device and driver > > > > Documentation/admin-guide/perf/cxl.rst | 65 ++ > > Documentation/admin-guide/perf/index.rst | 1 + > > drivers/cxl/Kconfig | 13 + > > drivers/cxl/Makefile | 1 + > > drivers/cxl/core/Makefile | 1 + > > drivers/cxl/core/core.h | 1 + > > drivers/cxl/core/cpmu.c | 72 ++ > > drivers/cxl/core/pci.c | 2 +- > > drivers/cxl/core/port.c | 4 +- > > drivers/cxl/core/regs.c | 50 +- > > drivers/cxl/cpmu.c | 940 +++++++++++++++++++++++ > > drivers/cxl/cpmu.h | 56 ++ > > drivers/cxl/cxl.h | 17 +- > > drivers/cxl/cxlpci.h | 1 + > > drivers/cxl/pci.c | 27 +- > > include/linux/perf_event.h | 1 + > > kernel/events/core.c | 1 + > > 17 files changed, 1245 insertions(+), 8 deletions(-) > > create mode 100644 Documentation/admin-guide/perf/cxl.rst > > create mode 100644 drivers/cxl/core/cpmu.c > > create mode 100644 drivers/cxl/cpmu.c > > create mode 100644 drivers/cxl/cpmu.h > > > > -- > > 2.37.2 > >