Received-SPF: pass (google.com: domain of linux-kernel+bounces-86187-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <8ee24cad-e9b8-4321-aad4-9a9ba4f8b7b6@amd.com>
Date: Thu, 29 Feb 2024 00:42:38 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] x86/mce: Dynamically size space for machine check records
To: Tony Luck <tony.luck@intel.com>, Borislav Petkov <bp@alien8.de>
Cc: "Mehta, Sohil" <sohil.mehta@intel.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>
Content-Language: en-US
From: "Naik, Avadhut" <avadnaik@amd.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?ZitHUVQ1REZ5MEMraC90a2RSUlovNHVhYlpxTTdIZ3ZSV3FMWFZZK1Q5NjRH?=
 =?utf-8?B?MkphcGRJQm1UemtSY2NlcGdmSG94aFhTMDBlK2RaZElXZXdVVXVXSWU4QzYy?=
 =?utf-8?B?Wno3QkliT2x2Wk0vMU1JU05DZFRwUzY4Y0hVWTNkMVdLWTNaa1p5R1Q2V3ZW?=
 =?utf-8?B?VmhIelZUOWVZUWNQQThwMmYvRE8yUmdVUjZVcEFzUE1oaG9kVFljekpUMnBH?=
 =?utf-8?B?Z2p5ZVZ1L1RwOStmRHZ4aTdraVdGeVVYd3h5US85QXM4SzZaNkpTZHpaWm1x?=
 =?utf-8?B?MzVzc0NLRk9WQ1lmTTVuVHRBMHNMdmNnWXdPTWNKOE5NVXdzbDdDODM2L1FW?=
 =?utf-8?B?QXBmaUJxcVBsZ3FrbkZkTnFFZVZXQThHOEpZTFFhUWJ2TXd2eDJVWFZPb25Q?=
 =?utf-8?B?ZVg3b0RLakRYbm9VQWFKL0k5Z2ZPeHEzSXdKTGg2TE1NZXBHVWpjV29PQ05Q?=
 =?utf-8?B?SDBacERPV0VkbDJnUnkxRjNBNXFwSVBTNm9jbDg5VlhWWTJlOTBrTE1QZnJT?=
 =?utf-8?B?QUcyRE1aVFpTMUNVeWVTb2V6QlgyTWd1dlNtdkZUMlZwcDdzSHI0dzc5MklV?=
 =?utf-8?B?emtxVnZET09ja3pzVlhsSUtQb05ReEpGM1RhYlNPZXZxU3BXbWJ2MEVDRk5V?=
 =?utf-8?B?alFka2hlbzQrNU1ZdFFLT29pK050aHFEZGgxd0JGa1JNSlhoRG5ybnoxZXda?=
 =?utf-8?B?ZDBCQ1BUUnlNeVlyUVJzWjcwTUR5VWN6RWFmV05vYVFTYldsT2RBSFAyeXV1?=
 =?utf-8?B?bktUY2lZN3locytIQnVZUS80Y0c4Tk9OUFBCOHRFNkRZcnhiT3BKbmlEVXBM?=
 =?utf-8?B?TzAvWkppeVZUMkhDQUNudDN6R3ZONENjeHJVOCtXMXA2Z3RPY05LTnVEN01I?=
 =?utf-8?B?cEJoeEV0KzhZRDNxeklNWWU1S2QraWxYWGNnQ21GRDBTbWlkVXZJd2JSa3Bu?=
 =?utf-8?B?OVU1TndHYUp6cHR4ZzE4NC84NEkrWDJHa0k5cjVoQ2kzVTRvRlZzN3dVRW5i?=
 =?utf-8?B?MDgza3llcjlZYnM3cStBZWppRlF1VWlJR0prTTc0cktvNmkzRE1wYjIwRzFS?=
 =?utf-8?B?dlkxMHYrMER1SjRBN01mTjFZRWlVQ2pvMGloQTdKcTZKcFZZakZMK1hSdmls?=
 =?utf-8?B?SEE3SkNBaTNRczZ6T3N3L2NEWHU3c0p3bFp6OGhjVEw2ZFlVTDJkaUY2M0Y4?=
 =?utf-8?B?L25tdGVORmFqSG9TS3BtRlA2c0tCRDNkQkNLYzhEOVIwSFI3NnNzVElPZzNT?=
 =?utf-8?B?TEwwUzV1cCszZGtWdkFoZTRkZ2xnZDFuM05JM3JvcmMxcDgwM0U4NnNIQ0tZ?=
 =?utf-8?B?V0wwRGtoMTA3dGhETTM0QnFCTlh5M0VmbEM0bFFFajE4RjZDYWR6cm94bEtW?=
 =?utf-8?B?a3c4WnRFcFRZaFozOVpmSlVmTXZpdlZUQTBuQXBpZk5jeGFtV1pObHR5aE42?=
 =?utf-8?B?MzJzeFRqdlpGRFpndElFUitQNU1SVHFUNXd6MncwMnBHNzVvSmVqL1gxWDFF?=
 =?utf-8?B?R05jUytoeFlhbHFHNmRTU1oySU9FSDF0ZllabnZoL3N0blpiMmxSc09DSjVR?=
 =?utf-8?B?MVNzczczejJIQmVBT0pXNVZEcGl4N1ZHOWNQNEtPMzZaR01XOVo0QjJhZzNC?=
 =?utf-8?B?YUV2MERaNm5tZDhhSE05UFN1K2lwbmpubVBvOURiZkMvMFZQQStJam9NaFc2?=
 =?utf-8?B?WlhESTJ3TmdTS0xrdndkamwxOUlYM3RiSk1PSExZbk5jOVpZb25jK1MwandJ?=
 =?utf-8?B?bnVtWFpKT25EdElzTjRFSDRjdVZSM2pPU0ZPdGZNaXdqVnFXYlRQYTlXNEps?=
 =?utf-8?B?dm41U1lSSENVSW5PdytsdEU4bTJmVlgrdkNQblJMa3poV0lqV3J4OHE5ME51?=
 =?utf-8?B?TEdrdlk5c1k3OHRDeUJwZEZZaDhTbU9maEdqKyt1TzRnU0JzVDhmTjc5VHhk?=
 =?utf-8?B?cFdaQjZpbzZ1NFZqbUo0L2hiYWNaRkw1TG9EQ3IxMlI0b1crVUwzYkdnZ01z?=
 =?utf-8?B?V211R0NqTXVFcm5PUENMUlR6ejdvK1hPT1ZFeWozY3NuS0FZYlhPRzZ2RFVv?=
 =?utf-8?B?cXJBS3FmaFl2ZHFjVG5kMmJMWEFDLytXKzV4NzQ1akd4cFh5NjgrOTY4VHRi?=
 =?utf-8?Q?CVb23YwfeUD+CADJoiqtLXbK9?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 299f51e8-5a99-41f2-31fa-08dc38f1a074
