Re: [PATCH v4 13/31] fs/resctrl: Add support for additional monitor event display formats

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 5/8/25 1:28 PM, Luck, Tony wrote:
> On Thu, May 08, 2025 at 08:49:56AM -0700, Reinette Chatre wrote:
>> On 4/28/25 5:33 PM, Tony Luck wrote:
>>> Resctrl was written with the assumption that all monitor events
>>> can be displayed as unsigned decimal integers.
>>>
>>> Some telemetry events provide greater precision where architecture code
>>> uses a fixed point format with 18 binary places.
>>>
>>> Add a "display_format" field to struct mon_evt which can specify
>>> that the value for the event be displayed as an integer for legacy
>>> events, or as a floating point value with six decimal places converted
>>> from the fixed point format received from architecture code.
>>
>> There was no discussion on this during the previous version.
>> While this version addresses the issue of architecture changing the
>> format it does not address the issue of how to handle different
>> architecture formats. With this change any architecture that may
>> want to support any of these events will be required to translate
>> whatever format it uses into the one Intel uses to be translated
>> again into format for user space. Do you think this is reasonable? 
>>
>> Alternatively, resctrl could add additional file that contains the
>> format so that if an architecture in the future needs to present data
>> differently, an interface will exist to guide userspace how to parse it.
>> Creation of such user interface cannot be delayed until the time
>> it is needed since then these formats would be ABI.
> 
> What if resctrl filesystem allows architecture to supply the number
> of binary places for fixed point values when enabling an event?

This sounds good. I do not think we are in a position to come up with
an ideal solution. That would require assumptions of what another
architecture may or may not do and thus we do not have complete information.

> 
> That would allow h/w implementations to pick an appropriate precision
> for each new event. Different implementations of the same event
> (e.g. "core_energy") may pick different precision across architectures
> or between generations of the same architecture.
> 
> File system code can then do:
> 
> 	if (binary_places == 0)
> 		display as integer
> 	else
> 		convert to floating point (with one decimal place per
> 		three binary places)

I do not think this problem needs to be solved in this work but there needs
to be a plan for how other architectures can be supported. When similar
enabling needs to be done for that hypothetical architecture then it can
be implemented ... if it is still valid based on what that architecture actually
supports.
It may be sufficient for the "plan" (as above) to be in comments.

> 
> Existing events are all integers and won't change (it would be weird
> for an architecture to report "mbm_local_bytes" with a fixed point
> rather than integer value).
> 
> New events may report in either integer or floating point format
> with varying amounts of precision. But I'm not sure that would be

Partly this will depend on the unit of measurement that should form part of
the definition of the event. For example, events reporting cycles or ticks
should only be integer, no?

> a burden for writing tools that can run on different architectures.

Maybe just a comment in the docs then ... and now I see that you did
so already. My apologies, I did not get to the last four patches.

Reinette