Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6ba3568c-c28c-1445-7c18-d7c5dee7200a@intel.com>
Date:   Wed, 9 Aug 2023 15:39:16 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.14.0
Subject: Re: [PATCH v5 23/24] x86/resctrl: Move domain helper migration into
 resctrl_offline_cpu()
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>,
        <dfustini@baylibre.com>
References: <20230728164254.27562-1-james.morse@arm.com>
 <20230728164254.27562-24-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230728164254.27562-24-james.morse@arm.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?TU1WUW8wdnU5T0RSZmJUWFVva1MyWDkxQ1NxQnhBcVlHQ1pFc2V1c1liaEh2?=
 =?utf-8?B?VmwydTZ2aWZ6ZFpDQVZtMUlXSjAzNTgzQWh5c29zWXhvMWoyV3RISjQrbVFG?=
 =?utf-8?B?NWpNZGljVG1XNG5tcG5QcU9sUmZPOTgrZkYzOVN2b292V0ZsKy9QeVBTZS9Z?=
 =?utf-8?B?OC94aHZITVNVSVhZbEI0WW9tRG9uc0lYc3NuNzlFa0VXbkEzTVZUMmhLOG5m?=
 =?utf-8?B?VWdYTC9kSkN1dG5vTTBtcUNxWWpzcE9mMmlzRnp5SkloZ1hWblg2bU9EMENm?=
 =?utf-8?B?bkNJUGgzT2h4aTdUYktpVFNsUS9FTFJ4SkdVekFPa1dlZllSekhPZXpIYTZE?=
 =?utf-8?B?TUlKdkNmYWpZUGc4UFU3YzFCT2o5YlNNaWpSbjBUYzhsTVlpejRwdFdzS1pj?=
 =?utf-8?B?dWFYYjBFbUFZUUhzRkFsWFBaRC9HLzh3V0g0NHYxZUZQRDUvYkYrb2JnZWZT?=
 =?utf-8?B?WmUzYSsrc3paREN5dTRud2NVa3NxUG5NckRITTlSV2MzMjdnYno5RUNrNGcw?=
 =?utf-8?B?RUZmdUIxQ0RKSHJVU2xKSmQrRms2MlJWbFJrdVdqWU9oTzVsTXV3NjVtTlNk?=
 =?utf-8?B?K28xT1lmK2FlTXVmcVBKQi9tUXB4dk4zR0FVcVIwaFNoUTlBdm8xWHAxek5X?=
 =?utf-8?B?dGljaWFsQjM5MHlmOUErRWVWUjltYlA5cDQrUTg4am16Sk10bDFVRDMvSHd3?=
 =?utf-8?B?cE1pRVBWanRUMVBxWWl2MFFMYld2cGxFL29QbWVsZ0tZcmRaUzYrSURuMGk2?=
 =?utf-8?B?WHljaitxYmdzZXppdHhhTHNjcXd6R3prUTZlb0NTQ25Ud2dQK05sUlVySE4z?=
 =?utf-8?B?R1pFaHhiaWFwVnI3K0g3Mk92aEhxMS9kSWRBdjhkK3lPQUFBeXpBNWo3cURT?=
 =?utf-8?B?OXRwOEdCNkozUUdOWG1Qc0prTk1OdWRQTWd2eEFuYVRtMEVLVlJJbVhVTGhX?=
 =?utf-8?B?akl0cUtDSWwvTVJnSDFCd0hveGxOSDZoY3ZvRWx3T0tqbW1ZUDZhK3U0dWor?=
 =?utf-8?B?L1JWM2JUSnJqdHdiQ0pMR1BOWmFtWlBpekF0Y01kRUk0QzZ3dWczeG1VWENP?=
 =?utf-8?B?bnVmYXNvSUZadlhzWCtXZTRFaThGY0N3aWtqUy9EL2EvKyt3MHN5N1IyQ1Yy?=
 =?utf-8?B?TG80SzhLTlF1MU5qak96clpYVHQ0bzdOUDlMa0VWNFhEMHBjYzlVcXVzT3N4?=
 =?utf-8?B?b0tuRS9kcEwvS1ROY0xjMkxqTEROMkxMRlJ3KzJHTGRTZmdiais3cC9scVpr?=
 =?utf-8?B?NjlGR2Zyam9qckVaZE5EYWsxLy9IcnVNbjZLQzFBbi9BL3RUc1pUaUtUWG5n?=
 =?utf-8?B?OFJLcHFaRjhYTVJiY0FMYWRwL1MxOWhEeWRjZHFJcUw3VHdYbGhtQTZLQmxX?=
 =?utf-8?B?RUZjN3R1N2dCdjJWaDk4aStUMFN1Z3plM1B3YlYzUms5M3U5ckJCWHQ4VXE4?=
 =?utf-8?B?T0dJck1vK0xoVVdZQWhjYUFBdXR0QzRXWER0NEJQUGFhS01oTHdSNXRhYmtW?=
 =?utf-8?B?bU1PUGFiWW5kQ0RQbHZSR0RFZyszemtiaW1RWlhaZTJ0RktKbXpmVGVLTHBH?=
 =?utf-8?B?UE56WU1HZUxjMHRReDR5L2t0VDZkSXFLV2E2TUptZjVVRGxuOTlrTjk0alMr?=
 =?utf-8?B?TUxtY1FXbGh6aDVlYWpTN1VUZWVFSURwaWRDSmpNZE95MnRVSjU1bXBnU01X?=
 =?utf-8?B?ZHE0aHpRU3hTZytpSkFRNnlrbFVIQjR5VGt2OGEvTFZkSXU3SS9VUDA5Mktl?=
 =?utf-8?B?ZDRBVFlFVnoxT0NSQUQvV1RtUWdRQ2ZpTGI2aUxFcHpjUGljR0JpM2kydHNn?=
 =?utf-8?B?dmtuOXhteENDWXVLQmZWb0Uwd1dvMnU4NTZ2V0l2VUxRcDBFcHQzcE90eGFP?=
 =?utf-8?B?OGZ6VU1ES010Y1A4Z0FvN1ZQZGVKb3ZtcFA2STc0WVMxVldtVFpYU2VxRm0v?=
 =?utf-8?B?dFJkelFpMForY3ZwbXJXeGl0RE1lZDRsWndESXcwVnJVZUd0WU5uOUwwZWlt?=
 =?utf-8?B?R0VrZzFNOW5CNjFScWJoeUpsZFBjYmxsTVFPV1E4a0tqVzNHb29yblF5Yk1W?=
 =?utf-8?B?QXNPV3Znb2RtcHgwWUU4NUkyVXlCWUJQYlkvMTZxcjZpSDE0ZW45aFhWekdR?=
 =?utf-8?B?ZmFjc3pGbWQ4RjRrdjh5ZjVKNUNXWElmWWFld3RXeitBa3FNOXBkTHB1bTk4?=
 =?utf-8?B?bEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b11cd07b-6822-4f21-5bf5-08db9929783a
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2023 22:39:19.4523
 (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: IdMfqgCErdVjGdCVZlpZ4uAAAR/c5oVwZqBuWeXny1+Hut/8i3GESLH27E4kGfz/HX6HMHs9p28p/cWcFzz/LEzsz/KjnAFrgMEY6jUW4oE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7549
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 7/28/2023 9:42 AM, James Morse wrote:
> When a CPU is taken offline the resctrl filesystem code needs to check
> if it was the CPU nominated to perform the periodic overflow and limbo
> work. If so, another CPU needs to be chosen to do this work.
> 
> This is currently done in core.c, mixed in with the code that removes
> the CPU from the domain's mask, and potentially free()s the domain.
> 
> Move the migration of the overflow and limbo helpers into the filesystem
> code, into resctrl_offline_cpu(). As resctrl_offline_cpu() runs before
> the architecture code has removed the CPU from the domain mask, the
> callers need to be told which CPU is being removed, to avoid picking
> it as the new CPU. This uses the exclude_cpu feature previously
> added.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     | 16 ----------------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 +++++++++++++++
>  2 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 6eb9408a942a..edc0dd123317 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -578,22 +578,6 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>  
>  		return;
>  	}
> -
> -	if (r == &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl) {
> -		if (is_mbm_enabled() && cpu == d->mbm_work_cpu) {
> -			cancel_delayed_work(&d->mbm_over);
> -			/*
> -			 * temporary: exclude_cpu=-1 as this CPU has already
> -			 * been removed by cpumask_clear_cpu()d
> -			 */
> -			mbm_setup_overflow_handler(d, 0, RESCTRL_PICK_ANY_CPU);
> -		}
> -		if (is_llc_occupancy_enabled() && cpu == d->cqm_work_cpu &&
> -		    has_busy_rmid(d)) {
> -			cancel_delayed_work(&d->cqm_limbo);
> -			cqm_setup_limbo_handler(d, 0, RESCTRL_PICK_ANY_CPU);
> -		}
> -	}
>  }
>  
>  static void clear_closid_rmid(int cpu)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 12a628b5d476..a256a96df487 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3892,7 +3892,9 @@ static void clear_childcpus(struct rdtgroup *r, unsigned int cpu)
>  
>  void resctrl_offline_cpu(unsigned int cpu)
>  {
> +	struct rdt_domain *d;
>  	struct rdtgroup *rdtgrp;
> +	struct rdt_resource *l3 = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;

Please always keep reverse fir order.

>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> @@ -3902,6 +3904,19 @@ void resctrl_offline_cpu(unsigned int cpu)
>  			break;
>  		}
>  	}
> +

Can there be a l3->mon_capable check here to make things clear?

> +	d = get_domain_from_cpu(cpu, l3);
> +	if (d) {
> +		if (is_mbm_enabled() && cpu == d->mbm_work_cpu) {
> +			cancel_delayed_work(&d->mbm_over);
> +			mbm_setup_overflow_handler(d, 0, cpu);
> +		}
> +		if (is_llc_occupancy_enabled() && cpu == d->cqm_work_cpu &&
> +		    has_busy_rmid(d)) {
> +			cancel_delayed_work(&d->cqm_limbo);
> +			cqm_setup_limbo_handler(d, 0, cpu);
> +		}
> +	}
>  }
>  
>  /*

Reinette