Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <4d6c291b-9b10-a4fe-6486-14b0a0e9ee86@oracle.com>
Date:   Tue, 26 Sep 2023 09:55:31 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v3] Crash: add lock to serialize crash hotplug handling
Content-Language: en-US
To:     Baoquan He <bhe@redhat.com>, linux-kernel@vger.kernel.org
Cc:     kexec@lists.infradead.org, akpm@linux-foundation.org,
        vschneid@redhat.com, sourabhjain@linux.ibm.com
References: <20230926120905.392903-1-bhe@redhat.com>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <20230926120905.392903-1-bhe@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UmhEYXlpWTY0U2tSSG1qYnBmZG5Da0lnVGJsQlFpMTQ4V09PbGw2bGNPNzNG?=
 =?utf-8?B?aTRYUmtldTREZUR4ZXZndFhmUkpMY040NGhrclFpK0p5WHRDUXZoNFJFUlRn?=
 =?utf-8?B?QWNEYWZtdFBlY0pyTlgwYWFsY3JOT2hpWGFqb0VwZjlGUmpUR2ZNTlpvanky?=
 =?utf-8?B?ZmJicUp3VmU0NUdSV3B1SUJXaDVhYjFZSFkrSzRaQmlUdDJ1WWZiR2hiWkJs?=
 =?utf-8?B?UURsNHAwNlJieWFIVFdOVGpCUFRlck16am9FcmRwQ1BsS1B5UFNqQUxBUFMz?=
 =?utf-8?B?d0JVR1k4NFVxR3A2SFdHb3hIK0Y0YUtpNW4wQmFweVRrOVpIZlVwR1hEdno2?=
 =?utf-8?B?Z2FFUTJrOW4yZXRnU0FhSVJJbDBTb1YrSTRkSlR5emJHQUh3NzZKcFY3YjJK?=
 =?utf-8?B?WTEwd05QNUhzTGJuYUdhTVl0TXJITDRFa3kwMzVBZTh2dWl2d2l3dGZnM0RP?=
 =?utf-8?B?MDQ3MERDK0JUT0FRN3hoSGVRaDVtNzZTODRyMVpuTkpvM2hMUmFoWStYU2RV?=
 =?utf-8?B?dWEzOG1HVVN2cDRXbTVrbGtVaCtlQTdsMTNvZERXVUVMVWRYTGQ4OUhYdk9m?=
 =?utf-8?B?OE9TcC8rT3FlRVhndHJnRUxLZUpKU3c0czRUMDZ2bUN3TjhuUVZDbTZaWFhK?=
 =?utf-8?B?bUZBb0VSeUl2NHBpeGpMR3R5MExEeWkxWk5hZXY1N2tKR3UwbGpzb0Jtc2tL?=
 =?utf-8?B?TDJxMDZwY3FmN0g4NjNMN0xXSW5PaXBMQkltMnVnN2NEUU1Gc2dRWnJGcTBM?=
 =?utf-8?B?bjZucFlaK1JoK0ZpM3Z0RDFHQ0hoZS8zTGRibXc4ays2bnpMQy9MTFgwWjBF?=
 =?utf-8?B?Q2xoYXg1aE5nYnFGQjA3T2V1NGJYV01IWjlQeldJZzBWRkJCT1N1eEIzWWVX?=
 =?utf-8?B?N2tCdzFCL25JY1N1TzVpaVpXa2xXcnFkN0JSTEVQaUk4OW5FVzBZNVRTTklh?=
 =?utf-8?B?dzZPT3RIMUoxeGZIS05Wc2JZME9IVGozSnlVQWk4TGduYi9IV3c4TmZnKzdM?=
 =?utf-8?B?R2FRQytZS2k5Q3R3VW9yeWZ0aDYxSGx3RVg0RG1jcnJES2EyY0RIRHpLWlA2?=
 =?utf-8?B?R0NuTnd2Z0pUME9oQVpMMm9pZWdQc0dGeG1XWlY0N25nQ1ZkNU52ekpmNmlF?=
 =?utf-8?B?N2lrUVBqSi9lUC84QVA5dWVGaXVCb0Q2NTMzVS83MFlSNS80R0dza0JQcGU1?=
 =?utf-8?B?WXV1aXR3aXE4djUzWkh1ZWhUR2EwdlVqT0xZdGZkUE9tSlpsT2tLUDhHYzR0?=
 =?utf-8?B?ajN5bVRjU3NrV3NOTS9mM0JjVzdNQUVDdnNuMElsbTFDN0gxMkliSWNxVkdr?=
 =?utf-8?B?UG83eWxLOUF0elkyc2ZkY1dtOEEwb2l6cjgxSDV4VWNtbC9TQjNrMzhTMm0x?=
 =?utf-8?B?emozd0tFaEM3SGp2amViNG1UVWo1OWszbWJkVG12K0lINWtTVkRzNkZ2ems5?=
 =?utf-8?B?SkFZYnpZdE1GU3MzMWI5Vk1wRmdiRWo4Z0Z6L3hHYVVTNVhlMGhaT2pjVHBx?=
 =?utf-8?B?cGRXZUNod1BTL0c2V1VBditwbnB1aXVuOUQ3ZDRYMGdoaWQ4c1JaMnI0ZExh?=
 =?utf-8?B?M05VQ1k2ODVPaTB2czJ4SUFIQnFXb2UvVys5U1dsYldNK2dNRG44T1NYYS9y?=
 =?utf-8?B?TE1BQ2UzR2x0bFAwa2cvcTlibEdHREhick9admtQZDgzZUdLSlVJYTZjQTRq?=
 =?utf-8?B?S0lqNVYwc2wyanZqaU9JdzR3TGNRR3VnaWlLL1VyY2pKMlFVdEppaHJQSklz?=
 =?utf-8?B?aEF0UzJSc1c1VEJuMDl1NXh3amVkb3ZTQ0RkcUROdmgzNFBiekpjWU9nS21P?=
 =?utf-8?B?c295Mm9FY2p3LzVIQ3ZIbUk0Zy9scWhDd1hwdDRhczBjNUZaYnhtU0hZSkVh?=
 =?utf-8?B?QUVHOFNya0pNQ296UXkwQXNGbzZUdzljWG13M2YySis4clRZbWs1V2lQYTBp?=
 =?utf-8?B?K0dSVEFPQ054eGgwMW1raDM5YW8ySEEvMk9nbklLd0lucXc2d2NwZkJLNXgr?=
 =?utf-8?B?VW9IZUl5eUo3UXNBUkNxQXVEV2tWbUxuSFZ0QjE5dkdwdFJueDRlRnQ1Vmhx?=
 =?utf-8?B?ekZ0ZVpRenFtMS91VFQ5YStNSXJ5Sk0yYXJ1RFBZQVB0cjVMMnJjV2krRk5q?=
 =?utf-8?B?UlIwYm5MSEkwNCt2UysrVDBOci92cVNIdTJJeTdVUUZFMjNQNGtreHo2akZi?=
 =?utf-8?B?YUE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?eHhDM2hleUd3c0U5STh2d2FaelhRREYxNDFlVURUM3VBcWNjLzFiRlVhVDVH?=
 =?utf-8?B?OGU1T1ZMOHFVRTd6Vm9EcGhjMmdSZjRMOTArWU1USWhvWVlIVWpzVG81ajZ4?=
 =?utf-8?B?aXRXNEtrb0lRRjVkZjVrVnBhaW1UZ3pBaFVvN1NsQ21OaHdkaDQ5RHZFQXRj?=
 =?utf-8?B?c3B4NjNTMU0vTndzZ2NLR2NTcEVyL283ZnFGNVBFb1NWaTJhVm0zM0pJeEF4?=
 =?utf-8?B?dkp4Ynd4dENBQU9MMWZPZUluUmw0TmdwUGJJNkZ4ZGt3YzR1SndqMXdNYytS?=
 =?utf-8?B?V1cxd0lxNm1SbnoxSHlod1M2cWlNNU9HbE43N3VnaHB1WmNUS05oWGJuVHJF?=
 =?utf-8?B?UUUyQlJWZTZTOVFISUN0VTVlOHV2d0Z2cDcyV3h5RkNHR0RsbVF3SkkxSVhX?=
 =?utf-8?B?bnpLSzhld1NKRCt6TWtuT09EenlhVEpVOXV5R3I4Qlg2THZva05SSENkTVF1?=
 =?utf-8?B?QUlybXpEM0pybkFtV3I1bzBaNHNMam42cmJPb3NENEZnbGhqakpmcS92VTVn?=
 =?utf-8?B?MmhpTmx0SWRZUWNYeWRnYUo1MkxmamNzc2RSTlAyUlFjQU90eEtwNjJ4ejFM?=
 =?utf-8?B?TWs5RjR6MHJKMGR1WVZ0RkxJcVllVnRtQjZ1eGRYUXhyK3dlSzFiN1o1ZHJ4?=
 =?utf-8?B?Tm5TRjF6dmd0YnE2UU9tTjlmUmhqTEsrMkJtVnplODgvcGV1dmpIM2ZTWUpm?=
 =?utf-8?B?c0ErVGJhWmFNMkNJYmFpSGdCWUR3R284V29VR0Rrak9vMXBvdyswZktNZDli?=
 =?utf-8?B?eTVOdmRCcUg5aTJ1QVVzZ0ZiUHhIcTIzRmZ4RW9pVGVpVTROb21XMU9nWWJW?=
 =?utf-8?B?R0p2aW94NlRTTFlQOWg3QkJsdG1uelJHT1Q3ZHFhQWdZWk8zZlIzRDNyQWZY?=
 =?utf-8?B?REs3U2FEd3lHVHlldGRUeWhCbFI1NVZ1YXh5SnZiREFpaDdPbnN1aUUyN0pT?=
 =?utf-8?B?N09SRnRGcXVOR0Y0Tmw4MmJiT0FZQ3BGL3hPK2pCZ1hsbFVXTEdjMnNKOWtP?=
 =?utf-8?B?MnlQVkRWcDV3TFltSGw3aHFjNlAzOUVPdTRKWE12NVhKOGY2TVdZQTlabUxK?=
 =?utf-8?B?akpzR0E5blFPekx0dmZMcWpyM0plYmlkWGFXek1PdkRsMXR1V0g1RytCVzQr?=
 =?utf-8?B?aGFTa3g0RWwvTGJpTTNHd0pWYWVXVHYyWDdxd0pUNFhqelBNZUhXbU5zLzg4?=
 =?utf-8?B?b2pEbERiYnVhcDBOam9YdEhFSHVRNU9iS2dzcWJkajVBWkNLUmVxdkJVRlhT?=
 =?utf-8?Q?kna9VEMIq3dsG80?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 199905b6-459b-4a7a-52d2-08dbbea0a30e
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2023 14:55:34.4448
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TzdM0nPRj2jtH1kcYiVI8n/LjVar/jZu9lF0hpBKSR/vYB1PPIGoJ9Q7bT3LScP5PnQ/R8KAuYbNbPHxjga/vMnAU4kKd3+EWWWwc6p2v0k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4162
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-09-26_11,2023-09-26_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 bulkscore=0 mlxscore=0
 mlxlogscore=999 suspectscore=0 phishscore=0 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2309260131
