Re: [PATCH 2/3] cxl/region: Add sysfs attribute for locality attributes of CXL regions

[Dave Jiang <dave.jiang@intel.com>]

On 12/12/23 06:46, Brice Goglin wrote:
> Le 08/12/2023 à 00:31, Dave Jiang a écrit :
> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index d879f5702cf2..72c47f624d63 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -645,6 +645,26 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>>   }
>>   static DEVICE_ATTR_RW(size);
>>   +#define ACCESS_ATTR(attrib)                    \
>> +static ssize_t attrib##_show(struct device *dev,        \
>> +               struct device_attribute *attr,    \
>> +               char *buf)                \
>> +{                                \
>> +    struct cxl_region *cxlr = to_cxl_region(dev);        \
>> +                                \
>> +    if (!cxlr->coord)                    \
>> +        return 0;                    \
>> +                                \
>> +    return sysfs_emit(buf, "%u\n",                \
>> +              cxlr->coord->attrib);            \
>> +}                                \
> 
> Hello
> 
> Latencies ares off by a factor of 1000 here (I see 586/686 for r/w attributes
> for NUMA nodes in Qemu but 1000x higher for region attributes). For NUMA node attributes,
> you're dividing latencies by a factor a 1000 in cxl_region_perf_attrs_callback():
>  
>         /* Adjust latencies from psec to nsec to be consistent with HMAT targets */
>         coord = *cxlr->coord;
>         coord.read_latency = DIV_ROUND_UP(coord.read_latency, 1000);
>         coord.write_latency = DIV_ROUND_UP(coord.write_latency, 1000);
> 
> For region attributes, I think you're missing the same?

Yes.... I was keeping the original computed raw data in picosecond base. Maybe I should just convert it to nanoseconds here since that's what Linux uses to begin with just to be consistent everywhere. 
> 
> Brice
>