X-MS-Exchange-CrossTenant-AuthSource: CH3PR12MB8403.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 06:42:41.1763
 (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: V+gVD1lp12LIQCMxxQpArEtP6dx3caLftQSGaZ3I+9Nj3bcpgeqasK9yKSBnDXlzAPJZ9PdohYN1oxMJv32Uog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR12MB9215

Hi,

On 2/28/2024 17:14, Tony Luck wrote:
> Systems with a large number of CPUs may generate a large
> number of machine check records when things go seriously
> wrong. But Linux has a fixed buffer that can only capture
> a few dozen errors.
> 
> Allocate space based on the number of CPUs (with a minimum
> value based on the historical fixed buffer that could store
> 80 records).
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> Discussion earlier concluded with the realization that it is
> safe to dynamically allocate the mce_evt_pool at boot time.
> So here's a patch to do that. Scaling algorithm here is a
> simple linear "4 records per possible CPU" with a minimum
> of 80 to match the legacy behavior. I'm open to other
> suggestions.
> 
> Note that I threw in a "+1" to the return from ilog2() when
> calling gen_pool_create(). From reading code, and running
> some tests, it appears that the min_alloc_order argument
> needs to be large enough to allocate one of the mce_evt_llist
> structures.
> 
> Some other gen_pool users in Linux may also need this "+1".
> 

Somewhat confused here. Weren't we also exploring ways to avoid
duplicate records from being added to the genpool? Has something
changed in that regard?

>  arch/x86/kernel/cpu/mce/genpool.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> 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;
>  	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;
> +	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);
> +	mce_poolsz = mce_numrecords * (1 << order);
> +	mce_pool = kmalloc(mce_poolsz, GFP_KERNEL);

To err on the side of caution, wouldn't kzalloc() be a safer choice here?

-- 
Thanks,
Avadhut Naik