X-Proofpoint-ORIG-GUID: 8lDC5PTMsXImAtNcudJBUH1sN4EEeNrE
X-Proofpoint-GUID: 8lDC5PTMsXImAtNcudJBUH1sN4EEeNrE
X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Tue, 26 Sep 2023 07:55:56 -0700 (PDT)


On 9/26/23 07:09, Baoquan He wrote:
> Eric reported that handling corresponding crash hotplug event can be
> failed easily when many memory hotplug event are notified in a short
> period. They failed because failing to take __kexec_lock.
> 
> =======
> [   78.714569] Fallback order for Node 0: 0
> [   78.714575] Built 1 zonelists, mobility grouping on.  Total pages: 1817886
> [   78.717133] Policy zone: Normal
> [   78.724423] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
> [   78.727207] crash hp: kexec_trylock() failed, elfcorehdr may be inaccurate
> [   80.056643] PEFILE: Unsigned PE binary
> =======
> 
> The memory hotplug events are notified very quickly and very many,
> while the handling of crash hotplug is much slower relatively. So the
> atomic variable __kexec_lock and kexec_trylock() can't guarantee the
> serialization of crash hotplug handling.
> 
> Here, add a new mutex lock __crash_hotplug_lock to serialize crash
> hotplug handling specifically. This doesn't impact the usage of
> __kexec_lock.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

