Received-SPF: pass (google.com: domain of linux-kernel+bounces-95900-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <3c2afc2b-85d7-4c2b-8a32-1a6b0f225328@intel.com>
Date: Thu, 7 Mar 2024 08:59:53 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] x86/mce: Dynamically size space for machine check
 records
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>
References: <20240307000256.34352-1-tony.luck@intel.com>
 <20240307121634.GAZemwIgbKKJGaUVFg@fat_crate.local>
Content-Language: en-US
From: Sohil Mehta <sohil.mehta@intel.com>
In-Reply-To: <20240307121634.GAZemwIgbKKJGaUVFg@fat_crate.local>
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?a1M1bnZmRkQwK0JsNkErZXZXclV5Szl0bDdLWVNBaktCaUlSY05nVkcyUlRG?=
 =?utf-8?B?WWFZdXJHV2Q4QkxQU1hkZ01TTXI1YUxoM1YybnVaMnNlUm5KUFRlWnZsRnkx?=
 =?utf-8?B?dFFWVkYrZ211aXVyK0RmNjRHZ0VkeXJKNE5pcmsrWGpRUlpqTmIwZkhOTEJi?=
 =?utf-8?B?SkFSVnM3WFE5enl6dm1GMTJVR3VuMmFIUmpMVklIMlJsZ3N5UzZYeTY0YVJL?=
 =?utf-8?B?VHpFZTVzeFVPT0RHb2JlcjJlZ296RXMyT3ZCUjFpVE1abmZvSkJBR3B4Nmd0?=
 =?utf-8?B?RTdDMTZrM3dxT2p2ZzNPbTlhdEtNalgvQmUzcS9sVEFyRWFVbnh3WFRKU0tU?=
 =?utf-8?B?MnoxU3RodHZrWllJRHpXQi9kRnl2dVZPTXhYSC8weFMxcU0zVGM5K1hOMGFn?=
 =?utf-8?B?YVFqallVeVBkN1l0REdCSmR4QzkrSWlSSk0vWlE2ZlB5b29Ra0RXekxPYkkv?=
 =?utf-8?B?ZHFxeHFzaFpLYWJGL2swVURwb3ljQUNwODR0ZnRkTFpVRTkvZGNSQnFwUVA4?=
 =?utf-8?B?TXZLVEFiNGlNR1BCc2h2ejRXWS9FZ2FLNDh5VFZRYld2dUlOalZZN1B4ZWZC?=
 =?utf-8?B?R3Q5M3lPcjMzbmo2Y0piRmVLTXZ2OURnNnFWRGxUVHdHa2pnV1g1WE1FMWxC?=
 =?utf-8?B?eEJyUk5rdk5BYnEzZzU0UXAyMHB1WkpnR21wQ0VGSzFKcDNqbkJpQWdJLzM1?=
 =?utf-8?B?ZUljVWw2NnhBMmxXZEdkMzllcWpDRG1FM0VmeE9UN24wTXRURFVzRVNiT3Na?=
 =?utf-8?B?VmlhazUrZTRHZU1OdWMzaDJJM2NORFhiZDVWVzFrb0xDelM5VzQ0ZjJqVlVS?=
 =?utf-8?B?eTRMQWdZMGp4dUtEWCtaMnBhT0w0VGpSSVR5N0JKcFZZbzJkOUJJSXJESm5j?=
 =?utf-8?B?bXl1NytHTXVZWEVGckpZOUlwdjNYZmU4VEVnN3BKcmVuTlBCa2lMc3dIRUdT?=
 =?utf-8?B?SWkxNzRQWUxiRkhqQjl2eWNVZ3paKzFJcGg5U0hSdnNQNW12cTZaZVZsanE2?=
 =?utf-8?B?MStBUlVqbDRCZzNsN3YxY2R0UXQ5TFloSEFQazRkZTVPK0pxZWd0NjB1S2ZK?=
 =?utf-8?B?UlFXaWd1S2lpRGFxRkR3M01TWll5KzU4anVjYmFxaEVwUUk3aUhZdkliY3BH?=
 =?utf-8?B?WkZMOEo3cnNtQlRua0VlZ2NOWnhIUDhoT0piWWcrRERVTGJ0NSt0aVRyeXIw?=
 =?utf-8?B?TFF6akZHRjRtWUsrWWs2aDJtZm9mK1hldndXZ01VaFhqbThURXJOOTYvSGIx?=
 =?utf-8?B?MldLa2VUQ0JxMmlubTBwOGZsTlVnWExZdFlEeE14cmFFdlA3R2J1RzRNYWV3?=
 =?utf-8?B?Zkg3WEs3S2hiaDlGeGRTeXR0VW1Zbm5veElYRUViNUIzUmp2bFpRdC8zNzNw?=
 =?utf-8?B?NDlmQ3hNSTlpY04raXVRa1E4MUxtcEYwR0lVYmFsVU9NMksvTkpIQzJkS3VX?=
 =?utf-8?B?S1lHMUQrTTYyaEFtQ2JMY1lJN2RkMmRpYTJVeE9qTUQwazVQVnpzQXQ0Wmda?=
 =?utf-8?B?MEZQT0RWbnRiQmNWb0dpaisraWRUb1p5SThiNjhZK0NPaVlacTdVVmhYY1hh?=
 =?utf-8?B?bXFNdmY4QkxSam5Yei9LWVlXNTZrc3RRQURwenFXR2VCT2dtV1l2OTBTM0h4?=
 =?utf-8?B?aTZzMCt2bDFQd3hXTkUxZUNVeVFSUFF3d3R3Ylh4NEdIaWZjeXlSLy9pSzNC?=
 =?utf-8?B?TDN4L3lGemx5N1M4cWZwcHNNWFpXbUt3N2hwaVB2aEwrV2ExcXBYRVJpRlpa?=
 =?utf-8?B?Njc5RkVkSlhmVDEzNFhjSFdOVjZWeVlZay8wMDFCdjJrRldKTUV1aFVEejFW?=
 =?utf-8?B?TTUwNGtkQmJISXJPNk05QUZ3NlM2SnByOFZWaGFWN0svc3VuUkJrTmZGR3VV?=
 =?utf-8?B?azBYaFF4eUdmQTBJaWUvaXJoVHdaRndhL2RQTk9xN2NyZWxJSkV0MlM5US9p?=
 =?utf-8?B?NURGUzIyZTlSZnl5STV6Z2loK2thZnFkWFZZWnRKUDhTUC9CVndURm8rbEpC?=
 =?utf-8?B?VkZ1MktEdWJRQUZiaUxtME52ZE5iWjdKcUdpdENEYTNyYlhLMWVGb05tc1NJ?=
 =?utf-8?B?SG9tV3ljVjkrdTJoTUpRa2Z1VE5vaFpjS0NlS3BSY21XWkh1amVUMzdRU3V5?=
 =?utf-8?Q?VLs6/2PGbxPXvtmPHIX2OEWfR?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b890d750-166c-4288-07c4-08dc3ec80760
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3320.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2024 17:00:01.8821
 (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: +7iiwOvNGnd4CjZFy5cfcYYHJh4w6KkdX0WDCk5Yc7S6f6o4SxWm42ICAU7BO6nBYH5JsgdcXosBsYhIiZBPHg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5251
X-OriginatorOrg: intel.com

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) {