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: <1250e10a-63b8-5192-493b-df4213c8e563@intel.com>
Date:   Wed, 9 Aug 2023 15:38:04 -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 20/24] x86/resctrl: Add cpu online callback for resctrl
 work
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-21-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230728164254.27562-21-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?dTFrbndDWFgvVUpieDg1MUpnSXQ1S3Jhb2oyeWYrdXBLR2QrK3gzYVh2OEt3?=
 =?utf-8?B?V1I5cEo2UFRkRFc3d1g5bWY4ZWw3cGhsMm9PT0M3VCs1ZzM3VVBDY1RsdmRI?=
 =?utf-8?B?aU1RYjFHVDdqQTJYWnpBRWEyS3hGQzBtMjcxTnVpOU9vblA3VHlwQmdDL2FI?=
 =?utf-8?B?RE9oeFBRbWpWY1hUYTF0NDJBN3pHbWRMb2hrcUdjM1NNMWtLdktxa0hGd0xt?=
 =?utf-8?B?cStVYjZkK1B6NGFiTnp3YTRpUno2czZIZ1NJdmpXMjBRU1JhcDBTUG4xUE9C?=
 =?utf-8?B?Nm5TcnVjMm1uU2RSVHRlZmoxQ0NyajgrNzU3WWFtWWowOUFraXB4SWJCcTIw?=
 =?utf-8?B?K0dPUy9LY0FnYW1wdkV3QjFEbEJMM1BPd2wrbTJORTd4VjMzcER0bDl3VE85?=
 =?utf-8?B?U08wWVJOQTVsRVRDMmk4R3h0WFV2WG5rZHZNanBjNnZKNnAvVGtDYit5TTd4?=
 =?utf-8?B?eHZNcFBNdDBybG9jb1JKWm9laTRiU0NSK3hKVW9VTWZscjN4MmpQUTUyVUdu?=
 =?utf-8?B?azNYOTFUdnNjcllKWlgzV0RtV1VZR2s0K2xZOGhVbkwxQ2M3VGk5ZGRaYkd3?=
 =?utf-8?B?UjRaZkZ0SmFjTVpPcFdSWFZvcnZqYW9NRlVzL2xZdHU3VWlHRGU0UmZ6ZXJz?=
 =?utf-8?B?QjRmT2pwOVk0L0dVeThlOVNLSjZmTFpQNVpyZThCSXBMb2hZZk5NNmVtbjll?=
 =?utf-8?B?T2RCcllvWGl0Tm9GdUxtajZrelVnMGJoQnhrdlM3OE4wN2RkYUZXaGxpK1RK?=
 =?utf-8?B?c2Y2R2ZRY3Z3OVpiS2FIUEplR21HeEVHa0VZZWNvVXlPQXh0VVU4c0tVT1B5?=
 =?utf-8?B?V2VUTHRZQjNPVFlXR3BQVzF5cGR6T1NiWVNDOG9JUWIxMWpuRkg4N3JKd2lD?=
 =?utf-8?B?ekdvc3BEbzdvVnlaaFhxODRLajRvMnB6ZzllN05MSERheUNKTmw1OTBscStL?=
 =?utf-8?B?UkR5OHY1ZldFMFBuZmhQaXlsT1F6R2x6Q2wvWW0reWRKOXFtVy9ySUxBbnlu?=
 =?utf-8?B?MSt3TDVhaEVWME9EU3ZxOHZpZVM1Q3VNMkxxMzVCVGNTR01QdEY2K0NKT1hE?=
 =?utf-8?B?dW4xRHB4ekdNWEpDMlNUSEhaNWtLT2U1ajk3WWQrQURqNlBaL2kzL2lBV0Mv?=
 =?utf-8?B?dWhkOGlIbnNKTCtCZXAzbGdnYTloWmRldHJUcTFvVmY5UVg5UjZsaWVjVVlj?=
 =?utf-8?B?T2hZNjJaMjJ5K0xJMzQrYkdpdDdhZEduZ2x5eDBkbDR6cjBYVWxDU2NSaVZJ?=
 =?utf-8?B?eEhIRktIMGJuY2srcjF5R3BKNDV4SFhFN1psTW5tNzJQazdtdXp6NFZqL1BY?=
 =?utf-8?B?aHBjZWhXVi81OTNCYXBBMUhpaUUyUUZocmJzcURGZUd6N2VnN3FpUi8vcUFU?=
 =?utf-8?B?RVhNQk9WeDBJMnJ0MFNrZ0E2Y2JvOXN3R2liUmtqQVJkQllHNWxHdXlsUXhW?=
 =?utf-8?B?NkxsL0ZwdFBzZm1HNkVRNnZuZ0t2cEtKN2RqUTR3cXJSV2E2UlBOeGdTNGZF?=
 =?utf-8?B?SndpWVJLaEYzd211a1hyMXVvMVo3cUp3WjJ4SFZOZVhXSTBhbE51SllwZFBT?=
 =?utf-8?B?NWRtWnJOSG4wOXBqNUxhaTUzZllCWjlKUlFyYko1NVJDLzhXZTVrTTJFc0ZZ?=
 =?utf-8?B?QUhrSkp1bGlMSkVUQTMwNFcvZ2psRmE3c1B1clplaHZvVmJSVEJrb201V3R1?=
 =?utf-8?B?Uk1wMzdnd0FidXZQVlBJVFArR0ZNQml2bldmSkVtdXY3dW1STW5kVXFPMHps?=
 =?utf-8?B?Ty9JU0h0akluVjZsU3VpRE1xaC9iOVoxNmZxVjNrL05jcmc5dVJ0QlMrbita?=
 =?utf-8?B?SWVzSmw1cVd3a3FiMmlmQmhkWGlvUzNsYnhzQXRBVVlFbjR5MTdWWkRvVk5W?=
 =?utf-8?B?VjRwVmFmbUlJNk1ORXUxVG5PTlVDVVFkcyt1cytVdVk2TmUzQXlMaHFLU3JG?=
 =?utf-8?B?ZmR2dEpjNUpSTzF3cHhkVEdWWlBEaUhjdEFsS0lNNlkxM2IvWXJseTM1UkJw?=
 =?utf-8?B?Ukl4b1BVczRqalRyVXh0WkFLT1A0aFVtYk1Id2c2anBrYUp5N21ESGlibzRQ?=
 =?utf-8?B?RGVSNmRPdVZjY3I4UWdUVjlWTm5qcWZnU21KWFVYa1BBWUtwSzBaSUtkMi9w?=
 =?utf-8?B?Q1B5Nm55NVRzcHVuU3JiVGJ5cFB0RlRtWXVYZUJseTU3MjZyZWR1TFJ3RHhR?=
 =?utf-8?B?MVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 87e08982-0b88-4765-2131-08db99294d33
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:38:07.2615
 (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: tFY6P+UMaok20sJ7u02Z6/2Fv9kYQjeBvc1rdRGsRoaKUhBboTww+vOlL3l5/l3ldqXgF3fc80xQALQmdQL2JRRzHxK54esiXMUSjM5x1gk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7549
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.9 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_BLOCKED,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,

(please also check subject lines for CPU instead of cpu)

On 7/28/2023 9:42 AM, James Morse wrote:
> The resctrl architecture specific code may need to create a domain when
> a CPU comes online, it also needs to reset the CPUs PQR_ASSOC register.
> The resctrl filesystem code needs to update the rdtgroup_default CPU
> mask when CPUs are brought online.
> 
> Currently this is all done in one function, resctrl_online_cpu().
> This will need to be split into architecture and filesystem parts
> before resctrl can be moved to /fs/.
> 
> Pull the rdtgroup_default update work out as a filesystem specific
> cpu_online helper. resctrl_online_cpu() is the obvious name for this,
> which means the version in core.c needs renaming.
> 
> resctrl_online_cpu() is called by the arch code once it has done the
> work to add the new CPU to any domains.
> 
> In future patches, resctrl_online_cpu() will take the rdtgroup_mutex
> itself.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v3:
>  * Renamed err to ret
> 
> Changes since v4:
>  * Changes in capitalisation.
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     | 11 ++++++-----
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 10 ++++++++++
>  include/linux/resctrl.h                |  1 +
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 8dfede01b0c9..a694563d3929 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -603,19 +603,20 @@ static void clear_closid_rmid(int cpu)
>  	wrmsr(MSR_IA32_PQR_ASSOC, 0, RESCTRL_RESERVED_CLOSID);
>  }
>  
> -static int resctrl_online_cpu(unsigned int cpu)
> +static int resctrl_arch_online_cpu(unsigned int cpu)
>  {
>  	struct rdt_resource *r;
> +	int ret;
>  
>  	mutex_lock(&rdtgroup_mutex);
>  	for_each_capable_rdt_resource(r)
>  		domain_add_cpu(cpu, r);
> -	/* The cpu is set in default rdtgroup after online. */
> -	cpumask_set_cpu(cpu, &rdtgroup_default.cpu_mask);
>  	clear_closid_rmid(cpu);
> +
> +	ret = resctrl_online_cpu(cpu);
>  	mutex_unlock(&rdtgroup_mutex);
>  
> -	return 0;
> +	return ret;
>  }

