Received-SPF: pass (google.com: domain of linux-kernel+bounces-75626-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <89745b84-5ead-4694-b14c-341ca5a688f4@intel.com>
Date: Wed, 21 Feb 2024 14:59:43 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] x86/resctrl: Fix WARN in get_domain_from_cpu()
Content-Language: en-US
To: Tony Luck <tony.luck@intel.com>, Thomas Gleixner <tglx@linutronix.de>
CC: Borislav Petkov <bp@alien8.de>, James Morse <james.morse@arm.com>,
	"x86@kernel.org" <x86@kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "Yu, Fenghua" <fenghua.yu@intel.com>, "Ingo
 Molnar" <mingo@redhat.com>, H Peter Anvin <hpa@zytor.com>, Babu Moger
	<Babu.Moger@amd.com>, "shameerali.kolothum.thodi@huawei.com"
	<shameerali.kolothum.thodi@huawei.com>, D Scott Phillips OS
	<scott@os.amperecomputing.com>, "carl@os.amperecomputing.com"
	<carl@os.amperecomputing.com>, "lcherian@marvell.com" <lcherian@marvell.com>,
	"bobo.shaobowang@huawei.com" <bobo.shaobowang@huawei.com>,
	"tan.shaopeng@fujitsu.com" <tan.shaopeng@fujitsu.com>,
	"baolin.wang@linux.alibaba.com" <baolin.wang@linux.alibaba.com>, Jamie Iles
	<quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>,
	"peternewman@google.com" <peternewman@google.com>, "dfustini@baylibre.com"
	<dfustini@baylibre.com>, "amitsinght@marvell.com" <amitsinght@marvell.com>,
	David Hildenbrand <david@redhat.com>
