Re: [PATCH 3/4] cxl: CXL Performance Monitoring Unit driver

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Liang, Kan" <kan.liang@linux.intel.com>
Cc: <linux-cxl@vger.kernel.org>, <peterz@infradead.org>,
	<mingo@redhat.com>, <acme@kernel.org>, <mark.rutland@arm.com>,
	<will@kernel.org>, <dan.j.williams@intel.com>,
	<bwidawsk@kernel.org>, <ira.weiny@intel.com>,
	<vishal.l.verma@intel.com>, <alison.schofield@intel.com>,
	<linuxarm@huawei.com>, <linux-perf-users@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/4] cxl: CXL Performance Monitoring Unit driver
Date: Tue, 21 Mar 2023 17:46:32 +0000	[thread overview]
Message-ID: <20230321174632.00001c5c@Huawei.com> (raw)
In-Reply-To: <52abbd9a-3a63-3dde-c20f-99787225200e@linux.intel.com>

> >> So each fixed counter has a dedicated event cap reg.
> >> All the configurable counters share the rest of the event cap regs.
> >> Is my understanding correct?  
> > 
> > Yes.
> >   
> >>
> >> To check the event cap, you have to go through the whole list, which
> >> seems not efficient. Have you consisdered to use other structure, e.g.,
> >> rb-tree to store the event capability information.  
> > 
> > Whilst this is a fairly short list for a given CPMU (max 32 elements),
> > there is no harm in having something cleaner. 
> > 
> > However I'm struggling a bit on what that structure should be.
> > There are two forms of indexing used.
> > 1. VID, GID and MASK all match precisely - could use an xarray, but
> >    the index created from these is too large, so I guess an RB tree.
> > 2. VID, GID and subset of MASK (no constraints on mask combinations)
> > 
> > Can't index by just VID/GID as there may well be multiple entries.
> > 
> > Can't index by VID/GID/MASK as whilst that is either unique (or there is
> > duplication we can ignore) there is no obvious way to search for
> > a subset of mask.  Could have a red black tree with lists for the nodes
> > but that's pretty ugly.
> > 
> > Any thoughts on what would work here?  
> 
> If it's a short list, I think it should be OK.
> But I think we may want to split the list into a fixed counter list and
> a configurable counter list. I don't see a reason to mix them in one
> list. It should further reduce the list. I think we may further factor
> out a wrapper e.g., cpmu_events_find_fixed_counter(vid,gid,mask),
> cpmu_events_find_config_counter(vid,gid,mask).

For this aspect, it turned out to be easier to factor out only the finding
of the event as that code rather than going all the way to the counter
(the last part only occurs in one location in current code anyway)

That allowed the same util functions to be used both to establish
visibility of the attrs and here without having to pass a nasty
flag in to say whether we should look for an available counter or not.

Thanks,

Jonathan