Date:   Thu, 6 Apr 2023 17:33:49 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     Liang Kan <kan.liang@...ux.intel.com>, <linux-cxl@...r.kernel.org>,
        <peterz@...radead.org>, <mingo@...hat.com>, <acme@...nel.org>,
        <mark.rutland@....com>, <will@...nel.org>, <linuxarm@...wei.com>,
        <linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Dave Jiang <dave.jiang@...el.com>
Subject: Re: [PATCH v4 5/5] docs: perf: Minimal introduction the the CXL PMU
 device and driver


> 
> > +
> > +    /sys/bus/cxl/device/cpmu<id>
> > +
> > +The associated PMU is registered as
> > +
> > +   /sys/bus/event_sources/devices/cpmu<id>
> > +
> > +In common with other CXL bus devices, the id has no specific meaning and the
> > +relationship to specific CXL device should be established via the device parent
> > +of the device on the CXL bus.  
> 
> So I went to go add some text about how to identify PMUs in a persistent
> manner from one boot to the next. For CXL memdevs this is done by the
> 'serial' attribute which is always stable regardless of the device init
> order. That's harder to get to from the pmu device because it may be
> associated with a device that does not have a memdev.
> 
> I think it's also going to be frustrating for userspace to see
> randomized pmu ids across devices since that probing will happen in
> parallel. So how about:

Solving this in general for perf PMU drivers was what the parent device thing
was about.  There is an argument that enabling any other path to get to
this association is both unnecessary and just possibly unwise.

The nice advantage of just using an IDA and relying on parentage for the
association was that I could avoid naming questions for all the other
places these might turn in a CXL topology. The Lazy / efficient option ;)

You can now see exactly which PCI device a given instance is associated with.
Custom ABI is going to be harder for anyone to use than that.

I suppose we can potentially enable both paths - but it's not quite
as straight forwards as you suggest.

> 
> 1/ Add serial as an attribute for each PMU to export

Where does it come from? We only have one source of serial number per device.
That's no where near enough to work out where a PMU is. 

> 2/ Change the device name format to be "pmuX.Y" where X can just reuse

Could use something a little more detailed cxl bus, but the one registered and use
to address this in bus/event_sources needs to be cxl specific so a cxl_ prefix
is needed I think

Given we need to namespace what the ids refer to, I'm currently going with
pmu_memX.Y pmu_dspX.Y.Z pmu_uspX.Y
on the cxl bus and
cxl_pmu_memX.Y cxl_pmu_dspX.Y.Z cxl_pmu_uspX.Y on even sources bus.
(Z needed because dsp index from 0 for each usp)
We can figure out what to do about other CXL EPs later and for now at least
there is no way to hand a CPMU instance off a host bridge (nothing in CEDT
to tell you where to find it).

I've had a fun day hacking PMUs onto the other emulated CXL devices to test
this. 
There is a can of worms I'll avoid for this series by just sticking to type3
device PMUs for now.

  I have no idea yet how we handle the interrupts safely for ports as those
  interrupts are in control the pcie port driver not the CXL dport one.
  At somepoint I'll send out an RFC about that if no one gets to it before
  me.  For now I've hacked portdrv to always allocate max vectors and am ignoring the
  lovely back traces due to thing getting torn down in the wrong order on shutdown.
  For upstream ports I've hacked portdrv to pretend it knows there is something to handle.
  As a starting point I think we'll need to teach portdrv enough about CXL
  to be able to tell if it should provide interrupt services..

Hence I'll keep the code to register the other PMUs for a future patch set
and just make sure the code is structured to enable that in this series.


> the memdev id for endpoints and be another value for switches, and Y is
> guaranteed to be 0-based and in hardware discovery order.

Also need to change registration order as PMUs were registered before the
memdev, but that's easy enough to do.

> 
> ...with that, someone can write a udev script that can persistently
> identify PMU[Y] on device[serial] each boot.

> 
> That also cleans up a /sys/bus/cxl/devices listing to make it clear
> which pmu instances belong together.
>  
> > +
> > +PMU driver provides description of available events and filter options in sysfs.
> > +
> > +The "format" directory describes all formats of the config (event vendor id,
> > +group id and mask) config1 (threshold, filter enables) and config2 (filter
> > +parameters) fields of the perf_event_attr structure.  The "events" directory
> > +describes all documented events show in perf list.
> > +
> > +The events shown in perf list are the most fine grained events with a single
> > +bit of the event mask set. More general events may be enable by setting
> > +multiple mask bits in config. For example, all Device to Host Read Requests
> > +may be captured on a single counter by setting the bits for all of
> > +
> > +* d2h_req_rdcurr
> > +* d2h_req_rdown
> > +* d2h_req_rdshared
> > +* d2h_req_rdany
> > +* d2h_req_rdownnodata
> > +
> > +Example of usage::
> > +
> > +  $#perf list
> > +  cpmu0/clock_ticks/                                 [Kernel PMU event]
> > +  cpmu0/d2h_req_itomwr/                              [Kernel PMU event]
> > +  cpmu0/d2h_req_rdany/                               [Kernel PMU event]
> > +  cpmu0/d2h_req_rdcurr/                              [Kernel PMU event]
> > +  -----------------------------------------------------------
> > +
> > +  $# perf stat -e cpmu0/clock_ticks/ -e cpmu0/d2h_req_itowrm/  
> 
> Ah here's the examples I was looking for in the last patch, nice.
> 
> > +
> > +Vendor specific events may also be available and if so can be used via
> > +
> > +  $# perf stat -e cpmu0/vid=VID,gid=GID,mask=MASK/
> > +
> > +The driver does not support sampling. So "perf record" and attaching to
> > +a task are unsupported.  
> 
> Is this a common restriction for CPU-external pmus, or do you see
> sampling support required to get this upstream?

It's a common restriction. Whilst we could potentially implement sampling
based on the presence of a suitable clock_ticks event it don't see it
as a requirement initially.

Jonathan

Powered by blists - more mailing lists