Re: [PATCH v3 11/26] fs/resctrl: Add support for additional monitor event display formats

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

Hi Tony,

Please add context.

On 4/7/25 4:40 PM, Tony Luck wrote:
> Add a type field to both the mon_evt and mon_data structures.

This is getting too much? How about mon_data pointing to mon_evt?

> 
> Legacy monitor events are still all displayed as an unsigned decimal
> 64-bit integer.
> 
> Add an additional format of fixed-point with 18 binary places displayed
> as a floating point value with six decimal places.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl_types.h |  8 ++++++++
>  fs/resctrl/internal.h         |  4 ++++
>  fs/resctrl/ctrlmondata.c      | 23 ++++++++++++++++++++++-
>  fs/resctrl/monitor.c          |  3 +++
>  fs/resctrl/rdtgroup.c         |  1 +
>  5 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> index 898068a99ef7..fbd4b55c41aa 100644
> --- a/include/linux/resctrl_types.h
> +++ b/include/linux/resctrl_types.h

Why is this exposed to architecture? There are no users in arch code.
changelog gives no hints on how to interpret this.

> @@ -58,6 +58,14 @@ enum resctrl_event_id {
>  #define QOS_NUM_MBM_EVENTS	(QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
>  #define MBM_EVENT_IDX(evt)	((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
>  
> +/*
> + * Event value display types

How about "Event value display formats"?

> + */
> +enum resctrl_event_type {

resctrl_event_format/resctrl_event_fmt?

> +	EVT_TYPE_U64,

EVT_FMT_U64/EVT_FORMAT_U64?

> +	EVT_TYPE_U46_18,
> +};
> +
>  static inline bool resctrl_is_mbm_event(int e)
>  {
>  	return (e >= QOS_L3_MBM_TOTAL_EVENT_ID &&
> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 74a77794364d..4a840e683e96 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -71,6 +71,7 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
>   * struct mon_evt - Entry in the event list of a resource
>   * @evtid:		event id
>   * @name:		name of the event
> + * @type:		format for display to user
>   * @configurable:	true if the event is configurable
>   * @any_cpu:		true if this event can be read from any CPU
>   * @list:		entry in &rdt_resource->evt_list
> @@ -79,6 +80,7 @@ struct mon_evt {
>  	enum resctrl_event_id	evtid;
>  	enum resctrl_res_level	rid;
>  	char			*name;
> +	enum resctrl_event_type	type;
>  	bool			configurable;
>  	bool			any_cpu;
>  	struct list_head	list;
> @@ -89,6 +91,7 @@ struct mon_evt {
>   * @list:            List of all allocated structures.
>   * @rid:             Resource id associated with the event file.
>   * @evtid:           Event id associated with the event file.
> + * @type:            Format for display to user
>   * @sum:             Set when event must be summed across multiple
>   *                   domains.
>   * @domid:           When @sum is zero this is the domain to which
> @@ -104,6 +107,7 @@ struct mon_data {
>  	struct list_head list;
>  	unsigned int rid;
>  	enum resctrl_event_id evtid;
> +	enum resctrl_event_type type;
>  	unsigned int sum;
>  	unsigned int domid;
>  	bool any_cpu;
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index cd77960657f0..5ea8113c96ac 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -562,6 +562,27 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  	resctrl_arch_mon_ctx_free(r, evtid, rr->arch_mon_ctx);
>  }
>  
> +#define NUM_FRAC_BITS	18
> +#define FRAC_MASK	GENMASK(NUM_FRAC_BITS - 1, 0)
> +
> +static void show_value(struct seq_file *m, enum resctrl_event_type type, u64 val)

show_value() is a bit vague ... print_event_value() ?

> +{
> +	u64 frac;
> +
> +	switch (type) {
> +	case EVT_TYPE_U64:
> +		seq_printf(m, "%llu\n", val);
> +		break;
> +	case EVT_TYPE_U46_18:
> +		frac = val & FRAC_MASK;
> +		frac = frac * 1000000;
> +		frac += 1ul << (NUM_FRAC_BITS - 1);

Could you please help me understand why above line is needed? Seems like
shift below will just undo it?

> +		frac >>= NUM_FRAC_BITS;
> +		seq_printf(m, "%llu.%06llu\n", val >> NUM_FRAC_BITS, frac);
> +		break;
> +	}
> +}
> +
>  int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  {
>  	struct kernfs_open_file *of = m->private;
> @@ -633,7 +654,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	else if (rr.err == -EINVAL)
>  		seq_puts(m, "Unavailable\n");
>  	else
> -		seq_printf(m, "%llu\n", rr.val);
> +		show_value(m, md->type, rr.val);
>  
>  out:
>  	rdtgroup_kn_unlock(of->kn);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 1cf0b085e07a..1efad57d1d85 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -847,16 +847,19 @@ static struct mon_evt all_events[QOS_NUM_EVENTS] = {
>  		.name	= "llc_occupancy",
>  		.evtid	= QOS_L3_OCCUP_EVENT_ID,
>  		.rid	= RDT_RESOURCE_L3,
> +		.type	= EVT_TYPE_U64,
>  	},
>  	[QOS_L3_MBM_TOTAL_EVENT_ID] = {
>  		.name	= "mbm_total_bytes",
>  		.evtid	= QOS_L3_MBM_TOTAL_EVENT_ID,
>  		.rid	= RDT_RESOURCE_L3,
> +		.type	= EVT_TYPE_U64,
>  	},
>  	[QOS_L3_MBM_LOCAL_EVENT_ID] = {
>  		.name	= "mbm_local_bytes",
>  		.evtid	= QOS_L3_MBM_LOCAL_EVENT_ID,
>  		.rid	= RDT_RESOURCE_L3,
> +		.type	= EVT_TYPE_U64,
>  	},
>  };
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 97c2ba8af930..bd41f7a0f416 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2927,6 +2927,7 @@ static struct mon_data *mon_get_kn_priv(int rid, int domid, struct mon_evt *mevt
>  	priv->sum = do_sum;
>  	priv->evtid = mevt->evtid;
>  	priv->any_cpu = mevt->any_cpu;
> +	priv->type = mevt->type;
>  	list_add_tail(&priv->list, &kn_priv_list);
>  
>  	return priv;

Reinette