References: <20240213184438.16675-1-james.morse@arm.com>
 <20240217105543.GAZdCQr_nosDP4tGuO@fat_crate.local> <87zfvwieli.ffs@tglx>
 <ZdUSwOM9UUNpw84Y@agluck-desk3>
 <d409c32d-0f9a-4773-bc25-9d39de3c9e9b@intel.com>
 <SJ1PR11MB6083A759748EAF62EDF21D0FFC502@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <ZdVFDIJmctsNaGd2@agluck-desk3> <ZdZPht8hY4J9uIOz@agluck-desk3>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZdZPht8hY4J9uIOz@agluck-desk3>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Vnp0ZmUwY2NuTWpVa1VDM0ptQjVYaGtuaFlwMFNIV2RoTFpEcnhTRWZqQXZu?=
 =?utf-8?B?SDQ3MHBCQ2xlY3VOVVl3Wk9lRlRWUk9WUHhhOGJNbm45TXhSQnpML0NRMDBR?=
 =?utf-8?B?Z1htZ3JlQWZkOUtZQ1JZeU1KOENZeGdrWVc2dXRCQVJvU1lQcVpabERFdEt0?=
 =?utf-8?B?QmNBTHNqK2EzLzFKYmtqbUFWOHpIK2poNXpKK2lhYTBVT2hvQSs1WE5ndmVt?=
 =?utf-8?B?eVI3bm44N3cyOUNJb2pzSDU0MFl6ZmZSZnNqekE4K3hSRmIrRmpNUCt3MUE4?=
 =?utf-8?B?RHFsNUhzQlFhdlpUSFB0WGJmK0VQMVJGNlBmajZJMEJTbC9DVHdrOFloZnFw?=
 =?utf-8?B?NDhaRWkyb0h6cWNYK2VxQVNyRUppeDZZMi9qZnFQeGpXaTNiRU5LNVRETXhS?=
 =?utf-8?B?MkRKbVNNRjRGZWFQN0VkTDJVQUZEZ3poS2R5SFZCbTZoU2UxdEl4ZHBaNG5I?=
 =?utf-8?B?d3REY3dKTEdBbDVRM2dzT0NvR3dFenFidnRPaU0rR0Z4R200WGFrcVdTLzR5?=
 =?utf-8?B?Q1FQdDVOYnFNRzNiV2JicWJNT3VIeHJuenJyazBqT2JLL1lONjdCejJnMlBC?=
 =?utf-8?B?aW9rUjd4bGQvR3hqRWhxakFIWjZ5NktuWWdORCt1SGVrWkFxUUswZTc2cTJr?=
 =?utf-8?B?Z29yQjBWeVROK1JRUjdsNnNXVi85NThYMkNncmhDTnhxWk5PL3ZkbGhqRmpl?=
 =?utf-8?B?aTd4SThYVFl5MlV6SkxwbVlSWTNwdFZWRHRFaDNrS2NKTkNpOHdRQjdyZlVR?=
 =?utf-8?B?SVJCNFhLOUF1a2hKMFQrV0xTMkpWNFh6STJMRWVWOWZlYWNqTnd3YzJrbkow?=
 =?utf-8?B?aFpEalZNSnZkM2RFcE10VHhwa25iVEloSCs5UCtsbmFWRXNicHFIUFh1RlRX?=
 =?utf-8?B?ejRybUpJUTljWXVxVFFrM2x2eXFobG4yclBja0dsYVFka2h4TW1pdFpTM0xS?=
 =?utf-8?B?NFJYT3FqQUtBaW41dTVob01TOThzL0FlR2VTdisremJMOGZWUFVlakZCanRn?=
 =?utf-8?B?OUhYUkxOaThnTXc5aUYrQy8xMFY2R0FZVVVaWU9ld29TSUpmS21NVDl5T0xO?=
 =?utf-8?B?K1BPSlA3cEVIV1R1Sm5ZbFNUWmJWZ3NpU1dmU21DUm4zSXVkWFN3M09jeGJD?=
 =?utf-8?B?UW01VHVOb1RCaG9QSlkyNGdLTGowUXhkUThqb2lKY3lWQzZCYjUzWXhiQzl2?=
 =?utf-8?B?WGJRQ2wxSXlZL3JwdlduV1N1TkFwUXVrYkw1VlZSVHRvUE9qUmJBOHpsL0pB?=
 =?utf-8?B?bHZEd2p0UWM4a1JtYWdVaTd6aUxGanJQWmFWbGlzR3dGY1V1azdTb1J2RkhX?=
 =?utf-8?B?cHBWTDErNm91dUF3ZzdldDJRMDhyQ1QzQ2dCNDY4a1NyMkIrN1JJbUsvemhB?=
 =?utf-8?B?RytkWjhHUFlva1ZTcDhwai9PejFodVlkQzg4RzFwN2k4enduTWY0Z0RWS1RJ?=
 =?utf-8?B?c3FOOHF1VDJVbFcrdkxRa2lkSjNvbWw2WjBXU1pQSW5xRHZIeFhoS1cycmhn?=
 =?utf-8?B?elRYdEkyS0xPUTlob3B4MHphekJJbEx2Mk5nMGplR1A1QkxwdVFQemR1bkRr?=
 =?utf-8?B?N29PTlZvNVB5M2VORzFVTUhzamxJcSsyVCtFUDdoczg2WHZ4cDI2NHVHaDNw?=
 =?utf-8?B?NnVqd3A2UGhXY0R4TERMd3pIRmd2anRXQjdVU1FzdlVkWVVoRXlHc0M2ZWpR?=
 =?utf-8?B?eEJoMk84QTg3dExjT3BHbkRFcWsvUnFFQzVjZkJsaEdnYTB6bHZ0eWJsZmxi?=
 =?utf-8?B?WmVWeW1YSXNhVXNzc1VXbzl5RXVpcGtlSXhpZ2pJQmdwQnBTQXA0a3grUkxO?=
 =?utf-8?B?aEtqS0puRDlzbGRRMkthWkFleWpGeXA5eEpsWDdITDcvZzFhaVZRZ3R6dXdF?=
 =?utf-8?B?RkZCVWVRVmgvaHZPZy9udHlyYVRFcExwdkJDaFNtMXNMdHhtcm9ORSthbmY4?=
 =?utf-8?B?aW9DQkx3U0F5Z1lQTjFOZzZIVm9Pbnh1aUVmZ05HNFFXWmRpQlRBRTRkRDN5?=
 =?utf-8?B?MW1iM1lmWFRCUWJCTzdBNWtjT2RzQVZHRlQ5ZmJ1aXlWTnNxaFAvTEJQWkUy?=
 =?utf-8?B?VWJPS0d3SG5sdkV1YVNWV2YzdWJYTVg1VHAvdTl4SzRrZkJCL1Z3MUR3TmNC?=
 =?utf-8?B?bjFpenlJSFFEamU3dUNkVFZMaEJ4b2t3TVRHVFJtUnFLWVJTSFh6QVdHd1gv?=
 =?utf-8?B?UkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35eeea86-8b40-4919-e2a5-08dc3330cc5a
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2024 22:59:46.1090
 (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: tw6UTbYmxq7n0hvRAtWJaHq7TqBcHOLXkhVQei1W0EgEkvM2yOk1l+nT5PEhrE5bKv5yt1FENG2nRLM07JkiwXY7IFo+UXcomeJK8xw3WUc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR11MB6045
X-OriginatorOrg: intel.com

Hi Tony,

On 2/21/2024 11:31 AM, Tony Luck wrote:
> reset_all_ctrls() and resctrl_arch_update_domains() use on_each_cpu_mask()
> to call rdt_ctrl_update() on potentially one CPU from each domain.
> 
> But this means rdt_ctrl_update() needs to figure out which domain to
> apply changes to. Doing so requires a search of all domains in a resource,
> which can only be done safely if cpus_lock is held. Both callers do hold
> this lock, but there isn't a way for a function called on another CPU
> via IPI to verify this.
> 
> Fix by adding the target domain to the msr_param structure and passing an
> array with CDP_NUM_TYPES entries. Then calling for each domain separately
> using smp_call_function_single()

This work contains no changes to get_domain_from_cpu(). I expect the WARN
within it to be removed as intended with [1] and then this work can build
on that without urgency. As I understand, to support the stated goal of this
work, I expect get_domain_from_cpu() in the end to not have any WARN or
IS_ENABLED checks, but just a lockdep_assert_cpus_held(). 

Do you have different expectations?

> Change the low level cat_wrmsr(), mba_wrmsr_intel(), and mba_wrmsr_amd()
> functions to just take a msr_param structure since it contains the
> rdt_resource and rdt_domain information.

Could moving the rdt_domain into msr_param be done in a separate patch?

..

> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 7997b47743a2..09f6e624f1bb 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -272,22 +272,6 @@ static u32 get_config_index(u32 closid, enum resctrl_conf_type type)
>  	}
>  }
>  
> -static bool apply_config(struct rdt_hw_domain *hw_dom,
> -			 struct resctrl_staged_config *cfg, u32 idx,
> -			 cpumask_var_t cpu_mask)
> -{
> -	struct rdt_domain *dom = &hw_dom->d_resctrl;
> -
> -	if (cfg->new_ctrl != hw_dom->ctrl_val[idx]) {
> -		cpumask_set_cpu(cpumask_any(&dom->cpu_mask), cpu_mask);
> -		hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> -
> -		return true;
> -	}
> -
> -	return false;
> -}
> -
>  int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_domain *d,
>  			    u32 closid, enum resctrl_conf_type t, u32 cfg_val)
>  {
> @@ -304,59 +288,50 @@ int resctrl_arch_update_one(struct rdt_resource *r, struct rdt_domain *d,
>  	msr_param.res = r;
>  	msr_param.low = idx;
>  	msr_param.high = idx + 1;
> -	hw_res->msr_update(d, &msr_param, r);
> +	hw_res->msr_update(&msr_param);
>  

Is this missing setting the domain in msr_param?

>  	return 0;
>  }
>  
>  int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>  {
> +	struct msr_param msr_param[CDP_NUM_TYPES];
>  	struct resctrl_staged_config *cfg;
>  	struct rdt_hw_domain *hw_dom;
> -	struct msr_param msr_param;
>  	enum resctrl_conf_type t;
> -	cpumask_var_t cpu_mask;
>  	struct rdt_domain *d;
> +	bool need_update;
> +	int cpu;
>  	u32 idx;
>  
>  	/* Walking r->domains, ensure it can't race with cpuhp */
>  	lockdep_assert_cpus_held();
>  
> -	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> -		return -ENOMEM;
> -
> -	msr_param.res = NULL;
> +	memset(msr_param, 0, sizeof(msr_param));
>  	list_for_each_entry(d, &r->domains, list) {
>  		hw_dom = resctrl_to_arch_dom(d);
> +		need_update = false;
>  		for (t = 0; t < CDP_NUM_TYPES; t++) {
>  			cfg = &hw_dom->d_resctrl.staged_config[t];
>  			if (!cfg->have_new_ctrl)
>  				continue;
>  
>  			idx = get_config_index(closid, t);
> -			if (!apply_config(hw_dom, cfg, idx, cpu_mask))
> +			if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
>  				continue;
> -
> -			if (!msr_param.res) {
> -				msr_param.low = idx;
> -				msr_param.high = msr_param.low + 1;
> -				msr_param.res = r;
> -			} else {
> -				msr_param.low = min(msr_param.low, idx);
> -				msr_param.high = max(msr_param.high, idx + 1);
> -			}
> +			hw_dom->ctrl_val[idx] = cfg->new_ctrl;
> +			cpu = cpumask_any(&d->cpu_mask);
> +
> +			msr_param[t].low = idx;
> +			msr_param[t].high = msr_param[t].low + 1;
> +			msr_param[t].res = r;
> +			msr_param[t].dom = d;
> +			need_update = true;
>  		}
> +		if (need_update)
> +			smp_call_function_single(cpu, rdt_ctrl_update, &msr_param, 1);

It is not clear to me why it is needed to pass this additional data. Why not
just use the original mechanism of letting the low and high of msr_param span the
multiple indices that need updating? There can still be a "need_update" but it
can be set when msr_param gets its first data. Any other index needing updating
can just update low/high and a single msr_param can be used.

Reinette

[1] https://lore.kernel.org/lkml/20240221122306.633273-1-james.morse@arm.com/