Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <8515c858-4be8-fbd6-7868-c8bfb5492f83@oracle.com>
Date:   Sat, 23 Sep 2023 07:10:36 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] Crash: add lock to serialize crash hotplug handling
Content-Language: en-US
To:     Baoquan He <bhe@redhat.com>, linux-kernel@vger.kernel.org
Cc:     akpm@linux-foundation.org, vschneid@redhat.com, dyoung@redhat.com,
        kexec@lists.infradead.org, sourabhjain@linux.ibm.com,
        konrad.wilk@oracle.com, boris.ostrovsky@oracle.com
References: <20230922235446.260966-1-bhe@redhat.com>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <20230922235446.260966-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?eG1WSGRCWm9NM3c3UHA5QlZIbkttT2dnY3ZTZ3VlMEF5MUhaMm9mNnBrcFFV?=
 =?utf-8?B?Y0JrK2UwZnlrQ2M3NG5hT3RrMnpERjUva0xNaGdudGJlUW9id0RUNlhrazd6?=
 =?utf-8?B?T0tYMUpYV2lMWlNrTzlYQzJVRHRkeUg2TEdDeE16WU1rQUZyRkFKaXdWcG5S?=
 =?utf-8?B?WTFyeDFPdTJsUjUwWDdZcEZCenVtSnpQRGt5YzlHZS8ySGdLWGJyZmdFclB2?=
 =?utf-8?B?Zm9kOTFnQ2w4SkJTTDBReFo4QmFEMkRSYWRqVU1ZZ3hrakFDT3ZQd3JVWTFT?=
 =?utf-8?B?VUtSRVh6cERlUUgrUjYwTUlJN1dSTVBYcENxRi92dUJoTHdCOXc5R2ZjY1A1?=
 =?utf-8?B?Ry9XTmgzU2FsbFVCQ2RDNUlxRVZuUithRTFmL01pekg1Mk40SUZ0MUZ1VS9F?=
 =?utf-8?B?bFBvLzltSU5hY2ZuZXlmendDWTJ5ZnFNV2QzNEU0bkZJUXhVb1M5ajFRUFRH?=
 =?utf-8?B?bU92VE1nbEZlWE8yell2MVFxTCtDeTZ6NTNISnY3dStobVhWbCtWazNzOFJF?=
 =?utf-8?B?V2dNTWlFTURaSDZLNVAyeEFZZ1QwaTVBalpjdUdxNzE5WXhiM1U1M2hxUVdW?=
 =?utf-8?B?Qmd3NHY0NEpxeVQxd0o3Y3JFMC9RV25NcWs5dHFqM2FQNldwRW1mcmVNQVNv?=
 =?utf-8?B?MVBZMXAyTUo4UG83NHMzWXZLU1NYMUVoK1MrUmJ3RzltZmFnYzY5OUNQbWIx?=
 =?utf-8?B?M2ZGdHJqZWwzWXZPZVhpRW53ZHVRT2hVc3pxTi9tSjlTUDYxbVY3RGdnMVBV?=
 =?utf-8?B?NVVKQSsraXlXaWVDV1Rqa3ZqNU55bkdPN2d0ak1LaHd4WXExQkRFaTZnVkl6?=
 =?utf-8?B?OHlvemhnQUhORGVmTDBDbW5zcWVBT3JzMXJaTjN6ajZOU2V6RzIxT2tGMFBZ?=
 =?utf-8?B?bWxjSkkzMC9lNGRBN2M4dlZVdHpVL3hLei9tYzlNdkp5K011cjRpSWc3emFy?=
 =?utf-8?B?ZUwrZjg2SzIvZjFORmdQM3piY25rbmFXSnNGemMwY0doUkFZUzMxMUx1ajVt?=
 =?utf-8?B?ZUIzaWlYM29nZXk3RkEzY0tVRkdIV1hZZjg5ck9EOUhWckRjTkF2WUt4Ynpp?=
 =?utf-8?B?emRIWDhaSnhia1FXeEt0U0VyWHB6YmY5WjJyckFra3JJdVpKRG9HdDhIcExx?=
 =?utf-8?B?NkYwS0FMNk9UelA5SHNxV3ZNMjlEUjlWY3dHV2s1SCtubkNOeVJUbU9nOFdr?=
 =?utf-8?B?aXErSndEVHBxazVNU2s2OXYyd1FibDA3ZFRSanlzY1ZETWlMdU1KemQrM0FE?=
 =?utf-8?B?QUFla3JRSVAvRmpkdnlTZW0yeUYyYjVaejJpTmxQbkRIampzc0dTbjhUbTdO?=
 =?utf-8?B?WFNpdlVTU0RPZ1pQN1NweG5Sa29JWjF3R2tOWDBMYm4zSVNaSmRTUVVobm5y?=
 =?utf-8?B?R0J3WkdnRmRBZkd4N1FZQ0lpVVJKSmdWcnpOWkxqTWZycnBGRCt1Tk1UOXAz?=
 =?utf-8?B?cFA2S0p1RWJIRXZCUDJaemEvVGpka0lZcGw4WHVtZktnYlk3RlRJVHY1RHBC?=
 =?utf-8?B?QmlONXlyUit1LzZ6ZVhMWkhCNWV6WFRDb0o1WWJwSXJNaVVSdnVMZUNOUEZm?=
 =?utf-8?B?cW81ZExEdUhuUE8wTnRwbHVVQXZxZFBHYUxHVjloSklaOU5nWXZXK05rbW9s?=
 =?utf-8?B?ZVR4YnNOLzVCQ2tiNXo1eE14S0V3ZHBQNHBWbHVNcEhpUWxzaEEvSVNYSlJX?=
 =?utf-8?B?bnpNMS9PVFhHT2NSMFo1OERUa0NKQTMrSWJZSk1zVzdveFBJMmprcUpYK0xh?=
 =?utf-8?B?eEdESDdtdWNkZGJNUnl6RXY0TEFwNXp5enZ3MFU3QTBvcGc3ejByd29POWNI?=
 =?utf-8?B?U0todE9nU25NVkV4VEFyQ1JVMTFWd3pHZ0JrbGxYaXdvTmxWY1ZEc0R6cDNM?=
 =?utf-8?B?YW15QmVwNVh2dkE3aXM3ZTNkQ0V5amtQNHk5dm0rajc1OFlKNlkzR0ZzU1NV?=
 =?utf-8?B?RlRpVXN1MEdaelFGUjBMbTJXS2VidjB2WWp3dlFYUGpvZGROdUFWUUpwUzQ5?=
 =?utf-8?B?cW95Ky9UaE5PdUN0bzBnR2RrRjN6RTltSkVjR0hjS3lmcEsrdnpodm5NWDc0?=
 =?utf-8?B?ejM4eTkzb05ER1c5N0pFejZpYVB3MndCVTk3ckJRZ1h5MzNPUXVqa3JuMW54?=
 =?utf-8?B?VXA1QUd3cE9qWTJuNmJGN3JQbDRTM3FQbU96SkhIenJsSjBWZ2VxcEdlajNy?=
 =?utf-8?B?aWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?bWVTL2tQMjhib0xVOGtuSk9wSjNsRjVIZDZmN3ByajFMOTRHTm5VQkl0eFZQ?=
 =?utf-8?B?d3RScDd1eXlzM2hQaFVnZDBxRFZBT210d0FvY2tpaWxjcUFITlZSTFJxaXUz?=
 =?utf-8?B?S2gvSGxSZFI2QlhFWEsrNEdDV0h5OHBMekVSbHMwZERxVGNsaGNiZ29HTmlL?=
 =?utf-8?B?VWxoUTc5TGlRTmt6dzhxNkQrSzN4d3haQ3pZb1p1YWxOaW9hNDdwbkRCT1RL?=
 =?utf-8?B?Mmw0QXB2b1J0MnU5cnpiWVVsUHA4eHpZRFlkMjFNQWRvVDJnS2RIbEoyR0FH?=
 =?utf-8?B?VnQ3V1pqSlprNUhjNGZKamJ3dForc00yK3JYUzlTUjMrcVNSa1dhMzBWZEcx?=
 =?utf-8?B?bUR3eFFGSUVBMjhlM2h6a25hT1JmUnlURXNsa0QrcERsMVZHYUZTWllHcXQ2?=
 =?utf-8?B?TnFLcTVDKzNMZXN3R0xFbXdhU3J0MFlKdlZiUEpiMk9UUU5CaFVTeVNOdGli?=
 =?utf-8?B?MmF6NnFlZ1ZhWm85K3ZrTGxLN0U3d3pxS3RURFozN0tNdzhjVm5sNmRGVEVC?=
 =?utf-8?B?N3d0b0FVZzhQd25kZCthZ1dpeExCZ2tHV2NvZnRtMXB6MUpyZ1ZEZWhrSFpi?=
 =?utf-8?B?SE1WS2dsaXRZZUxFazVKTHpLRzRsNFJjaW5Ub0dkcGZUeW9XOHkxNGlGSVhv?=
 =?utf-8?B?WlhkdUQ4T1B1dUp6WGR0ZHFLNEl3Wnd4SnArNnBsS1I3czlFZ1BQQVRRcSt5?=
 =?utf-8?B?enB2UHhZTXNhL3laRVI0ZWpIQ0VLYXl0NDJLNWpMc2pPNzAzV3VyQStwUFBT?=
 =?utf-8?B?cC8wd05WT2FKdUxDdnIxVVd1QXE3eDJjSFNZWkw0Ny9yNUtmMEIycEloZkc1?=
 =?utf-8?B?cnRTcnhHaGp4K2RpZmsxTC9WZzZEM2RaNzNXRGZIYVZGQm1zQ3NwWTg3RllO?=
 =?utf-8?B?ck54TXRmR1Q4UmFYK0VXM2piSHNTZDhQN29UTkltNjJBaUJpNVZmenY3OE10?=
 =?utf-8?B?WmVoTC9jMzJnN3dzeHR4TnpNNnc1dFp3Z0tkd29mb2txMUg3SGUxNzFjMkZk?=
 =?utf-8?B?UFJEZTBGOUp5b2IrSHFFb3RJdGtUTWJhKzkwL2VuY1ovUmZhTWRMV1VHczlO?=
 =?utf-8?B?YmY3WldJaDFZMjVWMUlwaXBkN3VzZW5abmhaLzJTTHovUGIxdHUzV3hnZEFm?=
 =?utf-8?B?MTZqSXhjUHR0N3E1MlB6cmdWU3V1WVlMMi8xKzdiQk9EemZoMnIyZXVDTE5Q?=
 =?utf-8?B?Tmptc2gwMWdtKzRsVlB4Y1huZWY3MzMvNWxXMVVLZjNOa21SK1FORy85NmQv?=
 =?utf-8?Q?UACKr2H36jRaYRj?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4c882b1f-1394-4f6d-cb17-08dbbc2e1d46
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2023 12:10:45.2389
 (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: i3il8tz+rRPN2fVmVsuDGiteXzpcLzMOK8ncsVO2/hbajWVj5DAQGV7g++fZe1MW71Lc4o9ryzgA8lV3oB2rWn+j8etJAvuKzPdNTB/5iuQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5671
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-23_09,2023-09-21_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 mlxscore=0
 adultscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2309180000
 definitions=main-2309230101
X-Proofpoint-GUID: hZUkjomhNPxaOOgaGfXhxV5H9A3nBp55
X-Proofpoint-ORIG-GUID: hZUkjomhNPxaOOgaGfXhxV5H9A3nBp55
X-Spam-Status: No, score=1.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	NICE_REPLY_A,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS autolearn=no
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
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 (fry.vger.email [0.0.0.0]); Sat, 23 Sep 2023 05:11:12 -0700 (PDT)
X-Spam-Level: *


On 9/22/23 18:54, Baoquan He wrote:
> Eric reported that handling corresponding crash hotplug event can be
> failed easily when many momery 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>
> ---
>   kernel/crash_core.c     |  3 +++
>   kernel/kexec_core.c     |  1 +
>   kernel/kexec_internal.h | 11 +++++++++++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 03a7932cde0a..e8851724a530 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -783,9 +783,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 +854,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();
>   }
>   

