Re: [PATCH v6 14/30] x86,fs/resctrl: Support binary fixed point event counters

["Luck, Tony" <tony.luck@intel.com>]

On Fri, Jun 27, 2025 at 02:22:18PM -0700, Fenghua Yu wrote:
> Hi, Tony,
> 
> On 6/26/25 09:49, Tony Luck wrote:
> > Resctrl was written with the assumption that all monitor events can be
> > displayed as unsigned decimal integers.
> > 
> > Hardware architecture counters may provide some telemetry events with
> > greater precision where the event is not a simple count, but is a
> > measurement of some sort (e.g. Joules for energy consumed).
> > 
> > Add a new argument to resctrl_enable_mon_event() for architecture code
> > to inform the file system that the value for a counter is a fixed-point
> > value with a specific number of binary places.  The file system will
> > only allow architecture to use floating point format on events that it
> > marked with mon_evt::is_floating_point.
> 
> User app needs to know if a number is a floating pointer value or an integer
> value. I see you document the energy and activity events as floating point
> values and all others are integer values.
> 
> Is it better to show the value types in info directory?
> 
> e.g. create an info file "events_floating" which shows all events with
> floating point values. Events not in this info are integer by default.
> 
> This may have two benefits:
> 
> 1. An app can query the type info to parse the values accordingly without
> hard coding event types.
> 
> 2. Any future floating point events can be added here without changing the
> document.

Maybe. It's obvious which are floating point because the values
have a "." in them.  Some apps may not care about the difference
and just read everything as if they are floating point. Maybe
likely since the next step is to compute the rate with:
	(current_value - previous_value) / delta_t
which will be done as a floating point calculation with
microsecond timestamps.

But it wouldn't be hard to add an info file that lists which are
in floating point (maybe also to provide the precision as
suggested by Dave Martin).

[snip]

> > +static void print_event_value(struct seq_file *m, int binary_bits, u64 val)
> > +{
> > +	struct fixed_params *fp = &fixed_params[binary_bits];
> 
> Is it worth to have a boundary check here like? I'm afraid without the
> hardening check, a future caller may give a wrong value and cause hard
> debugged failure.
> 
> if (WARN_ON_ONCE(binary_bits >=MAX_BINARY_BITS))
> 
>     return;

Seems like belt and braces overkill. resctrl_enable_mon_event()
already has a check for MAX_BINARY_BITS and will not enable
an event if architecture provides a too-large value.

-Tony