Message-ID: <a9008c2d-e83d-4bc6-8197-0753666a7ec2@arm.com>
Date: Thu, 24 Apr 2025 12:15:34 +0100
From: James Morse <james.morse@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, x86@...nel.org,
 linux-kernel@...r.kernel.org
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>,
 Babu Moger <Babu.Moger@....com>, shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
 Shanker Donthineni <sdonthineni@...dia.com>, fenghuay@...dia.com,
 Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH v8 08/21] x86/resctrl: Expand the width of dom_id by
 replacing mon_data_bits

Hi Reinette,

On 16/04/2025 01:34, Reinette Chatre wrote:
> There is no occurence of "dom_id" in patch other than subject.

Fixed.


> On 4/11/25 9:42 AM, James Morse wrote:
>> MPAM platforms retrieve the cache-id property from the ACPI PPTT table.
>> The cache-id field is 32 bits wide. Under resctrl, the cache-id becomes
>> the domain-id, and is packed into the mon_data_bits union bitfield.
>> The width of cache-id in this field is 14 bits.
>>
>> Expanding the union would break 32bit x86 platforms as this union is
>> stored as the kernfs kn->priv pointer. This saved allocating memory
>> for the priv data storage.
>>
>> The firmware on MPAM platforms have used the PPTT cache-id field to
>> expose the interconnect's id for the cache, which is sparse and uses
>> more than 14 bits. Use of this id is to enable PCIe direct cache
>> injection hints. Using this feature with VFIO means the value provided
>> by the ACPI table should be exposed to user-space.
>>
>> To support cache-id values greater than 14 bits, convert the
>> mon_data_bits union to a structure. These are shared between control
>> and monitor groups, and are allocated on first use. The list of
>> allocated struct mon_data is free'd when the filesystem is umount()ed.

>> 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
>> @@ -676,17 +676,22 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>>  		goto out;
>>  	}
>>  
>> -	md.priv = of->kn->priv;
>> -	resid = md.u.rid;
>> -	domid = md.u.domid;
>> -	evtid = md.u.evtid;
>> +	md = of->kn->priv;
>> +	if (WARN_ON_ONCE(!md)) {
>> +		ret = -EIO;
>> +		goto out;
>> +	}
>> +
>> +	resid = md->rid;
>> +	domid = md->domid;
>> +	evtid = md->evtid;
> 
> What is not visible in this hunk is the types of these variables, which is:
> 	u32 resid, evtid, domid;
> 
> These types support the previously used bitfields well but now that the
> data is provided via a struct it should be possible to use appropriate
> types and avoid this unnecessary switch between types (more below).

Sure, I left them alone as they were already mismatched...


>>  	r = resctrl_arch_get_resource(resid);
>>  
>> -	if (md.u.sum) {
>> +	if (md->sum) {
>>  		/*
>>  		 * This file requires summing across all domains that share
>>  		 * the L3 cache id that was provided in the "domid" field of the
>> -		 * mon_data_bits union. Search all domains in the resource for
>> +		 * struct mon_data. Search all domains in the resource for
>>  		 * one that matches this cache id.
>>  		 */
>>  		list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 36a862a4832f..d932dd1eaa74 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 list of all allocated structures.

> To help readers this can mention the name of the list. Can simply be
> 	@list:          Entry in @listname.

Sure,
| Member of the global @mon_data_kn_priv_list list.

I was curious how kernel-doc would resolves references like that - turns out its just
formatting.


>> + * @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.
>> + *
>> + * Stored in the kernfs kn->priv field, readers and writers must hold
>> + * rdtgroup_mutex.
> 
> "Stored in the kernfs kn->priv field" can be made more specific with, for example,
> "Pointed to by kernfs kn->priv field of monitoring event file"

Sure,


>>   */
>> -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;
>> +	unsigned int rid;
>> +	enum resctrl_event_id evtid;
>> +	unsigned int sum;
>> +	unsigned int domid;
>>  };
> 
> The usage of the unsigned int was in support for the bitfield, but now
> the most appropriate types can be used instead? rid can be int, or even enum
> resctrl_res_level, domid can be int, and sum can be bool.

Yup. This was just left as is as its the existing behaviour.


> Also, every struct in this file follows the custom as documented in
> maintainer-tip.rst: "Struct declarations should align the struct member
> names in a tabular fashion". This struct should follow custom.

Fixed.


>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index c69ed978aa50..aa0bc57e1c7f 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 'priv' structures for rdtgroup_mondata_show().
> 
> "struct mon_data 'priv' structures" seems redundant use of struct/structures?

Heh, this is because I go back and add 'struct' before structure names as a second pass.


> How about:
> "List of struct mon_data containing private data of event files for use by rdtgroup_mondata_show()."

Done!


>> + * Protected by rdtgroup_mutex.
>> + */
>> +static LIST_HEAD(kn_priv_list);
> 
> Considering all the different "kn" involved in resctrl I find this name
> very generic for a global variable. I am not sure if something like
> "mon_data_kn_priv_list" would be considered too long? Open to recommendations.

Its suitably descriptive and doesn't cause a line length problems. Done.


>> +
>>  /* The filesystem can only be mounted once. */
>>  bool resctrl_mounted;
>>  
>> @@ -3089,6 +3095,62 @@ 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 in multiple directories. Keep a list
> 
> "The same values are used in multiple directories." is vague.
> How about "The same values are used across the mon_data directories
> of all control and monitor groups." Please feel free to improve.

I've tacked "for the same event in the same domain" on the end, otherwise
it reads like there is a single set of values.


>> + * of allocated structures and re-use an existing one with the same
>> + * list of values for rid, domain, etc.
> 
> "list of values" -> "values"?

Fixed,


> Also, if using "rid", which is parameter name, why use "domain" instead
> of "domid"? If using parameter names the kernel-doc "@" can be used
> for highlighting.

Done,


> With the many usages of "event type being created" below the above description
> will be helpful if it could define what is meant with an "event type".

I'll change these to 'event file', which at least matches the earlier comments.


>> + *
>> + * @rid:    The resource id for the event type being created.
>> + * @domid:  The domain id for the event type being created.
>> + * @mevt:   The event type being created.
>> + * @do_sum: Whether SNC summing monitors are being created.
>> + */
>> +static struct mon_data *mon_get_kn_priv(int rid, int domid,
>> +					struct mon_evt *mevt,
>> +					bool do_sum)


Thanks,

James

Powered by blists - more mailing lists