Received-SPF: pass (google.com: domain of linux-kernel+bounces-86020-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <015bf75e-bbe7-44ea-a176-9f1257f56b81@intel.com>
Date: Wed, 28 Feb 2024 17:56:26 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] x86/mce: Dynamically size space for machine check records
Content-Language: en-US
To: Tony Luck <tony.luck@intel.com>, Borislav Petkov <bp@alien8.de>
CC: "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>,
	"yazen.ghannam@amd.com" <yazen.ghannam@amd.com>, Avadhut Naik
	<avadhut.naik@amd.com>
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>
From: Sohil Mehta <sohil.mehta@intel.com>
In-Reply-To: <Zd--PJp-NbXGrb39@agluck-desk3>
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?UXJBTGJKeE9yQjVOVUh1QXRjV04vV2I4eUhwYzBHa282aWhsd2ZEVGtFZEtk?=
 =?utf-8?B?c1pzdjhuVGl1V1QyUXlzdldpSUd5b0dBUkl5TEIwRHBOODNwS2JTZnkyVG1X?=
 =?utf-8?B?djFwRnlrY1hVcVI1YXFqbUdZam1uYWpPbXQxRWZyUWlQYk54WFZHVWtJM1ZP?=
 =?utf-8?B?dTBJT3pSakx6RlRad1dobG9pdkdhKzY4VlArYXREak9TazNGNWx5QXpwZURR?=
 =?utf-8?B?VDNyVk82SW9hcjhzNitsdlNKQ2NuVlEvMHVHNzlCN2V3RjB5MTIrQ0lZOHds?=
 =?utf-8?B?LzMyTm9xMEFJU3lHdXdCOGJUVFVSdWp0SXo3NERlV2FjMGFxRHZ2d1YzUjQ0?=
 =?utf-8?B?b0xSRHFVdXBXOFN4c21QMFJlZEZ5azRBcXJIUGRRZ3V2TmJJZzZpSEt2Qkdx?=
 =?utf-8?B?S1ZSM2s3ZDNtdG5CN3JhNm9aTW5peVJyR2VFcmNUS3I2ZG9sTk1MNzNDKzZZ?=
 =?utf-8?B?UDhYU21qYUFFMjJrWkh6ZHlCdVFJOFVsMGJLZi9iZDY1eXVMbmE5T3Bod1Fa?=
 =?utf-8?B?RGFndTJNY2FiQ0hCWENNNzZUZnVLMHA0eGIyU1dmZXZJbGFheDJZSFVxcW1v?=
 =?utf-8?B?Yko2VnJvbE12ZWd4UlRMN3dadi9zK2VnZ1BVUEdMclcvYy90N2NoSHZvcHJi?=
 =?utf-8?B?a1dSMlRQSVJqSXJEZFdwdFRLblpUMkRxaU1zcjhxVGU1S3FEc0ZqOFkwZ2RI?=
 =?utf-8?B?S2ZZbkp2d3p2RUUrdmlHNDNVNVAzTHc0U0FyeEo1cStNeTdYazVhdDcyRTVQ?=
 =?utf-8?B?cVNIejNTeTBhUVJ2WnowNmpyWU9EelFJVWY3bC9aMGVqeEhic0hqOS9nbG8w?=
 =?utf-8?B?aGt6T3FjUENsS0IxQWhsQzQrR00yL2trTXF0emNFTy9rUUt1YmlmNllHM24v?=
 =?utf-8?B?SzEyWHR6MVVmb3E1S1p4aHVTenRlcGUzcERJVmhZL2hXN3hjeDdWOG5ZOSs0?=
 =?utf-8?B?Q2xmSFBqQzllU2tINnZ6eUJYWXhldW8rOG05QlZLdnM3Mm4xWHViaFZNNE5P?=
 =?utf-8?B?U3RTTmRUamxWZzlkK2c1aW1EaVlneWxRTUFGZlpPM1JKTURPcUhFdnBjMVlq?=
 =?utf-8?B?SGU3Z3IvZWVBdkdSK085NjBmMzdhaWprVy9aU2ZhdFJPTUVqM2l4dWZCK0Vt?=
 =?utf-8?B?UUZKT2dzZ2w5VUd3UEdwWjlTbS9Kbk1NRFpGWUV0K1pQVkZBTHA0eloxVU1M?=
 =?utf-8?B?MFpYMitqWUlxdjBrbWppTW1DTTFtSElpZHUrOW1VS0d6YTcvQTlIRnpvaFQz?=
 =?utf-8?B?YzY1OGdEL2FKbG1Ta2IyMXZKTlR6dEdVaFNGaTFTQVZqWEM4RXM0dkFHellt?=
 =?utf-8?B?dlBQMW80N3l1Rzc4MmN6cDQ3VUM4SHhJWFZYczVQMU1Vb0FyZFZ3WUtFNVBo?=
 =?utf-8?B?NEROMnpTMHhzMlRnVTBaSWdhVTlDYWRXdU9sMG92bTJlT2RKM3R1Q3dvdFdr?=
 =?utf-8?B?Kyt1b2hGQnZIMi9KTXVydElmVHNDZy9zMXlnRlMzMW5hTFdWK2p3b1hRSTRN?=
 =?utf-8?B?b1lOZjhpNmp5ekdnLzQzeWNZb1o3d2FhOGFJUTFGdGRyR3BDTnVyREkyNnVQ?=
 =?utf-8?B?eXdKeEdZQ2VBYzJ1TTlFNEtEaWs0ZHlkTWJIZnRVbkpmdHJFK3MxQmJvOUtX?=
 =?utf-8?B?ZDBYN1cvQTFXL3ZDTkY4OFp4MGJaYUVVSkNTMnV0emFPNW9TODIzUWRXL2VS?=
 =?utf-8?B?bTk3MkZzZVJJcWNsSGdCNFBQdTZtMTVJcmhIaE5ZOXlvY3BhODg1M3VXZGpF?=
 =?utf-8?B?UFJ4VjF1VWp5Z29rN251MGF4OHRDZUVVM1F3Mjc1WjJubHRRU2J5VWo3amt3?=
 =?utf-8?B?Ukl3MTdxRWhZcEhHN1ZhbnNQWHR1UytxbEZ3bjN4VFhmREZHWUhXaHc5NmtQ?=
 =?utf-8?B?anp6M0luOWZMVExaM2EvV1IwNEVTMHJveFVHOVU0dk56Y0FRV3dEcUJUdkVG?=
 =?utf-8?B?eDd6VUlBMzgvSmlROW5UbFpGamxua0VBT0gvR2JCa1cxaElsbFY2SGNwZFBE?=
 =?utf-8?B?VWViTVQrY3RMRWpvYjczUkQyUU41cHFSUC95cWtLMDZhdmRSRTRETHhQczl4?=
 =?utf-8?B?UVdlWElDS293OGZLN1RmU3NCc2ZUSURkeEZwd0RWNS9yaG93VzYxeU4zbzBU?=
 =?utf-8?Q?HbjxwKdBBpXzAwiK3brz5yBAc?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8a03fcc7-9983-433e-e2fb-08dc38c9a453
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3320.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 01:56:27.7510
 (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: ZmmesgZdtgY7HY6pG6yqbBeN+Upt2ocZ2IiyQ18nQH4HFYZ8QR94RanjVbxrS62MrwBRTehNXXSl4geoTfwY/Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4809
X-OriginatorOrg: intel.com

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.

> +	tmpp = gen_pool_create(order, -1);
>  	if (!tmpp)
>  		goto out;
>  
> -	ret = gen_pool_add(tmpp, (unsigned long)gen_pool_buf, MCE_POOLSZ, -1);
> +	mce_numrecords = max(80, num_possible_cpus() * 4);

How about using MCE_MIN_ENTRIES here?

> +	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);
>  		goto out;