Re: [PATCH v9 08/27] x86/resctrl: Expand the width of domid by replacing mon_data_bits

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

Hi James,

On 4/25/25 10:37 AM, James Morse wrote:
> ---
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 19 ++++--
>  arch/x86/kernel/cpu/resctrl/internal.h    | 39 ++++++-----
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 79 +++++++++++++++++++++--
>  3 files changed, 103 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 0a0ac5f6112e..159972c3fe73 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -667,7 +667,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	u32 resid, evtid, domid;

I was expecting this to look differently after reading 
https://lore.kernel.org/lkml/a9008c2d-e83d-4bc6-8197-0753666a7ec2@arm.com/

I believe u32 was used for resid, evtid, and domid because of how they
used to be initialized from the bitfield within the union. With the switch to
a struct that now has the proper types these can also use proper types.

	enum resctrl_res_level resid;
	enum resctrl_event_id evtid;
	int domid;

This highlights that the incorrect type propagated from rdtgroup_mondata_show()
to mon_event_read() where its "int evtid" parameter should also be
"enum resctrl_event_id evtid", which is a good complement to patch #14
that fixes the type used by functions called by mon_event_read().

>  	struct rdtgroup *rdtgrp;
>  	struct rdt_resource *r;
> -	union mon_data_bits md;
> +	struct mon_data *md;
>  	int ret = 0;
>  
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);


> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 36a862a4832f..954dc391fc33 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -103,27 +103,26 @@ struct mon_evt {
>  };
>  
>  /**
> - * union mon_data_bits - Monitoring details for each event file.
> - * @priv:              Used to store monitoring event data in @u
> - *                     as kernfs private data.
> - * @u.rid:             Resource id associated with the event file.
> - * @u.evtid:           Event id associated with the event file.
> - * @u.sum:             Set when event must be summed across multiple
> - *                     domains.
> - * @u.domid:           When @u.sum is zero this is the domain to which
> - *                     the event file belongs. When @sum is one this
> - *                     is the id of the L3 cache that all domains to be
> - *                     summed share.
> - * @u:                 Name of the bit fields struct.
> + * struct mon_data - Monitoring details for each event file.
> + * @list:            Member of the global @mon_data_kn_priv_list list.
> + * @rid:             Resource id associated with the event file.
> + * @evtid:           Event id associated with the event file.
> + * @sum:             Set when event must be summed across multiple
> + *                   domains.
> + * @domid:           When @sum is zero this is the domain to which
> + *                   the event file belongs. When @sum is one this
> + *                   is the id of the L3 cache that all domains to be
> + *                   summed share.
> + *
> + * Pointed to by the kernfs kn->priv field of monitoring event files.
> + * Readers and writers must hold rdtgroup_mutex.
>   */
> -union mon_data_bits {
> -	void *priv;
> -	struct {
> -		unsigned int rid		: 10;
> -		enum resctrl_event_id evtid	: 7;
> -		unsigned int sum		: 1;
> -		unsigned int domid		: 14;
> -	} u;
> +struct mon_data {
> +	struct list_head	list;
> +	enum resctrl_res_level	rid;
> +	enum resctrl_event_id	evtid;
> +	int			domid;
> +	bool			sum;
>  };
>  

Thank you.

>  /**
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index eccdfcb1a6f5..7ef5cf0c4d1d 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -45,6 +45,12 @@ LIST_HEAD(rdt_all_groups);
>  /* list of entries for the schemata file */
>  LIST_HEAD(resctrl_schema_all);
>  
> +/*
> + * List of struct mon_data containing private data of event files for use by
> + * rdtgroup_mondata_show(). Protected by rdtgroup_mutex.
> + */
> +static LIST_HEAD(mon_data_kn_priv_list);
> +
>  /* The filesystem can only be mounted once. */
>  bool resctrl_mounted;
>  
> @@ -3093,6 +3099,63 @@ static void rmdir_all_sub(void)
>  	kernfs_remove(kn_mondata);
>  }
>  
> +/**
> + * mon_get_kn_priv() - Get the mon_data priv data for this event.
> + *
> + * The same values are used across the mon_data directories of all control and
> + * monitor groups for the same event in the same domain. Keep a list of
> + * allocated structures and re-use an existing one with the same values for
> + * @rid, @domid, etc.
> + *
> + * @rid:    The resource id for the event file being created.
> + * @domid:  The domain id for the event file being created.
> + * @mevt:   The type of event file being created.
> + * @do_sum: Whether SNC summing monitors are being created.
> + */
> +static struct mon_data *mon_get_kn_priv(int rid, int domid,

"int rid" -> "enum resctrl_res_level rid"

> +					struct mon_evt *mevt,
> +					bool do_sum)
> +{
> +	struct mon_data *priv;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	list_for_each_entry(priv, &mon_data_kn_priv_list, list) {
> +		if (priv->rid == rid && priv->domid == domid &&
> +		    priv->sum == do_sum && priv->evtid == mevt->evtid)
> +			return priv;
> +	}
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return NULL;
> +
> +	priv->rid = rid;
> +	priv->domid = domid;
> +	priv->sum = do_sum;
> +	priv->evtid = mevt->evtid;
> +	list_add_tail(&priv->list, &mon_data_kn_priv_list);
> +
> +	return priv;
> +}
> +
> +/**
> + * mon_put_kn_priv() - Free all allocated mon_data structures.
> + *
> + * Called when resctrl file system is unmounted.
> + */
> +static void mon_put_kn_priv(void)
> +{
> +	struct mon_data *priv, *tmp;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	list_for_each_entry_safe(priv, tmp, &mon_data_kn_priv_list, list) {
> +		list_del(&priv->list);
> +		kfree(priv);
> +	}
> +}
> +
>  static void resctrl_fs_teardown(void)
>  {
>  	lockdep_assert_held(&rdtgroup_mutex);

Reinette