Re: [PATCH v2] x86/mce: Dynamically size space for machine check records

From: Sohil Mehta <sohil.mehta@intel.com>
To: Borislav Petkov <bp@alien8.de>, Tony Luck <tony.luck@intel.com>
Cc: "Naik, Avadhut" <avadnaik@amd.com>,
	Yazen Ghannam <yazen.ghannam@amd.com>, <x86@kernel.org>,
	<linux-edac@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] x86/mce: Dynamically size space for machine check records
Date: Thu, 7 Mar 2024 08:59:53 -0800	[thread overview]
Message-ID: <3c2afc2b-85d7-4c2b-8a32-1a6b0f225328@intel.com> (raw)
In-Reply-To: <20240307121634.GAZemwIgbKKJGaUVFg@fat_crate.local>

Hi Tony,

Overall, the patch looks good to me. Independent of the minor
suggestions below, please feel free to add.

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>


On 3/7/2024 4:16 AM, Borislav Petkov wrote:
> On Wed, Mar 06, 2024 at 04:02:56PM -0800, Tony Luck wrote:
>> -	ret = gen_pool_add(tmpp, (unsigned long)gen_pool_buf, MCE_POOLSZ, -1);
>> +	mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
>> +	mce_poolsz = mce_numrecords * (1 << order);
>> +	mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
>> +	if (!mce_pool) {
>> +		gen_pool_destroy(tmpp);
>> +		goto out;
>> +	}
>> +	ret = gen_pool_add(tmpp, (unsigned long)mce_pool, mce_poolsz, -1);
>>  	if (ret) {
>>  		gen_pool_destroy(tmpp);
>> +		kfree(mce_pool);
>>  		goto out;
> 
> Might as well get rid of the out label too since you're not doing the
> error handling pattern of jumping to err* labels and then unwinding. See
> diff below.
> 

> diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
> index 42ce3dc97ca8..cadf28662a70 100644
> --- a/arch/x86/kernel/cpu/mce/genpool.c
> +++ b/arch/x86/kernel/cpu/mce/genpool.c
> @@ -126,25 +126,24 @@ static int mce_gen_pool_create(void)
>  	order = order_base_2(sizeof(struct mce_evt_llist));
>  	tmpp = gen_pool_create(order, -1);
>  	if (!tmpp)
> -		goto out;
> +		return ret;
>  
>  	mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
>  	mce_poolsz = mce_numrecords * (1 << order);
>  	mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
>  	if (!mce_pool) {
>  		gen_pool_destroy(tmpp);
> -		goto out;
> +		return ret;
>  	}
>  	ret = gen_pool_add(tmpp, (unsigned long)mce_pool, mce_poolsz, -1);
>  	if (ret) {
>  		gen_pool_destroy(tmpp);
>  		kfree(mce_pool);
> -		goto out;
> +		return ret;
>  	}
>  
>  	mce_evt_pool = tmpp;
>  
> -out:
>  	return ret;
>  }
>  
> 

I was about the suggest the same thing and maybe slightly more. By
initializing ret when really needed, I find the code a bit easier to
follow. No strong preference here.

diff --git a/arch/x86/kernel/cpu/mce/genpool.c
b/arch/x86/kernel/cpu/mce/genpool.c
index cadf28662a70..83a01d20bbd9 100644
--- a/arch/x86/kernel/cpu/mce/genpool.c
+++ b/arch/x86/kernel/cpu/mce/genpool.c
@@ -118,22 +118,21 @@ int mce_gen_pool_add(struct mce *mce)

 static int mce_gen_pool_create(void)
 {
-	int mce_numrecords, mce_poolsz, order;
+	int mce_numrecords, mce_poolsz, order, ret;
 	struct gen_pool *tmpp;
-	int ret = -ENOMEM;
 	void *mce_pool;

 	order = order_base_2(sizeof(struct mce_evt_llist));
 	tmpp = gen_pool_create(order, -1);
 	if (!tmpp)
-		return ret;
+		return -ENOMEM;

 	mce_numrecords = max(MCE_MIN_ENTRIES, num_possible_cpus() * MCE_PER_CPU);
 	mce_poolsz = mce_numrecords * (1 << order);
 	mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);
 	if (!mce_pool) {
 		gen_pool_destroy(tmpp);
-		return ret;
+		return -ENOMEM;
 	}
 	ret = gen_pool_add(tmpp, (unsigned long)mce_pool, mce_poolsz, -1);
 	if (ret) {


