Message-ID: <5b2d621f-459f-463e-abc6-9157513f1fee@intel.com>
Date: Tue, 8 Jul 2025 13:49:26 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...el.com>
CC: Fenghua Yu <fenghuay@...dia.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>, "Drew
 Fustini" <dfustini@...libre.com>, Dave Martin <Dave.Martin@....com>, "Anil
 Keshavamurthy" <anil.s.keshavamurthy@...el.com>, Chen Yu
	<yu.c.chen@...el.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v6 00/30] x86,fs/resctrl telemetry monitoring

Hi Tony,

On 7/8/25 12:08 PM, Luck, Tony wrote:
> On Thu, Jul 03, 2025 at 10:22:06AM -0700, Luck, Tony wrote:
>> On Thu, Jul 03, 2025 at 09:45:15AM -0700, Reinette Chatre wrote:
>>> Hi Tony and Dave,
>>>
>>> On 6/26/25 9:49 AM, Tony Luck wrote:
>>>>  --- 14 ---
>>>> Add mon_evt::is_floating_point set by resctrl file system code to limit
>>>> which events architecture code can request be displayed in floating point.
>>>>
>>>> Simplified the fixed-point to floating point algorithm. Reinette is
>>>> correct that the additional "lshift" and "rshift" operations are not
>>>> required. All that is needed is to multiply the fixed point fractional
>>>> part by 10**decimal_places, add a rounding amount equivalent to a "1"
>>>> in the binary place after those supplied. Finally divide by 2**binary_places
>>>> (with a right shift).
>>>>
>>>> Explained in commit comment how I chose the number of decimal places to
>>>> use for each binary places value.
>>>>
>>>> N.B. Dave Martin expressed an opinion that the kernel should not do
>>>> this conversion. Instead it should enumerate the scaling factor for
>>>> each event where hardware reported a fixed point value. This patch
>>>> could be dropped and replaced with one to enumerate scaling factors
>>>> per event if others agree with Dave.
>>>
>>> Could resctrl accommodate both usages? For example, it does not
>>> look too invasive to add a second file <mon_evt::name>.raw for the
>>> mon_evt::is_floating_point events that can output something like Dave
>>> suggested in [1]:
>>>
>>> .raw file format could be:
>>> 	#format:<output that depends on format>
>>> 	#fixed-point:<value>/<scaling factor>
>>>
>>> Example output:
>>> 	fixed-point:0x60000/0x40000
>>
>> Dave: Is that what you want in the ".raw" file? An alternative would be
>> to put the format information for non-integer events into an
>> "info" file ("info/{RESOURCE_NAME}_MON/monfeatures.raw.formats"?)
>> and just put the raw value into the ".raw" file under mon_data.
> 
> Note that I thought it easier for users to keep the raw file to just
> showing a value, rather than including the formatting details in
> Reinette's proposal.

Could you please elaborate what makes this easier? It is not obvious to me
how it is easier for user to open, parse, and close two files rather than one.
(more below)
> 
> Patch to implement my alternative suggestion below. To the user things
> look like this:
> 
> $ cd /sys/fs/resctrl/mon_data/mon_PERF_PKG_01
> $ cat core_energy
> 0.02203
> $ cat core_energy.raw
> 5775
> $ cat /sys/fs/resctrl/info/PERF_PKG_MON/mon_features_raw_scale
> core_energy 262144
> activity 262144
> $ bc -ql
> 5775 / 262144
> .02202987670898437500
> 
> If this seems useful I can write up a commit message and include
> as its own patch in v7. Suggestions for better names?
> 

I expect users to regularly interact with the monitoring files. For example,
"read the core_energy of group x every second". An API like above would require
a contract that the scale value will never change from resctrl mount to
resctrl unmount. I understand that this implementation supports exactly this by
allowing an architecture to only enable an event once, but do you think this is
something that will always be the case? If not then an interface like above will
require user space to open, parse, close two files instead of one on a frequent basis.
This is not ideal if user space wants to read monitoring data of multiple
groups frequently.

I would also like to keep extensibility in mind. We now know that
unsigned decimal and fixed-point binary needs to be supported. I think any
new interface used to communicate formatting information to user space should be done
in a way that can be extended for a new format. That is, for example, why
I used the actual term "fixed-point" in the example. Something like this avoids
needing assumptions that a raw value always implies fixed-point format.

Reinette

Powered by blists - more mailing lists