Received-SPF: pass (google.com: domain of linux-kernel+bounces-86972-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: <b019fb64-fde5-4ade-964b-03a3a217dfd2@amd.com>
Date: Thu, 29 Feb 2024 10:49:18 -0500
User-Agent: Mozilla Thunderbird
Cc: yazen.ghannam@amd.com, "Naik, Avadhut" <avadnaik@amd.com>,
 "x86@kernel.org" <x86@kernel.org>,
 "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 Avadhut Naik <avadhut.naik@amd.com>
Subject: Re: [PATCH] x86/mce: Dynamically size space for machine check records
To: Sohil Mehta <sohil.mehta@intel.com>, Tony Luck <tony.luck@intel.com>,
 Borislav Petkov <bp@alien8.de>
References: <20240212093246.GBZcnlvkPKDC8C7rv5@fat_crate.local>
 <SJ1PR11MB6083B3511D18787BE823AB2CFC482@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <20240212175408.GIZcpbQHVjEtwRKLS-@fat_crate.local>
 <SJ1PR11MB60830AF35FA89C7869B8C11EFC482@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <20240212191401.GLZcpt-XHFqPg3cDw-@fat_crate.local>
 <SJ1PR11MB6083C60D7584B02E9CAF19D5FC482@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <ZcqPhVO_DtD2x5N7@agluck-desk3>
 <20240212220833.GQZcqW4WxKH34i-oBR@fat_crate.local>
 <20240212221913.GRZcqZYRd6EPTTnN97@fat_crate.local>
 <20240212224220.GSZcqezMhPojxvIcvO@fat_crate.local>
 <Zd--PJp-NbXGrb39@agluck-desk3>
 <015bf75e-bbe7-44ea-a176-9f1257f56b81@intel.com>
Content-Language: en-US
From: Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <015bf75e-bbe7-44ea-a176-9f1257f56b81@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?dDk0OHFCQ1dZem43NGpxdGRuS1JPWmo3N2o5WGV0dnBkUmpacm1HVk1oNWZz?=
 =?utf-8?B?UHlNcFo1UmJDZytWT1pnZlRINGdZY0dCcGw3Y3NPay9sK3ZtSDdIN2ZPVS9X?=
 =?utf-8?B?K0hGV0tqbjdiUFZNQWRqTUhWVHdUamhPMGRNcnhlem9WNEhHdzdWdS8veHVV?=
 =?utf-8?B?bzRTd2IrZllreUdhWWwwZXg4RlpTWVZ3Nlp2blVMSE9FUGp2ZzU2ZWNBdnBu?=
 =?utf-8?B?Z0g1bGhNajZpalg1ckE5YzVqR3JwSXpzNkMxRkhJZHFDbmYxR3ZqRlJGMHpG?=
 =?utf-8?B?S05vZTZSZVlJZFBjWFpaNXV3K25xdzBvdWlndFFKdFlCSE1XVURIVUZvaEJN?=
 =?utf-8?B?bmthb0pqWm42R1JTTEJNczJRZHlHUWRJcXdZaXl0aGNRNXhsNXprTTcxamZD?=
 =?utf-8?B?dWJWRitLUE9vVkJlLzhmSW00V1VCMUNtUmQ4QnZuMEJydmwyc050UmpLVHVn?=
 =?utf-8?B?cVp2bEtsQVdtTjZCQzUzd2QzZk1raU8weTV0QUFkenFwVXRrMjhxQnE0aVNv?=
 =?utf-8?B?OWtwaGV4a3k2ZEU5cENCSHJLN09GeVdOUTVhWXpvUE1kcHQzZnFsS0xzTTg5?=
 =?utf-8?B?M0tFRmR2aVpWN0NMQ2RuMnlMN1pnVzdHSEN4ZUVsQjEycnpndTgrSnI2QUhC?=
 =?utf-8?B?cDR2dTRSUkxPaUgvU2I2RVVPc3Vwd2tnSEM2d05lWjVHUzkxbkh5Ulc1RE5X?=
 =?utf-8?B?a1k0QmYrWjRSdW1WQnI3YXdiQ3l4bDdyZ2lMSHpLTjhjcmdGUVFnT2Z0eDBa?=
 =?utf-8?B?aWpzK3dIeWMzRHZzZlVrLzNQUHJDMkV3NG9VaFpOSzJzSEk5bURUK09SUUk5?=
 =?utf-8?B?WGZsQXMyWTJ1TkN0WFJNeWo3ckVRYVlmU01HbWtMSHpoN0ZnenN1YzNwSGRx?=
 =?utf-8?B?dkFXcE5qSDNZb3ovdkJmZ1BUSDBFMVVsM2FIYkd2T2U4eU1NVXZhTU56WU5w?=
 =?utf-8?B?VldpQWkvNlNrbElkTG1mdUlydnpUN3NDdVVxSDlIV2d5emdZT1lvcUZpOWl2?=
 =?utf-8?B?VlFsaWxmQ2c3MWZ0TjRhTGZ0bk1KaDh5M3RXcVVGdkxIUU0yaEhXU25pVGt2?=
 =?utf-8?B?dWR5NitucmV0eXdrbnBQekRHRytGRWNSOGVoU3l0d3R5WGJ0elJOQVBFeXdz?=
 =?utf-8?B?NVhTWkRLbWtRSkpFU0tibklkUVVBV1FuUFN3NzdmYnJKNm9yWldQNGtLWU5Q?=
 =?utf-8?B?RnNCbm9nRGErV2pydWd2NVZ4a1FrYXNLVGcyWU1hU3NEY2tJQ0JRQ2RYQXoy?=
 =?utf-8?B?VEdHcW5kdGxJQi91ZVRQLzlHejVZbGM4dzhndWVoTVZkWFY4dUJQdE9zUDhG?=
 =?utf-8?B?Q2tnaitlZWJsUDFWU2lPOFJ2VWpRYmsrTERqeElYWUVRUjRlNVBvcW5RRWVT?=
 =?utf-8?B?eWhZa005RDg5b1FzdmRFTldYNGY3Yk1FZkh5b04zeStWczQ2RldZZlByTURF?=
 =?utf-8?B?aG9DQXh3dEh6ODZsV1FxZXRLZzcwM3k0bXJPN2V2MGl5WWpSL3lUVThWdnFP?=
 =?utf-8?B?MWZ6QW03dVpCVjRpaGVidmI5N1M3RGc5NmJwTzFpczB3TFVKaG1Lb1VZWXZZ?=
 =?utf-8?B?RWg5Rll2K3krZEVFOFd1dVJvY2hmdXhGR2FlbUlFSVo3UDVQQ1Bvc0pzWWJv?=
 =?utf-8?B?Zlk4ZTB6MmgwazNSaDV1ZUZ6UVJSaFg0MUhCK3F6OGgxUEtEdElPVVFIZjlx?=
 =?utf-8?B?SVJpdCt5QWhRR2V1Mllmc0lwY1F1aC9GL0dsWU1IN0pHcTJXOVh2bTVkSUx4?=
 =?utf-8?B?VnFNU2hIOTBYeDNKblQxdEdQTVBPTXhEVVNjTDZiOU15TENnbTdFaDRhb0dK?=
 =?utf-8?B?UzFGV2V1VHNnZ09zU21MeU1CYnFWVmZ5YzhKb0VaUnhTQUNCenQ2VXpMbFBN?=
 =?utf-8?B?enZXUXNNeFFrbk41OGQ0VGEvS2RBR0RVNDFvUjBONENSN0ZpYjloSkZUeGor?=
 =?utf-8?B?QjR1bzROUmREcllmWmhkVGJ6N0t1cXk0T1NzRVVlUmgxYTNwTHhLZG1hZGZZ?=
 =?utf-8?B?b3NBUUlEQWNRMVVibUlzU3NZcSthQWVBaENMekVvYlczanE1MDlaK2c2TkFs?=
 =?utf-8?B?MmVuOWkvR1RWVFBLUVZUY1lwMUptTkxSeThIbTBGN0xtY2QvdEhJY09ROVRu?=
 =?utf-8?Q?vVLInAnMGcGxmkcgyOXlYan8v?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6e30f22f-f4df-44a2-d85c-08dc393dfed3
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 15:49:21.2052
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vaCCdOUPKFWn4rUzK0i892JkwWBnjnHLq9X7UYgS3+6Cy1mLzifQPs0zfdciZqRcfplj/yX//rINcxTzWRa9zQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB7518

On 2/28/2024 8:56 PM, Sohil Mehta wrote:
> A few other nits.
> 
> On 2/28/2024 3:14 PM, Tony Luck wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/genpool.c b/arch/x86/kernel/cpu/mce/genpool.c
>> index fbe8b61c3413..a1f0a8f29cf5 100644
>> --- a/arch/x86/kernel/cpu/mce/genpool.c
>> +++ b/arch/x86/kernel/cpu/mce/genpool.c
>> @@ -16,14 +16,13 @@
>>    * used to save error information organized in a lock-less list.
>>    *
>>    * This memory pool is only to be used to save MCE records in MCE context.
>> - * MCE events are rare, so a fixed size memory pool should be enough. Use
>> - * 2 pages to save MCE events for now (~80 MCE records at most).
>> + * MCE events are rare, so a fixed size memory pool should be enough.
>> + * Allocate on a sliding scale based on number of CPUs.
>>    */
>> -#define MCE_POOLSZ	(2 * PAGE_SIZE)
>> +#define MCE_MIN_ENTRIES	80
>>   
>>   static struct gen_pool *mce_evt_pool;
>>   static LLIST_HEAD(mce_event_llist);
>> -static char gen_pool_buf[MCE_POOLSZ];
>>   
>>   /*
>>    * Compare the record "t" with each of the records on list "l" to see if
>> @@ -118,14 +117,25 @@ int mce_gen_pool_add(struct mce *mce)
>>   
>>   static int mce_gen_pool_create(void)
>>   {
>> +	int mce_numrecords, mce_poolsz;
> 
> Should order be also declared in this line? That way we can have all the
> uninitialized 'int's together.
> 
>>   	struct gen_pool *tmpp;
>>   	int ret = -ENOMEM;
>> +	void *mce_pool;
>> +	int order;
>>   
>> -	tmpp = gen_pool_create(ilog2(sizeof(struct mce_evt_llist)), -1);
>> +	order = ilog2(sizeof(struct mce_evt_llist)) + 1;
> 
> I didn't exactly understand why a +1 is needed here. Do you have a
> pointer to somewhere to help understand this?
> 
> Also, I think, a comment on top might be useful since this isn't obvious.
> 

Would order_base_2() work here? It automatically rounds up to the next power.

Thanks,
Yazen