Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7b25449b-72e9-3eca-73a9-592d7400b746@oracle.com>
Date:   Tue, 18 Apr 2023 08:55:51 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v21 2/7] crash: add generic infrastructure for crash
 hotplug support
Content-Language: en-US
To:     Baoquan He <bhe@redhat.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, vgoyal@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, nramas@linux.microsoft.com, thomas.lendacky@amd.com,
        robh@kernel.org, efault@gmx.de, rppt@kernel.org, david@redhat.com,
        sourabhjain@linux.ibm.com, konrad.wilk@oracle.com,
        boris.ostrovsky@oracle.com
References: <20230404180326.6890-1-eric.devolder@oracle.com>
 <20230404180326.6890-3-eric.devolder@oracle.com>
 <ZC6nWzPuIWOxmvv2@MiWiFi-R3L-srv>
 <80767ccc-ffd4-9cb9-44e4-a8d4f0e13853@oracle.com>
 <ZC9cp5RLyNNx0DMG@MiWiFi-R3L-srv>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <ZC9cp5RLyNNx0DMG@MiWiFi-R3L-srv>
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?aWdjaldmS1hldUQ1TVpLYlVzNnB2VGNSald6TFBFTnU5ZGE4MGh6Uk5kOFho?=
 =?utf-8?B?eVUxanVkTGRlVjRBTzhnK3o2MDFKdElqVUxCbEJYdlRxNXhQRDVMd3E2QXpq?=
 =?utf-8?B?SXVGay85NkRHYTl5MFdLMUZYYzBsMGxnMVhxWkZTQmtIMTBNWEk0cFBFQkpY?=
 =?utf-8?B?dU4xOEtSZys3L0MzR2NYd3J5TDEwdXN6d0Vqa3QrNFRLK3Q2N0RDUXFEb1pB?=
 =?utf-8?B?dW1JbVJkQXFoSG05MzdOdUtpZTUyQWowZkhyQzBqRnVpb0J6TThTWDE5d0FI?=
 =?utf-8?B?V2dQZmZFV2w1QUhJdnF6MnJTUVRRVjFRM28wbEJncUcxVWYycnR5ejNYUW5E?=
 =?utf-8?B?V3JQZW4zZjI3NUE5Mjh2S215RDBleDRCUlE0R3E3djVFTW12cHlBQ0VFVjRJ?=
 =?utf-8?B?ejArR0MzeWZpeElMWUJFYzBSNTRQTnBodHc2OEdjZWp5Sm0wTXNRVTNjWW9V?=
 =?utf-8?B?NkhqQ1FkazJVUVFzSi9BdXBMRHBTNi9EN1RXLzNIL2IxOEUra1BqR0RuRGkw?=
 =?utf-8?B?QTA3a3ZuNEozbDEvanY5cjI2N1E1cHpRTVdvL3BZWUNzT01hMzNuU2JFZVU3?=
 =?utf-8?B?SW5lKytxUFVZVkR5eFlMcVQvUVFxdmN6MGNabUswd2hIKzFGRkJUWUtMaDla?=
 =?utf-8?B?RlNXanhnNkJFZllLc05DM0JkV3VPQnJuZkZ3M0pkK0lxTHlRamJyZnFaei9R?=
 =?utf-8?B?RXd1bDdhbUhqL2ttWWN2d1lGTTMzMEF5SHVqaStaNEYvOGpwSUxDR2ZwUHp4?=
 =?utf-8?B?cXk2YTIzYlg3RG9Ua05jZzdkaytGK1U4eHBtTldoemR3ZjdBdFhWUE83SWt0?=
 =?utf-8?B?T1ZRdDFLZ29pRzFwcjJNS2RmYmZmUStENlNzVG1vQzhQVG5nbXRaQVprWW9n?=
 =?utf-8?B?bVdHQU5Sc3BDSHpzVE1nUUZINVdEUDNzMmlrdnRMcm8rQkVxQVorNXBHRXMy?=
 =?utf-8?B?L0pINjFxUFB0dEJxdm9rVXlaVVZnU2hMdzNXTFZ1U2hNUGJKTVM1S1RvY3Q0?=
 =?utf-8?B?dklPNHpYaDBOSXl3c0d1QTRMMHo0SDZqZFZIRzVnOHVXb2FXRTdrTjNzNTVE?=
 =?utf-8?B?MFR3OE9yclByVmRHUjdiL1Q0UXAzZUVyRWdiN2FhYWw2L3NiR1ZzVzR0RnRw?=
 =?utf-8?B?aFAyWG15V1FGckxBbllka05ZQ2JXcVBLY1ZEWVRuWkxocU9lMkJ5ckZONkJk?=
 =?utf-8?B?RWNnUTY4a0FUNFN4UGtOa2dUbzlUeVNHS1Nsc1RSV25WbXorNkR5aDlIeHhx?=
 =?utf-8?B?YkpTVWczUGtNZVZqL3ZGaHlkWFl6YTRjRXYrUi91TVJyblhoMXVuWFNucGRO?=
 =?utf-8?B?S0lLY1RKbzdMS3hBNERFZ1B0eVhkc2JjQ3Y5U2RkWDJwbnZpb0VLK2t2UVhJ?=
 =?utf-8?B?YWwxWGFCYlp5TEZkL2haL0s1N2dxTHAvaDl2YWF4NVd3bVRPa29mSFF6OTlW?=
 =?utf-8?B?T0s2MUo2a1Y3MFdTakx6SnRRVnVvdEM0cDRaa0RXZTdzNlIweFdWZ01tcDFP?=
 =?utf-8?B?SjJlSzl3L0s5UGloN1o3QWVTbTBibmFzM2Z5L1V4T1NSbHNyeU9YRTN4WkQy?=
 =?utf-8?B?MVQ1QTJGdVI3SHFNVHg3TERnNjFBeVI4VEloWXRQWWgwdDlKSWpqYXJGL0wv?=
 =?utf-8?B?WGRqYjhKVjJyQ2ZnUC9rNGsxUjZXRlF1bCtSa1lQR05OZnV4S29vK0J6bDg0?=
 =?utf-8?B?azVNbkZWaU9RRW1mbTJmWDR2Z29nc2V5ZThuQnJ1RGRSQ3oyUGlobjZlSXdH?=
 =?utf-8?B?a0l0aVlZVExqT0FTWWZjbzJXNWRJdG13QVltQzZiWldQVVV4TkJKMUk2a1Nr?=
 =?utf-8?B?cytSSWVpb0l0NUprbURGTFlsb3J3WFNsQjFxN1NtcUxHZnk1MXl0Q01rMnVV?=
 =?utf-8?B?amFkVXhBMjF4VC9LOFRDRmdzVXZIVFBhNU5SWEduV0tPNDZmQTNqNzBWWHo2?=
 =?utf-8?B?MFdyQ3p5eURZNGhKTEFySXFmeHFRL3Rualp3SVM3bCs2eTRubzNQWDlRTFlj?=
 =?utf-8?B?dHAxOVFSczlBWmpaZ1ZCcTI1Y242WUNadENteWplMWFZclJ1eFBYdGhscWd0?=
 =?utf-8?B?U25BVzBtSzlHa3pjdk9YM3Z3d3BJUzdRelFEZys5VjQ4T04xeHNXU01DSWVZ?=
 =?utf-8?B?V3FCRjNQZUphQllnQjNuc21ZUE5Nam91SlVWcjByMUZlWWUwcXhtQWt2dE9U?=
 =?utf-8?Q?Wq8lErx8lYoVSVhwsEOxaVo=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?U3lreE1qdUNReG9wSUNEMzR3bnNaZmRMMEN4TzJ0V0lSZ2NPZy96VzJNZnRQ?=
 =?utf-8?B?RlpSWk5WOWExYUpyb3RGMFNnNXI1NHIxRjNXVU5KTnFoM3VpMU13M29PdTdQ?=
 =?utf-8?B?cnRNVTVoZmNNVkRmSGJzUDZvdUNEWUZ1V1l4Vk1uZXB2YkE1cUVXdUtZRHEz?=
 =?utf-8?B?bmlCZTluNXhIR2tnblpXbmQvZ045bGQzS1VMaEc5Wkt1WFppNGlvK0U5Wlpx?=
 =?utf-8?B?OFNPQUdDUzJhbkFKRFVqQmR1ZUhoNUlYM2c5R2dHZ3g1TWMrTHRUM3Ezd054?=
 =?utf-8?B?cm1HczBxMUxtUnZEZjhQSXdYT201OGNzU3cxSVQ2cmU1bW1oWS9DMDhZQ0hi?=
 =?utf-8?B?WHVLcXJIS2RLVDd5YnVsai9WOUV0cXFHRzJlV1l0SUVQZlpDZUJWV1cvTjNV?=
 =?utf-8?B?bU5YOVh0MjJiQzk2ZVVLMjROT0xzSWdIa0VacVo2VEVxT0lOMWdaN1crNEVn?=
 =?utf-8?B?Um9JVGFEdldWSXlWWllFc0VET0hWLzhhNi9aRzBGRGZidHBUNjVpNjVEQXRj?=
 =?utf-8?B?Q2dDMUVDS0tNM2RBY2ZzS0pvWHNoMFljZTNXS09KemoxbzYveCtSRWpJb01X?=
 =?utf-8?B?dTF2VWpnSHBZYUs3cURTZVlCaG9xYTdiSzYvZ1dHNUNZckNRcWtzR3JoNzkr?=
 =?utf-8?B?NzVnYnRyVlp6QmhjR2o4ODhPam9iV1FpSTU3aitSQTJleGZNVnA3SmNVdXNo?=
 =?utf-8?B?cXdSRytvOWZrejJ5cmxZUUxtWVBFVHdETkg3U0JzM1hRSmo3YndzbHNTcTF5?=
 =?utf-8?B?MmlmOHhXNEtvb1ZRMUo4QTd6Wkp5N1lmWlV0amNyZHcza3RoUlpwckVVZTYv?=
 =?utf-8?B?U0hLNzRTUjc5cEZ0Z2pvUkxBeGVuaWFXbzVwci9qNlNYTjVWYldISUpwRkRO?=
 =?utf-8?B?bnQvVjR1YlRKN1JJYk43MVU2Ri9VNXVYSG50cDdnWUowRHdBVWZ1NXFGOGRz?=
 =?utf-8?B?TzlyZnRjVFZhZFRqN1c2TkVySWVGMUJVYnZRYzYyay81a2YzMnd0L1ROWVAr?=
 =?utf-8?B?Ynk0TlFUNm5lakF1RnNWL0kyTndhQW1UOThUS2traWZWK3hpZ2xCZWltR2R4?=
 =?utf-8?B?UlU3cnFlMmhMQmJhSEN6bWIyU1Bkb2w0OHJ4Uy8rYko4VU5PVFFNZ0xwV2Fn?=
 =?utf-8?B?cFNkZmMvaTVacW40R2l0RmoxQ0xJTzVhYzZxdUpWalFvZ3krZHFFc09pNkpS?=
 =?utf-8?B?M0ZqbGJ5ZFpGU0h6SXBqTWVZUHJnalNMeGpGdStiY1JjeHBPbzZQc1N4a05t?=
 =?utf-8?B?dHBFM1lTRlBqTkRGNE1WeTV6bGpLZ1VlL0VUZ1lqRXlLaExsRmlQeTQxa0lG?=
 =?utf-8?B?UTduNTZacmFXdnlzKzd0RUcwejJ2M0V6VzFYaEFxMEVkNWFielF6RitaUVdo?=
 =?utf-8?B?YlFWVDRXYlh1akllR2crWWZleVNiaU1raHN5VHQ1dE9Wb0UrdUpEWitPbjJZ?=
 =?utf-8?B?cVIxenJ6OXdpakxlRXA4ZWxDNU9JZDZoVERGNzh0OTVleE42WjY2ME5lWmt3?=
 =?utf-8?B?Smt2QUsvSTRUT3YyZzVwbDlMNUE0NTFVbTlrMlg5MXRPTFZWZW54RG5SSVA4?=
 =?utf-8?Q?d70nx7BFTTUmrNp8oxMRSFMx4=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d086b3f4-0639-4e06-67e2-08db4014a831
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4542.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2023 13:56:07.3804
 (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: 0mwblCyh1nJ2ecwWgr4ELsR8ttra1iTOOT9X6YQwPfNZXyFlbZ8xhJ6EyX1JYzzJ7QjayOo09Md2trEY8Vf58uW1LTg/tmQwIqoy4IGYtIk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB5947
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-18_10,2023-04-18_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0
 adultscore=0 malwarescore=0 bulkscore=0 suspectscore=0 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303200000 definitions=main-2304180121
X-Proofpoint-ORIG-GUID: BGMqpPJmvT-Ea8R-Im-1hMhyUjQUNTTQ
X-Proofpoint-GUID: BGMqpPJmvT-Ea8R-Im-1hMhyUjQUNTTQ
X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        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


On 4/6/23 18:58, Baoquan He wrote:
> On 04/06/23 at 11:10am, Eric DeVolder wrote:
>>
>>
>> On 4/6/23 06:04, Baoquan He wrote:
>>> On 04/04/23 at 02:03pm, Eric DeVolder wrote:
>>> ......
>>>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>>>> +{
>>>> +	struct kimage *image;
>>>> +
>>>> +	/* Obtain lock while changing crash information */
>>>> +	if (!kexec_trylock()) {
>>>> +		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	/* Check kdump is not loaded */
>>>> +	if (!kexec_crash_image)
>>>> +		goto out;
>>>> +
>>>> +	image = kexec_crash_image;
>>>> +
>>>> +	if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>>>> +		hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>>>> +		pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>>>> +	else
>>>> +		pr_debug("hp_action %u\n", hp_action);
>>>
>>> Seems we passed in the cpu number just for printing here. Wondering why
>>> we don't print out hot added/removed memory ranges. Is the cpu number
>>> printing necessary?
>>>
>> Baoquan,
>>
>> Ah, actually until recently it was used to track the 'offlinecpu' in this
>> function, but tglx pointed out that was un-necessary. That resulted in
>> dropping the code in this function dealing with offlinecpu, leaving this as
>> its only use in this function.
>>
>> The printing of cpu number is not necessary, but helpful; I use it for debugging.
> 
> OK, I see. I am not requesting memory range printing, just try to prove
> cpu number printing is not so justified. If it's helpful, I am OK with
> it. Let's see if other people have concern about this.
> 

I do not plan on adding the memory range printing.

>>
>> The printing of memory range is also not necessary, but in order to do that,
>> should we choose to do so, requires passing in the memory range to this
>> function. This patch series did do this early on, and by v7 I dropped it at
>> your urging (https://lore.kernel.org/lkml/20220401183040.1624-1-eric.devolder@oracle.com/).
>> At the time, I provided it since I considered this generic infrastructure,
>> but I could not defend it since x86 didn't need it. However, PPC now needs
>> this, and is now carrying this as part of PPC support of CRASH_HOTPLUG (https://lore.kernel.org/linuxppc-dev/20230312181154.278900-6-sourabhjain@linux.ibm.com/T/#u).
>>
>> If you'd rather I pickup the memory range handling again, I can do that. I
>> think I'd likely change this function to be:
>>
>>    void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu,
>>       struct memory_notify *mhp);
>>
>> where on a CPU op the 'cpu' parameter would be valid and 'mhp' NULL, and on a memory op,
>> the 'mhp' would be valid and 'cpu' parameter invalid(0).
>>
>> I'd likely then stuff these two parameters into struct kimage so that it can
>> be utilized by arch-specific handler, if needed.
>>
>> And of course, would print out the memory range for debug purposes.
>>
>> Let me know what you think.
> 

I do not plan on adding the memory range handling; I'll let Sourabh do that as he has a use case for it.

As such, I don't see any other request for changes.

Thanks!
eric