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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Eric DeVolder <eric.devolder@oracle.com>,
        linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, bhe@redhat.com, vgoyal@redhat.com
Cc:     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, eric.devolder@oracle.com
Subject: Re: [PATCH v17 3/6] crash: add generic infrastructure for crash
 hotplug support
In-Reply-To: <20230118213544.2128-4-eric.devolder@oracle.com>
References: <20230118213544.2128-1-eric.devolder@oracle.com>
 <20230118213544.2128-4-eric.devolder@oracle.com>
Date:   Thu, 19 Jan 2023 22:31:26 +0100
Message-ID: <878rhyi53l.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Eric!

On Wed, Jan 18 2023 at 16:35, Eric DeVolder wrote:
> CPU and memory change notifications are received in order to
> regenerate the elfcorehdr.
>
> To support cpu hotplug, a callback is registered to capture the
> CPUHP_AP_ONLINE_DYN online and offline events via
> cpuhp_setup_state_nocalls().

This sentence does not make sense. The callback is not registered to
capture CPUHP_AP_ONLINE_DYN events.

What this does is: It installs a dynamic CPU hotplug state with
callbacks for online and offline. These callbacks store information
about a CPU coming up and going down. Right?

But why are they required and what's the value?

This changelog tells WHAT it does and not WHY. I can see the WHAT from
the patch itself. 

Don't tell me the WHY is in the cover letter. The cover letter is not
part of the commits and changelogs have to be self contained.

Now let me cite from your cover letter:

> When the kdump service is loaded, if a CPU or memory is hot
> un/plugged, the crash elfcorehdr, which describes the CPUs
> and memory in the system, must also be updated, else the resulting
> vmcore is inaccurate (eg. missing either CPU context or memory
> regions).

The CPU hotplug state you are using for this is patently inaccurate
too. With your approach the CPU is tracked as online very late in the
hotplug process and tracked as offline very early on unplug.

So if the kernel crashes before/after the plug/unplug tracking event
then your recorded state is bogus and given the amount of callbacks
between the real online/offline and the recording point there is a
pretty large window.

You can argue that this is better than the current state and considered
good enough for whatever reason, but such information wants to be in the
changelog, no?

Thanks,

        tglx

Hint: The requirements for changelogs are well documented in Documentation/process/