The crash_check_update_elfcorehdr() also has kexec_trylock() and needs similar treatment.

>   static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 9dc728982d79..b95a73f35d9a 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -48,6 +48,7 @@
>   #include "kexec_internal.h"
>   
>   atomic_t __kexec_lock = ATOMIC_INIT(0);
> +DEFINE_MUTEX(__crash_hotplug_lock);
>   
>   /* Flag to indicate we are going to kexec a new kernel */
>   bool kexec_in_progress = false;
> diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
> index 74da1409cd14..1db31625ef20 100644
> --- a/kernel/kexec_internal.h
> +++ b/kernel/kexec_internal.h
> @@ -28,6 +28,17 @@ static inline void kexec_unlock(void)
>   	atomic_set_release(&__kexec_lock, 0);
>   }
>   
> +/*
> + * Different than kexec/kdump loading/unloading/crash or kexec 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 specificially.
> + * */
> +extern struct mutex __crash_hotplug_lock;
> +#define crash_hotplug_lock() mutex_lock(&__crash_hotplug_lock)
> +#define crash_hotplug_unlock() mutex_unlock(&__crash_hotplug_lock)
> +
>   #ifdef CONFIG_KEXEC_FILE
>   #include <linux/purgatory.h>
>   void kimage_file_post_load_cleanup(struct kimage *image);

The new content for kexec_internal.h and kexec_core.c could/should probably be
moved into crash_core.c, within the CONFIG_CRASH_HOTPLUG?

eric