I've run this patch in my regression environment and I do not see any
lock failures! And I've done this with a variety of DIMM sizes up to 8GiB in
order to vary the "size of the swarm". Both with kexec_load and kexec_file_load.

Tested-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
> v2->v3:
>   - crash_check_update_elfcorehdr() need take __crash_hotplug_lock
>     too because there's tiny racing window when kexec_load interface
>     is taken. Eric pointed out this.
> v1->v2:
>   - Move mutex lock definition into CONFIG_CRASH_HOTPLUG ifdeffery
>     scope in kernel/crash_core.c because the lock is only needed and
>     used in that scope. Suggested by Eric.
>   kernel/crash_core.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 03a7932cde0a..2f675ef045d4 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -739,6 +739,17 @@ subsys_initcall(crash_notes_memory_init);
>   #undef pr_fmt
>   #define pr_fmt(fmt) "crash hp: " fmt
>   
> +/*
> + * Different than kexec/kdump loading/unloading/jumping/shrinking which
> + * usually rarely happen, there will be many crash hotplug events notified
> + * during one short period, e.g one memory board is hot added and memory
> + * regions are online. So mutex lock  __crash_hotplug_lock is used to
> + * serialize the crash hotplug handling specifically.
> + */
> +DEFINE_MUTEX(__crash_hotplug_lock);
> +#define crash_hotplug_lock() mutex_lock(&__crash_hotplug_lock)
> +#define crash_hotplug_unlock() mutex_unlock(&__crash_hotplug_lock)
> +
>   /*
>    * This routine utilized when the crash_hotplug sysfs node is read.
>    * It reflects the kernel's ability/permission to update the crash
> @@ -748,9 +759,11 @@ int crash_check_update_elfcorehdr(void)
>   {
>   	int rc = 0;
>   
> +	crash_hotplug_lock();
>   	/* Obtain lock while reading crash information */
>   	if (!kexec_trylock()) {
>   		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> +		crash_hotplug_unlock();
>   		return 0;
>   	}
>   	if (kexec_crash_image) {
> @@ -761,6 +774,7 @@ int crash_check_update_elfcorehdr(void)
>   	}
>   	/* Release lock now that update complete */
>   	kexec_unlock();
> +	crash_hotplug_unlock();
>   
>   	return rc;
>   }
> @@ -783,9 +797,11 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>   {
>   	struct kimage *image;
>   
> +	crash_hotplug_lock();
>   	/* Obtain lock while changing crash information */
>   	if (!kexec_trylock()) {
>   		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> +		crash_hotplug_unlock();
>   		return;
>   	}
>   
> @@ -852,6 +868,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>   out:
>   	/* Release lock now that update complete */
>   	kexec_unlock();
> +	crash_hotplug_unlock();
>   }
>   
>   static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)