Re: [PATCH v7 3/5] cxl: Fix incorrect region perf data calculation

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 5 Apr 2024 15:34:25 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Dave Jiang wrote:
> > Current math in cxl_region_perf_data_calculate divides the latency by 1000
> > every time the function gets called. This causes the region latency to be
> > divided by 1000 per memory device and the math is incorrect. This is user
> > visible as the latency access_coordinate exposed via sysfs will show
> > incorrect latency data.
> > 
> > Normalize values from CDAT to nanoseconds. Adjust sub-nanoseconds latency
> > to at least 1. Remove adjustment of perf numbers from the generic target
> > since hmat handling code has already normalized those numbers. Now all
> > computation and stored numbers should be in nanoseconds.
> > 
> > cxl_hb_get_perf_coordinates() is removed and HB coords are calculated
> > in the port access_coordinate calculation path since it no longer need
> > to be treated special.
> > 
> > Fixes: 3d9f4a197230 ("cxl/region: Calculate performance data for a region")
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> > v7:
> > - Remove min_not_zero(). Incorrectly set everything to 1. DIV_ROUNDUP()
> >   will ensure sub-nanoseconds values not 0 unless value 0 to begin with.
> >   (Jonathan)
> > - Reflowed patch order
> > - Remove cxl_hb_get_perf_coordinates() as change made function unnessary.
> > - Add hb access_coordinate back to port caclculation.
> > ---
> >  drivers/cxl/acpi.c      | 13 +-----
> >  drivers/cxl/core/cdat.c | 89 ++++++++++++++++-------------------------
> >  drivers/cxl/core/port.c | 36 ++---------------
> >  drivers/cxl/cxl.h       |  2 -
> >  4 files changed, 40 insertions(+), 100 deletions(-)  
> 
> yum... bug fix that removes more than double the code it adds.
> 
> [..]
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index eddbbe21450c..48704976693e 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -20,6 +20,31 @@ struct dsmas_entry {
> >  	int qos_class;
> >  };
> >  
> > +static u32 cdat_normalize(u16 entry, u64 base, u8 type)
> > +{
> > +	u32 value;
> > +
> > +	/*
> > +	 * Check for invalid and overflow values
> > +	 */
> > +	if (entry == 0xffff || !entry)
> > +		return 0;
> > +	else if (base > (UINT_MAX / (entry)))
> > +		return 0;
> > +
> > +	value = entry * base;  
> 
> Might be worth a reminder comment here that CDAT fields follow the
> format of HMAT fields when a future reader wonders why these type names
> are not CDAT_ACCESS_LATENCY, etc. Bonus points for a "see Table 5
> Device Scoped Latency and Bandwidth Information Structure in Coherent
> Device Attribute Table (CDAT) Specification v1.01"

Call out a specific HMAT version if you do add such a comment.
We don't want anyone to happen to look at an early version and get
even more confused :(


> 
> Either way you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>