This is unexpected that resctrl_online_cpu() returns an error ... and
then the caller exits with failure without error handling or unwinding
the previous work. Is this error return needed? The function just
always returns zero so it looks like it could just be void.

>  
>  static void clear_childcpus(struct rdtgroup *r, unsigned int cpu)
> @@ -965,7 +966,7 @@ static int __init resctrl_late_init(void)
>  
>  	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>  				  "x86/resctrl/cat:online:",
> -				  resctrl_online_cpu, resctrl_offline_cpu);
> +				  resctrl_arch_online_cpu, resctrl_offline_cpu);
>  	if (state < 0)
>  		return state;
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index fef78a3dc632..7bd3a3dc0f44 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3868,6 +3868,16 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>  	return 0;
>  }
>  
> +int resctrl_online_cpu(unsigned int cpu)
> +{
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	/* The CPU is set in default rdtgroup after online. */
> +	cpumask_set_cpu(cpu, &rdtgroup_default.cpu_mask);
> +
> +	return 0;
> +}
> +
>  /*
>   * rdtgroup_init - rdtgroup initialization
>   *
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 5e4b4df9610b..35d3c97df212 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -223,6 +223,7 @@ u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_domain *d,
>  			    u32 closid, enum resctrl_conf_type type);
>  int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d);
>  void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d);
> +int resctrl_online_cpu(unsigned int cpu);
>  
>  /**
>   * resctrl_arch_rmid_read() - Read the eventid counter corresponding to rmid


Reinette