From: Borislav Petkov <bp@alien8.de>
To: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
x86@kernel.org, "H . Peter Anvin" <hpa@zytor.com>,
linux-kernel@vger.kernel.org, Dave Young <dyoung@redhat.com>,
Baoquan He <bhe@redhat.com>, Vivek Goyal <vgoyal@redhat.com>,
Eric Biederman <ebiederm@xmission.com>,
kexec@lists.infradead.org,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org,
Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
devicetree@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
linux-doc@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
Feng Zhou <zhoufeng.zf@bytedance.com>,
Kefeng Wang <wangkefeng.wang@huawei.com>,
Chen Zhou <dingguo.cz@antgroup.com>,
John Donnelly <John.p.donnelly@oracle.com>
Subject: Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code
Date: Tue, 28 Dec 2021 17:13:06 +0100 [thread overview]
Message-ID: <Ycs3kpZD/vpoo1AX@zn.tnic> (raw)
In-Reply-To: <20211228132612.1860-3-thunder.leizhen@huawei.com>
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
> Use parse_crashkernel_high_low() to bring the parsing of
> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> are strongly dependent, make code logic clear and more readable.
>
> Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
> /* crashkernel=XM */
> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> if (ret != 0 || crash_size <= 0) {
> - /* crashkernel=X,high */
> - ret = parse_crashkernel_high(boot_command_line, total_mem,
> - &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + /* crashkernel=X,high and possible crashkernel=Y,low */
> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
So it is getting there but it needs more surgery...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
WARNING: multiple messages have this Message-ID (diff)
From: Borislav Petkov <bp@alien8.de>
To: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
x86@kernel.org, "H . Peter Anvin" <hpa@zytor.com>,
linux-kernel@vger.kernel.org, Dave Young <dyoung@redhat.com>,
Baoquan He <bhe@redhat.com>, Vivek Goyal <vgoyal@redhat.com>,
Eric Biederman <ebiederm@xmission.com>,
kexec@lists.infradead.org,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org,
Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
devicetree@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
linux-doc@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
Feng Zhou <zhoufeng.zf@bytedance.com>,
Kefeng Wang <wangkefeng.wang@huawei.com>,
Chen Zhou <dingguo.cz@antgroup.com>,
John Donnelly <John.p.donnelly@oracle.com>
Subject: Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code
Date: Tue, 28 Dec 2021 17:13:06 +0100 [thread overview]
Message-ID: <Ycs3kpZD/vpoo1AX@zn.tnic> (raw)
In-Reply-To: <20211228132612.1860-3-thunder.leizhen@huawei.com>
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
> Use parse_crashkernel_high_low() to bring the parsing of
> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> are strongly dependent, make code logic clear and more readable.
>
> Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
> /* crashkernel=XM */
> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> if (ret != 0 || crash_size <= 0) {
> - /* crashkernel=X,high */
> - ret = parse_crashkernel_high(boot_command_line, total_mem,
> - &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + /* crashkernel=X,high and possible crashkernel=Y,low */
> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
So it is getting there but it needs more surgery...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Borislav Petkov <bp@alien8.de>
To: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
x86@kernel.org, "H . Peter Anvin" <hpa@zytor.com>,
linux-kernel@vger.kernel.org, Dave Young <dyoung@redhat.com>,
Baoquan He <bhe@redhat.com>, Vivek Goyal <vgoyal@redhat.com>,
Eric Biederman <ebiederm@xmission.com>,
kexec@lists.infradead.org,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org,
Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
devicetree@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
linux-doc@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
Feng Zhou <zhoufeng.zf@bytedance.com>,
Kefeng Wang <wangkefeng.wang@huawei.com>,
Chen Zhou <dingguo.cz@antgroup.com>,
John Donnelly <John.p.donnelly@oracle.com>
Subject: Re: [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code
Date: Tue, 28 Dec 2021 17:13:06 +0100 [thread overview]
Message-ID: <Ycs3kpZD/vpoo1AX@zn.tnic> (raw)
In-Reply-To: <20211228132612.1860-3-thunder.leizhen@huawei.com>
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote:
> Use parse_crashkernel_high_low() to bring the parsing of
> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they
> are strongly dependent, make code logic clear and more readable.
>
> Suggested-by: Borislav Petkov <bp@alien8.de>
Yeah, doesn't look like something I suggested...
> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void)
> /* crashkernel=XM */
> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base);
> if (ret != 0 || crash_size <= 0) {
> - /* crashkernel=X,high */
> - ret = parse_crashkernel_high(boot_command_line, total_mem,
> - &crash_size, &crash_base);
> - if (ret != 0 || crash_size <= 0)
> + /* crashkernel=X,high and possible crashkernel=Y,low */
> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size);
So this calls parse_crashkernel() and when that one fails, it calls this
new weird parse high/low helper you added.
But then all three end up in the same __parse_crashkernel() worker
function which seems to do the actual parsing.
What I suggested and what would be real clean is if the arches would
simply call a *single*
parse_crashkernel()
function and when that one returns, *all* crashkernel= options would
have been parsed properly, low, high, middle crashkernel, whatever...
and the caller would know what crash kernel needs to be allocated.
Then each arch can do its memory allocations and checks based on that
parsed data and decide to allocate or bail.
So it is getting there but it needs more surgery...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2021-12-28 16:13 UTC|newest]
Thread overview: 126+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-28 13:25 [PATCH v19 00/13] support reserving crashkernel above 4G on arm64 kdump Zhen Lei
2021-12-28 13:25 ` Zhen Lei
2021-12-28 13:25 ` Zhen Lei
2021-12-28 13:26 ` [PATCH v19 01/13] kdump: add helper parse_crashkernel_high_low() Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-30 10:14 ` Leizhen (ThunderTown)
2021-12-30 10:14 ` Leizhen (ThunderTown)
2021-12-30 10:14 ` Leizhen (ThunderTown)
2021-12-30 10:40 ` Borislav Petkov
2021-12-30 10:40 ` Borislav Petkov
2021-12-30 10:40 ` Borislav Petkov
2021-12-30 11:08 ` Leizhen (ThunderTown)
2021-12-30 11:08 ` Leizhen (ThunderTown)
2021-12-30 11:08 ` Leizhen (ThunderTown)
2021-12-31 9:22 ` Leizhen (ThunderTown)
2021-12-31 9:22 ` Leizhen (ThunderTown)
2021-12-31 9:22 ` Leizhen (ThunderTown)
2021-12-31 12:29 ` Leizhen (ThunderTown)
2021-12-31 12:29 ` Leizhen (ThunderTown)
2021-12-31 12:29 ` Leizhen (ThunderTown)
2022-01-11 15:03 ` john.p.donnelly
2022-01-11 15:03 ` john.p.donnelly
2022-01-11 15:03 ` john.p.donnelly
2021-12-28 13:26 ` [PATCH v19 02/13] x86/setup: Use parse_crashkernel_high_low() to simplify code Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 16:13 ` Borislav Petkov [this message]
2021-12-28 16:13 ` Borislav Petkov
2021-12-28 16:13 ` Borislav Petkov
2021-12-29 2:27 ` Leizhen (ThunderTown)
2021-12-29 2:27 ` Leizhen (ThunderTown)
2021-12-29 2:27 ` Leizhen (ThunderTown)
2021-12-29 7:27 ` Dave Young
2021-12-29 7:27 ` Dave Young
2021-12-29 7:27 ` Dave Young
2021-12-29 7:45 ` Dave Young
2021-12-29 7:45 ` Dave Young
2021-12-29 7:45 ` Dave Young
2021-12-29 10:11 ` Borislav Petkov
2021-12-29 10:11 ` Borislav Petkov
2021-12-29 10:11 ` Borislav Petkov
2021-12-29 10:38 ` Dave Young
2021-12-29 10:38 ` Dave Young
2021-12-29 10:38 ` Dave Young
2021-12-29 11:11 ` Borislav Petkov
2021-12-29 11:11 ` Borislav Petkov
2021-12-29 11:11 ` Borislav Petkov
2021-12-29 14:13 ` Leizhen (ThunderTown)
2021-12-29 14:13 ` Leizhen (ThunderTown)
2021-12-29 14:13 ` Leizhen (ThunderTown)
2021-12-29 10:03 ` Borislav Petkov
2021-12-29 10:03 ` Borislav Petkov
2021-12-29 10:03 ` Borislav Petkov
2021-12-29 10:46 ` Dave Young
2021-12-29 10:46 ` Dave Young
2021-12-29 10:46 ` Dave Young
2021-12-29 15:04 ` Leizhen (ThunderTown)
2021-12-29 15:04 ` Leizhen (ThunderTown)
2021-12-29 15:04 ` Leizhen (ThunderTown)
2021-12-29 16:51 ` Borislav Petkov
2021-12-29 16:51 ` Borislav Petkov
2021-12-29 16:51 ` Borislav Petkov
2021-12-30 2:39 ` Leizhen (ThunderTown)
2021-12-30 2:39 ` Leizhen (ThunderTown)
2021-12-30 2:39 ` Leizhen (ThunderTown)
2021-12-30 8:56 ` Leizhen (ThunderTown)
2021-12-30 8:56 ` Leizhen (ThunderTown)
2021-12-30 8:56 ` Leizhen (ThunderTown)
2021-12-29 12:19 ` Leizhen (ThunderTown)
2021-12-29 12:19 ` Leizhen (ThunderTown)
2021-12-29 12:19 ` Leizhen (ThunderTown)
2022-01-11 15:04 ` john.p.donnelly
2022-01-11 15:04 ` john.p.donnelly
2022-01-11 15:04 ` john.p.donnelly
2021-12-28 13:26 ` [PATCH v19 03/13] kdump: make parse_crashkernel_{high|low}() static Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2022-01-11 15:04 ` john.p.donnelly
2022-01-11 15:04 ` john.p.donnelly
2022-01-11 15:04 ` john.p.donnelly
2021-12-28 13:26 ` [PATCH v19 04/13] kdump: reduce unnecessary parameters of parse_crashkernel_{high|low}() Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2022-01-11 15:05 ` john.p.donnelly
2022-01-11 15:05 ` john.p.donnelly
2022-01-11 15:05 ` john.p.donnelly
2021-12-28 13:26 ` [PATCH v19 05/13] x86/setup: Add and use CRASH_BASE_ALIGN Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2022-01-11 15:06 ` john.p.donnelly
2022-01-11 15:06 ` john.p.donnelly
2022-01-11 15:06 ` john.p.donnelly
2021-12-28 13:26 ` [PATCH v19 06/13] kexec: move crashk[_low]_res to crash_core module Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2022-01-11 15:06 ` john.p.donnelly
2022-01-11 15:06 ` john.p.donnelly
2022-01-11 15:06 ` john.p.donnelly
2021-12-28 13:26 ` [PATCH v19 07/13] kdump: Add helper reserve_crashkernel_mem[_low]() Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` [PATCH v19 08/13] x86/setup: Move CRASH[_BASE]_ALIGN and CRASH_ADDR_{LOW|HIGH}_MAX to asm/kexec.h Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` [PATCH v19 09/13] x86/setup: Use generic reserve_crashkernel_mem[_low]() Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` [PATCH v19 10/13] arm64: kdump: introduce some macros for crash kernel reservation Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` [PATCH v19 11/13] arm64: kdump: reimplement crashkernel=X Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2022-01-12 14:45 ` Dave Kleikamp
2022-01-12 14:45 ` Dave Kleikamp
2022-01-12 14:45 ` Dave Kleikamp
2022-01-13 1:17 ` Leizhen (ThunderTown)
2022-01-13 1:17 ` Leizhen
2022-01-13 1:17 ` Leizhen (ThunderTown)
2021-12-28 13:26 ` [PATCH v19 12/13] of: fdt: Add memory for devices by DT property "linux,usable-memory-range" Zhen Lei
2021-12-28 13:26 ` [PATCH v19 12/13] of: fdt: Add memory for devices by DT property "linux, usable-memory-range" Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` [PATCH v19 13/13] kdump: update Documentation about crashkernel Zhen Lei
2021-12-28 13:26 ` Zhen Lei
2021-12-28 13:26 ` Zhen Lei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ycs3kpZD/vpoo1AX@zn.tnic \
--to=bp@alien8.de \
--cc=John.p.donnelly@oracle.com \
--cc=bhe@redhat.com \
--cc=catalin.marinas@arm.com \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=dingguo.cz@antgroup.com \
--cc=dyoung@redhat.com \
--cc=ebiederm@xmission.com \
--cc=frowand.list@gmail.com \
--cc=hpa@zytor.com \
--cc=kexec@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
--cc=tglx@linutronix.de \
--cc=thunder.leizhen@huawei.com \
--cc=vgoyal@redhat.com \
--cc=wangkefeng.wang@huawei.com \
--cc=will@kernel.org \
--cc=x86@kernel.org \
--cc=zhoufeng.zf@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.