Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <5f1256d3-737e-a447-abbe-f541767b2c8f@intel.com>
Date:   Mon, 25 Sep 2023 16:25:15 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.1
Subject: Re: [PATCH v5 3/8] x86/resctrl: Split the rdt_domain structure
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>, <x86@kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        James Morse <james.morse@arm.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Babu Moger <babu.moger@amd.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <patches@lists.linux.dev>
References: <20230722190740.326190-1-tony.luck@intel.com>
 <20230829234426.64421-1-tony.luck@intel.com>
 <20230829234426.64421-4-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230829234426.64421-4-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djhJdmc2VXNaVkRuR0doSVBRUXF4K3hQdWhia1h5RWgrZDBpSDJ1bGJkR054?=
 =?utf-8?B?MlcvcUVCcUNZbzVXdENqWGpZNUt0SHR0cWpraTR3QllUY3c1QTlpR0ljdzFQ?=
 =?utf-8?B?d3kzcEszWGlHSVNpSVQ4T2piZFhUZTl3eWp1dy8wbTN4WlNXeEhDZUhPeXlk?=
 =?utf-8?B?MU50emp4YTQ0TWNTOXg4dXJyRFc5K1lmcFhpamdMQ29pVno0YWlsSVdtdVB2?=
 =?utf-8?B?cUJKWHVrWnlzdk81SElJLzJzTEI4cUswMm40anR5V0dOTVpDM0ZPV1JPT0g5?=
 =?utf-8?B?U2ZBVGR0aU5yRGRrcVQ0L2xkVFVZZmZQUExTYkpqZmdaZFZLTzVaRUhyTUpN?=
 =?utf-8?B?MzFDdFNsWDFaNFZnbXl0VXZqQklLeTVHNUJiZ21hd1dsNmRHajcyK1k3bjRD?=
 =?utf-8?B?UFI3K3VBWjJuaDBFL0RCRCtHWm9CRzVLK3h5THV3YW9QS3FaTDVseDdxSlJa?=
 =?utf-8?B?NDRtWXdCQjZJdm8zYVl1cnk0NFVxTjY4ZGY0SW1uWnBGNExwMUFUYnVoZ0JW?=
 =?utf-8?B?VmhHWEVVenNUVkVxUXhVdHBFMThJVEx3ZTZLWTdiY0xiN01VZjZCVnp6Vkt5?=
 =?utf-8?B?MmMwM25OWXJWMTJEYzVxQVZTdVJXV3MzdVZvNDR1WWxjaDlGMVFaNzQ5TXdM?=
 =?utf-8?B?TDZNRnMrb2xKY3Fwa2lZUVRtNmZmQlMzYisrUHlIaEg3cGNEMStLUTk4ZzJN?=
 =?utf-8?B?ZVdxa09SbnY3dGdhTlR2SEU3aXF1SkZwUGhUdmJGaUxtVy9hTGVlTWZ1ZHhs?=
 =?utf-8?B?Y1lPaC9kdEpYNkRjcUZ3Tm9nTGw4TkdLbS8vQUkwWTduQkE0eHAreVRaMTFK?=
 =?utf-8?B?Ukc0TE9tb29uTWVQSnlVS2xJWXh1MSt2ZUttT284eVdKZW5sZ1BiVWl1Z1RQ?=
 =?utf-8?B?eS90Yk9VME9SNFBmbXlBSHBKaE43dkFYVzBpcWtvZFlwQXR5Y3VQd043dGFT?=
 =?utf-8?B?d3FhVXhKVTg3aUNZOWdzL3pBcllrZ0pFc3VQT1V3OFJXUXBrNnJTaXlWeDFt?=
 =?utf-8?B?T2c5MW9kUzl4ZEpQM09QcEhUcEFXR0pnWlhPZzROalJHeFBBVU5BWTNXWDBu?=
 =?utf-8?B?bU1UV0FleUVkVTkrOGZ4T01vdG5hSWVZeElMSUN0SE5uTVk3Z3cvQ3ZvMmpJ?=
 =?utf-8?B?eTYrQUtyTkh2OTl2cjBxSDFHNnQ1K3JyTVV5Mkp3bkpJcTA3VjM0TzcwbklX?=
 =?utf-8?B?RzFnNDdyeGphYUN2Zk9EQWs5a2RoNWhLYkxPV1pLRkVESTN3cHBMaEZZd3ZY?=
 =?utf-8?B?MkJxYVA0aERHdGtLcU4xWS9iT29GaHhXbHRrTVdPby9WNkNwUFhVM3BKcEsr?=
 =?utf-8?B?TEpZWjQ2VmZJbU9XQ2JoN0I5SmpxOW1sQ3FHcUVvNkwyQSt4OGV1Z2QwNllG?=
 =?utf-8?B?MVVEd3R5L3NoNGRYZ1dEdVhJSjljWlNNWTZmWVdqWWVpYlk0K2J4Y0p2eVMz?=
 =?utf-8?B?OWJaZmJwTHlDU0o1V2FwcXcxMkVyRENtWUhIazNxaklacGxSRU5pQnh2a1F4?=
 =?utf-8?B?K1dZMFozSE1Pb2kzVTdnaVpRTUI1b1hlMlV6UTUzN0JiOXZaakRMdDcxekV0?=
 =?utf-8?B?T0YzRFBVZ3VlMkRlOExVbEpEQm9RT0g5TU5tMkJXcTZ1MXh0aisvR0Q5bCtV?=
 =?utf-8?B?S3plVjlyS2dEaVVmZDgrSkFyNDBKaUVkakIrOWphZmdsUEtRRlNUeTBtdDJv?=
 =?utf-8?B?MWhXOUE3NmxvZmNseEdxMDY1R0FWYUt4YmJtSDNUM1NwdmxxcDh0WnRObmlK?=
 =?utf-8?B?K3J5Mnp3NmFCamdLcXM5eWRPbE9nZVJ2UFBWZjZqd2J1MkZFOTdac3hhU3lh?=
 =?utf-8?B?YTVHdHRBd0hFREdEYTFUS01BbGh4bEM1c1E2L3h5WUdpUmxnQ0l1cStxVndp?=
 =?utf-8?B?VHZGc3hUbTh1ZWhrUUpwK1NxZnlmWjdvQWJkVy9PSDVabUxCckV1c2NWaEwr?=
 =?utf-8?B?QnQwSFlSZHBlME5VWjE0OHM5dDhaNkIvMmxoaTQyb2hnVG1OVDgxbEI4djY2?=
 =?utf-8?B?cFgwZTJQWjRUZVBVSGxXaUNWbkx3SDN6b2FaZkRaaXhBQU82OHJ1aFZLbE9Z?=
 =?utf-8?B?ei9XTHdmVERsQitrU0xXSXN2MW1WSW9nQXZEVyt6eVJDMWFvdkk3UXhyOUVu?=
 =?utf-8?B?VjRuQzkwUDdKTUxnSVJXNmVNNUV0SU15Q2NnNzZ0Q0FBS0dnSFFiWmdFY1VY?=
 =?utf-8?B?NXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ddb2cd4-f44e-451a-5fe5-08dbbe1eae01
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Sep 2023 23:25:18.1992
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: D0D3FrfFo9IbCcpTn0wxG7SHzTAABLbwhYzuIY06tsgVX661r9i2W0OwDMcJrou+bKtVswXC1a+Vm+/i6TknZKzD0EkgZ130eq0ZyGHn5HU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8033
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Mon, 25 Sep 2023 16:25:39 -0700 (PDT)

