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: <d3ae80cd-7e1f-50e3-d90d-becf779788c0@oracle.com>
Date:   Wed, 13 Apr 2022 07:37:16 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v6 4/8] 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,
        konrad.wilk@oracle.com, boris.ostrovsky@oracle.com
References: <20220401183040.1624-1-eric.devolder@oracle.com>
 <20220401183040.1624-5-eric.devolder@oracle.com>
 <YlPy8CKU4zHsx6Bc@MiWiFi-R3L-srv>
 <821b3c62-eedf-c1a3-4aba-671c90d83288@oracle.com>
 <YlY4SlF07iH08Qnj@MiWiFi-R3L-srv>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <YlY4SlF07iH08Qnj@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?R242ODdTRVFlL2lJdm9LaHR0bUkwZzl4L1lEYityQTBWZVAraWd6M0YvKytI?=
 =?utf-8?B?cEFLMS9tME5URllCTmZMWDZqWEZrRUk3dllFbGYvQ0F4SkRYSnE3dm9DMWVq?=
 =?utf-8?B?MCtER1hZdXVwVEVhdHBydWRBdG5Pc1BQMXRpRnZUQVhMYm5PTzF5aVRuOUZi?=
 =?utf-8?B?VW9PZE5ZNlFMM20xYzNITjRxSXYyS0FFRFhLMjlKYlVZTzJkRS9qQllqd2xj?=
 =?utf-8?B?RlpMZ3lBTDlCTG9RalNjTnR6bHBWb1pQWi9vSHRONXhzRkVwNWxaNGdGTjFH?=
 =?utf-8?B?YVZkWFFpWHBwS3hSb2tXTmlSNFk5dzRhQlZFbFdneEw5UlNwckFpOFFnZTVj?=
 =?utf-8?B?cXp0dVRFdFowYVh4WHl3VTZMZWJ5OWRwQS9wWjEzYTZudllPUzdVR3lFOFRs?=
 =?utf-8?B?MWZ3R3hJV1lEUDk1dHFNUllUeWZGYU8rZUZkVlhqeWsrRzJqNjcxVVoyV2pZ?=
 =?utf-8?B?M1B4cEZmR0Z3Sk4rV2NpU3NGQ0NVMG5rMGVlS0pkTmt1TFN3TW5rNmVLMzVz?=
 =?utf-8?B?T01vTnJ6MXFkMVRuemhrdENCdlZoYXA1TDFYajNCUEozaEF6anFrOXdyaGI4?=
 =?utf-8?B?RGwrd043RTFQY0FJSHlBNkEzTVc1TWZTdnlPaW5wcElkYzJmWDVJeDl2MTVi?=
 =?utf-8?B?M0RoWnUyL0s2VXJmVFVzdS92SVVQMFNvcHdnL2ZmdVM1Zit0aTRSUVROMWds?=
 =?utf-8?B?bVdtMldnU3lUQkQ1NnkzSmszVmVNaG9vWXF1ZWgvbmRvbUswZHNSVldpZFRY?=
 =?utf-8?B?L01zbFNtTVpoRndIVVRlUVh6a0doODdCTjl0VHQxenVEUUZ1dDBUUEdQL1Ar?=
 =?utf-8?B?TTN0SXZLOEU0dVZlblVwUTN3KzVWaWpyQlB2NnFFUmptZXBSWDFzT0pTaVk1?=
 =?utf-8?B?T1JISWE0NE1MWVRXMVVJQkxrclJPcjQrNGpObnh4MVFSL1krd09NbnV4OTJw?=
 =?utf-8?B?MGswSVNzaUxVSm0yNHRzdVZDSk81RG15ZVNKZllXdEVtZ0dEVVJTcHdSVSsz?=
 =?utf-8?B?bnZNelN2ZUQ0ZDl2b1BCM005bzdPdDlCK042eGhhRjdOVVFMRzdUWHNxd2xO?=
 =?utf-8?B?ZjJRb3pLWEM2REh6cm5YLzFoVDhTOWxWdHdwem0vNUQ5dGlOOXRKR1E2R2lp?=
 =?utf-8?B?ZmpiZ2FDa2graFljVUp4SVZZK3dzbzRZb1BkNk4wOEM5WUExNHh0ZE1MWUQ0?=
 =?utf-8?B?VEp5Z3FjaHJWdzY4aElaNmVhVDZ4WXoyM2Irb3ppeE1GU2lBcFVJcHB0bzBi?=
 =?utf-8?B?bDVibm4rSXh4TzlHYWpwT092THR4cTZPNjFiVlNnN0M0RXo5S1BFWEdEdncw?=
 =?utf-8?B?ZTBMT2M0bENzWnNYN25pTXkwSmlCMVFLMVkwVE1hWmppTWhjelZMS0RSZDFh?=
 =?utf-8?B?a0lkVG1GYmRjd2FidDNlY0F2MFJzdVBQaUUvOVhoRTVVeGcva3JwZ1FKdnlz?=
 =?utf-8?B?cFpGdCtEd2d3NlgvTU1QYytCWGRpSWVPbDBlaTRZQmVjR21uK2wzSitaVGxP?=
 =?utf-8?B?TFFyWGp0TkV6cVJtelRGSVBEb2NndzJqdDgyM2RzOFFETk9FaldnOXQzSzJX?=
 =?utf-8?B?Y3NLVklFSnZtaVFhWWs1VGJFYVlKSDMxditnTm5hd0JaV3NSTVMwOHI5Skkz?=
 =?utf-8?B?V21mZE00anROOGQwbFdqYm9nRFNRSlFFWHlOY05tbEhVWklqRmtCd2ZwVGF5?=
 =?utf-8?B?c0Q4bFdXVEJxVXkxZUdjR0trOHJ1aTh6dUdoZnhOdFdiZ3p2a2hzdWs0aEV0?=
 =?utf-8?B?dm5IVGN4SmI2QnVLVGZhSCs4cExXWEhsUEM5dHVYdFpuYVViS1VyWnhDaDg5?=
 =?utf-8?B?K0RWMGFlSnZvdWRvSGJCNmhpRWJ2cGRybFNnK2VrTG8rZGVJS2xGYlRaVEty?=
 =?utf-8?B?ckNkZHNqYldrWDFoVXRmZGhuQmdkUGpReTRYMnRoU3VWQmpHd1pCbExwTGk1?=
 =?utf-8?B?SG9YNUFkM1BBREw2SmU1VGtVNlczVjBvb2EzQlJkU21LN3llU2tmeTVQdE5v?=
 =?utf-8?B?Vm1iYVFSeGRBb1dCaWlTUXFjN2h1akE3OFZkcHE3WDNKMmg5V1MyNlYrT3R2?=
 =?utf-8?B?Q3pYM0dXV2owOC81aGpPTkdKeVdWdWJCT0p5YVhxRVVTNlgwWVFXc25RVHFx?=
 =?utf-8?B?RnRidXNybjU5eG53OTg3M3dIS1dEUFZ4ektXeG8zUFg2NVZQb0tyZXZTd1hv?=
 =?utf-8?B?SWRNcVdFTTdkRW5FcDBCUHV3TmFhMThWSDgvaXE0WUZ2b3VhazNWNWQvY1Fw?=
 =?utf-8?B?M3hxQTlhSzVlWGlkTUxQTUdSVXU1UTh4dG4zeXpEbXlJeld5elF3b0EwRklP?=
 =?utf-8?B?NzZSaTBUUzFxV2V4RlJHanBtckhWVVlUK2drWXBVQ2EyWFV1ZG96elpqeE5n?=
 =?utf-8?Q?05gGwGWfIhwuana//lF1s0SrvAPr0+alnrKqK?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 765a9992-99d3-4710-df10-08da1d4a5be6
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Apr 2022 12:37:23.5489
 (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: 5Nbfn7qQubnIu11uUUOC06hhD8yuPFQ4R4++WkitAqIpfLKd4U391VT1gpC6pNqCIXs8MR04YcWBq/LbeIZ1rg6/rhMjS1QCmfnya0rLD0M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR10MB5429
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-13_01:2022-04-13,2022-04-13 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=962
 spamscore=0 suspectscore=0 malwarescore=0 adultscore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204130067
X-Proofpoint-ORIG-GUID: O19bCs8GF4XePGmyw1AKW2-rZVtrxlrr
X-Proofpoint-GUID: O19bCs8GF4XePGmyw1AKW2-rZVtrxlrr
X-Spam-Status: No, score=-4.1 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_H5,RCVD_IN_MSPIKE_WL,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/12/22 21:41, Baoquan He wrote:
> On 04/11/22 at 08:54am, Eric DeVolder wrote:
>>
>>
>> On 4/11/22 04:20, Baoquan He wrote:
>>> Hi Eric,
>>>
>>> On 04/01/22 at 02:30pm, Eric DeVolder wrote:
>>> ... ...
>>>
>>>> +static void crash_hotplug_handler(unsigned int hp_action,
>>>> +	unsigned long a, unsigned long b)
>>>
>>> I am still struggling to consider if these unused parameters should be
>>> kept or removed. Do you foresee or feel on which ARCH they could be used?
>>>
>>> Considering our elfcorehdr updating method, once memory or cpu changed,
>>> we will update elfcorehdr and cpu notes to reflect all existing memory
>>> regions and cpu in the current system. We could end up with having them
>>> but never being used. Then we may finally need to clean them up.
>>>
>>> If you have investigated and foresee or feel they could be used on a
>>> certain architecture, we can keep them for the time being.
>>
>> So 'hp_action' and 'a' are used within the existing patch series.
>> In crash_core.c, there is this bit of code:
>>
>> +       kexec_crash_image->offlinecpu =
>> +           (hp_action == KEXEC_CRASH_HP_REMOVE_CPU) ?
>> +               (unsigned int)a : ~0U;
>>
>> which is referencing both 'hp_action' and using 'a' from the cpu notifier handler.
>> I looked into removing 'a' and setting offlinecpu directly, but I thought
>> it better that offlinecpu be set within the safety of the kexec_mutex.
>> Also, Sourabh Jain's work with PowerPC utilizing this framework directly
>> references hp_action in the arch-specific handler.
>>
>> The cpu and memory notifier handlers set hp_action accordingly. For cpu handler,
>> the 'a' is set with the impacted cpu. For memory handler, 'a' and 'b' form the
>> impacted memory range. I agree it looks like the memory range is currently
>> not useful.
> 
> OK, memory handler doesn't need the action, memory regions. While cpu
> handler needs it to exclude the hot plugged cpu.
> 
> We could have two ways to acheive this as below. How do you think about
> them?
> 
> static void crash_hotplug_handler(unsigned int hp_action,
>          unsigned long cpu)
> 
> static int crash_memhp_notifier(struct notifier_block *nb,
>          unsigned long val, void *v)
> {
> ......
>          switch (val) {
>          case MEM_ONLINE:
>                  crash_hotplug_handler(KEXEC_CRASH_HP_ADD_MEMORY,
>                          -1UL);
>                  break;
> 
>          case MEM_OFFLINE:
>                  crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_MEMORY,
>                          -1UL);
>                  break;
>          }
>          return NOTIFY_OK;
> }
> 
> static int crash_cpuhp_online(unsigned int cpu)
> {
>          crash_hotplug_handler(KEXEC_CRASH_HP_ADD_CPU, cpu);
>          return 0;
> }
> 
> static int crash_cpuhp_offline(unsigned int cpu)
> {
>          crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>          return 0;
> }

I'm OK with the above. Shall I post v7 or are you still looking at patches 7 and 8?
Thanks!
Eric

> 
> OR,
> 
> static void crash_hotplug_handler(unsigned int hp_action,
>          int* cpu)
> 
> static int crash_cpuhp_online(unsigned int cpu)
> {
>          crash_hotplug_handler(KEXEC_CRASH_HP_ADD_CPU, NULL);
>          return 0;
> }
> 
> static int crash_cpuhp_offline(unsigned int cpu)
> {
> 	int dead_cpu = cpu;
>          crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_CPU, &cpu);
>          return 0;
> }
>