Hi Tony,

Subject:

x86/resctrl: Split the rdt_domain and rdt_hw_domain structures


On 8/29/2023 4:44 PM, Tony Luck wrote:
> The same rdt_domain structure is used for both control an monitor

"control an monitor" -> "control and monitor"

> functions. But this results in wasted memory as some of the fields
> are only used by control functions, while most are only used for monitor
> functions.
> 
> Create a new rdt_mondomain structure tailored explicitly for use in
> monitor parts of the core. Slim down the rdt_domain structure by
> removing the unused monitor fields.
> 

Similar to the previous patch I think it will make the code
easier to understand if the naming is clear for both
monitoring and control structured. Why not rdt_mondomain
and rdt_ctrldomain instead?


> Similar breakout of struct rdt_hw_mondomain from struct rdt_hw_domain.

rdt_hw_mondomain and rdt_hw_ctrldomain?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h                   | 46 +++++++++++++++--------
>  arch/x86/kernel/cpu/resctrl/internal.h    | 38 +++++++++++++------
>  arch/x86/kernel/cpu/resctrl/core.c        | 18 ++++-----
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  4 +-
>  arch/x86/kernel/cpu/resctrl/monitor.c     | 40 ++++++++++----------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 24 ++++++------
>  6 files changed, 101 insertions(+), 69 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 33856943a787..08382548571e 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -53,7 +53,29 @@ struct resctrl_staged_config {
>  };
>  
>  /**
> - * struct rdt_domain - group of CPUs sharing a resctrl resource
> + * struct rdt_domain - group of CPUs sharing a resctrl control resource
> + * @list:		all instances of this resource
> + * @id:			unique id for this instance
> + * @cpu_mask:		which CPUs share this resource
> + * @plr:		pseudo-locked region (if any) associated with domain
> + * @staged_config:	parsed configuration to be applied
> + * @mbps_val:		When mba_sc is enabled, this holds the array of user
> + *			specified control values for mba_sc in MBps, indexed
> + *			by closid
> + */
> +struct rdt_domain {
> +	// First three fields must match struct rdt_mondomain below.

Please avoid comments within declarations. Even so, could you please
elaborate what the above means? Why do the first three fields have to
match? I understand there is common code, for example, __rdt_find_domain()
that operated on the same members of the two structs but does that
require the members be in the same position in the struct?
I understand that a comment may be required if position in the struct
is important but I cannot see that it is.

> +	struct list_head		list;
> +	int				id;
> +	struct cpumask			cpu_mask;
> +
> +	struct pseudo_lock_region	*plr;
> +	struct resctrl_staged_config	staged_config[CDP_NUM_TYPES];
> +	u32				*mbps_val;
> +};
> +
> +/**
> + * struct rdt_mondomain - group of CPUs sharing a resctrl monitor resource
>   * @list:		all instances of this resource
>   * @id:			unique id for this instance
>   * @cpu_mask:		which CPUs share this resource
> @@ -64,16 +86,13 @@ struct resctrl_staged_config {
>   * @cqm_limbo:		worker to periodically read CQM h/w counters
>   * @mbm_work_cpu:	worker CPU for MBM h/w counters
>   * @cqm_work_cpu:	worker CPU for CQM h/w counters
> - * @plr:		pseudo-locked region (if any) associated with domain
> - * @staged_config:	parsed configuration to be applied
> - * @mbps_val:		When mba_sc is enabled, this holds the array of user
> - *			specified control values for mba_sc in MBps, indexed
> - *			by closid
>   */
> -struct rdt_domain {
> +struct rdt_mondomain {
> +	// First three fields must match struct rdt_domain above.

Same comment.

>  	struct list_head		list;
>  	int				id;
>  	struct cpumask			cpu_mask;
> +
>  	unsigned long			*rmid_busy_llc;
>  	struct mbm_state		*mbm_total;
>  	struct mbm_state		*mbm_local;
> @@ -81,9 +100,6 @@ struct rdt_domain {
>  	struct delayed_work		cqm_limbo;
>  	int				mbm_work_cpu;
>  	int				cqm_work_cpu;
> -	struct pseudo_lock_region	*plr;
> -	struct resctrl_staged_config	staged_config[CDP_NUM_TYPES];
> -	u32				*mbps_val;
>  };
>  
>  /**

...

> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 468c1815edfd..5167ac9cbe98 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -521,7 +521,7 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>  }
>  
>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> -		    struct rdt_domain *d, struct rdtgroup *rdtgrp,
> +		    struct rdt_mondomain *d, struct rdtgroup *rdtgrp,
>  		    int evtid, int first)
>  {
>  	/*
> @@ -544,7 +544,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  	struct rdtgroup *rdtgrp;
>  	struct rdt_resource *r;
>  	union mon_data_bits md;
> -	struct rdt_domain *d;
> +	struct rdt_mondomain *d;

Reverse fir order.

>  	struct rmid_read rr;
>  	int ret = 0;
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 66beca785535..42262d59ef9b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -170,7 +170,7 @@ static int __rmid_read(u32 rmid, enum resctrl_event_id eventid, u64 *val)
>  	return 0;
>  }
>  
> -static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_domain *hw_dom,
> +static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_mondomain *hw_dom,
>  						 u32 rmid,
>  						 enum resctrl_event_id eventid)
>  {
> @@ -189,10 +189,10 @@ static struct arch_mbm_state *get_arch_mbm_state(struct rdt_hw_domain *hw_dom,
>  	return NULL;
>  }
>  
> -void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d,
> +void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mondomain *d,
>  			     u32 rmid, enum resctrl_event_id eventid)
>  {
> -	struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> +	struct rdt_hw_mondomain *hw_dom = resctrl_to_arch_mondom(d);
>  	struct arch_mbm_state *am;
>  
>  	am = get_arch_mbm_state(hw_dom, rmid, eventid);
> @@ -208,9 +208,9 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_domain *d,
>   * Assumes that hardware counters are also reset and thus that there is
>   * no need to record initial non-zero counts.
>   */
> -void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_domain *d)
> +void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mondomain *d)
>  {
> -	struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> +	struct rdt_hw_mondomain *hw_dom = resctrl_to_arch_mondom(d);
>  
>  	if (is_mbm_total_enabled())
>  		memset(hw_dom->arch_mbm_total, 0,
> @@ -229,11 +229,11 @@ static u64 mbm_overflow_count(u64 prev_msr, u64 cur_msr, unsigned int width)
>  	return chunks >> shift;
>  }
>  
> -int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
> +int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_mondomain *d,
>  			   u32 rmid, enum resctrl_event_id eventid, u64 *val)
>  {
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> -	struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> +	struct rdt_hw_mondomain *hw_mondom = resctrl_to_arch_mondom(d);

Reverse fir.

>  	struct arch_mbm_state *am;
>  	u64 msr_val, chunks;
>  	int ret;
> @@ -245,7 +245,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>  	if (ret)
>  		return ret;
>  
> -	am = get_arch_mbm_state(hw_dom, rmid, eventid);
> +	am = get_arch_mbm_state(hw_mondom, rmid, eventid);
>  	if (am) {
>  		am->chunks += mbm_overflow_count(am->prev_msr, msr_val,
>  						 hw_res->mbm_width);
> @@ -266,7 +266,7 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>   * decrement the count. If the busy count gets to zero on an RMID, we
>   * free the RMID
>   */
> -void __check_limbo(struct rdt_domain *d, bool force_free)
> +void __check_limbo(struct rdt_mondomain *d, bool force_free)
>  {
>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>  	struct rmid_entry *entry;
> @@ -305,7 +305,7 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
>  	}
>  }
>  
> -bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d)
> +bool has_busy_rmid(struct rdt_resource *r, struct rdt_mondomain *d)
>  {
>  	return find_first_bit(d->rmid_busy_llc, r->num_rmid) != r->num_rmid;
>  }
> @@ -334,7 +334,7 @@ int alloc_rmid(void)
>  static void add_rmid_to_limbo(struct rmid_entry *entry)
>  {
>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> -	struct rdt_domain *d;
> +	struct rdt_mondomain *d;
>  	int cpu, err;
>  	u64 val = 0;
>  
> @@ -383,7 +383,7 @@ void free_rmid(u32 rmid)
>  		list_add_tail(&entry->list, &rmid_free_lru);
>  }
>  
> -static struct mbm_state *get_mbm_state(struct rdt_domain *d, u32 rmid,
> +static struct mbm_state *get_mbm_state(struct rdt_mondomain *d, u32 rmid,
>  				       enum resctrl_event_id evtid)
>  {
>  	switch (evtid) {
> @@ -516,7 +516,7 @@ void mon_event_count(void *info)
>   * throttle MSRs already have low percentage values.  To avoid
>   * unnecessarily restricting such rdtgroups, we also increase the bandwidth.
>   */
> -static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
> +static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mondomain *dom_mbm)
>  {
>  	u32 closid, rmid, cur_msr_val, new_msr_val;
>  	struct mbm_state *pmbm_data, *cmbm_data;
> @@ -600,7 +600,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
>  	}
>  }
>  
> -static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, int rmid)
> +static void mbm_update(struct rdt_resource *r, struct rdt_mondomain *d, int rmid)
>  {
>  	struct rmid_read rr;
>  
> @@ -641,12 +641,12 @@ void cqm_handle_limbo(struct work_struct *work)
>  	unsigned long delay = msecs_to_jiffies(CQM_LIMBOCHECK_INTERVAL);
>  	int cpu = smp_processor_id();
>  	struct rdt_resource *r;
> -	struct rdt_domain *d;
> +	struct rdt_mondomain *d;

Reverse fir (Please check all code).

Reinette