From: Baoquan He <bhe@redhat.com>
To: "chenjiahao (C)" <chenjiahao16@huawei.com>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com,
conor.dooley@microchip.com, guoren@kernel.org, heiko@sntech.de,
bjorn@rivosinc.com, alex@ghiti.fr, akpm@linux-foundation.org,
atishp@rivosinc.com, thunder.leizhen@huawei.com,
horms@kernel.org, linux-kernel@vger.kernel.org,
linux-riscv@lists.infradead.org, kexec@lists.infradead.org,
linux-doc@vger.kernel.org
Subject: Re: [PATCH -next v2 1/2] riscv: kdump: Implement crashkernel=X,[high,low]
Date: Fri, 31 Mar 2023 07:32:54 +0800 [thread overview]
Message-ID: <ZCYcJos4MLBvpP9/@MiWiFi-R3L-srv> (raw)
In-Reply-To: <b8a32e3c-c083-20de-16d1-f628b02b739b@huawei.com>
On 03/30/23 at 09:40pm, chenjiahao (C) wrote:
......
> Agreed, I will clean this up later in next version.
> > > + if (ret || !crash_size)
> > > + return;
> > > +
> > > + /*
> > > + * crashkernel=Y,low is valid only when crashkernel=X,high
> > > + * is passed and high memory is reserved successful.
> > > + */
> > > + ret = parse_crashkernel_low(boot_command_line, 0, &crash_low_size, &crash_base);
> > > + if (ret == -ENOENT)
> > > + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> > > + else if (ret)
> > > + return;
> > > +
> > > + search_start = dma32_phys_limit;
> > > + } else if (ret || !crash_size) {
> > > + /* Invalid argument value specified */
> > > return;
> > > + }
> > > crash_size = PAGE_ALIGN(crash_size);
> > > @@ -1201,16 +1246,26 @@ static void __init reserve_crashkernel(void)
> > > */
> > > crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > > search_start,
> > > - min(search_end, (unsigned long) SZ_4G));
> > > + min(search_end, (unsigned long)dma32_phys_limit));
> > > if (crash_base == 0) {
> > The above conditional check isn't right. If crashkernel=size@offset
> > specified, the reservation failure won't trigger retry. This seems to be
> > originally introduced by old commit, while this need be fixed firstly.
>
> Just a little curious about the rule to cope with this specific case. If
> "crashkernel=size@offset" was passed
>
> but reserve failed, should try again to allocate in high memory, regardless
> the specified size@offset,
>
> or just throw a warning and return? Since I noticed the current logic here
> on Arm64 is to check if !fixed_base first
Yeah, we need mark the "crashkernel=size@offset" case and avoid to
retry. Because you won't succeed if memblock has already failed to
reserve an unavailable memory region, retry is meaningless. This has
been done in x86, arm64.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
WARNING: multiple messages have this Message-ID (diff)
From: Baoquan He <bhe@redhat.com>
To: "chenjiahao (C)" <chenjiahao16@huawei.com>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com,
conor.dooley@microchip.com, guoren@kernel.org, heiko@sntech.de,
bjorn@rivosinc.com, alex@ghiti.fr, akpm@linux-foundation.org,
atishp@rivosinc.com, thunder.leizhen@huawei.com,
horms@kernel.org, linux-kernel@vger.kernel.org,
linux-riscv@lists.infradead.org, kexec@lists.infradead.org,
linux-doc@vger.kernel.org
Subject: Re: [PATCH -next v2 1/2] riscv: kdump: Implement crashkernel=X,[high,low]
Date: Fri, 31 Mar 2023 07:32:54 +0800 [thread overview]
Message-ID: <ZCYcJos4MLBvpP9/@MiWiFi-R3L-srv> (raw)
In-Reply-To: <b8a32e3c-c083-20de-16d1-f628b02b739b@huawei.com>
On 03/30/23 at 09:40pm, chenjiahao (C) wrote:
......
> Agreed, I will clean this up later in next version.
> > > + if (ret || !crash_size)
> > > + return;
> > > +
> > > + /*
> > > + * crashkernel=Y,low is valid only when crashkernel=X,high
> > > + * is passed and high memory is reserved successful.
> > > + */
> > > + ret = parse_crashkernel_low(boot_command_line, 0, &crash_low_size, &crash_base);
> > > + if (ret == -ENOENT)
> > > + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> > > + else if (ret)
> > > + return;
> > > +
> > > + search_start = dma32_phys_limit;
> > > + } else if (ret || !crash_size) {
> > > + /* Invalid argument value specified */
> > > return;
> > > + }
> > > crash_size = PAGE_ALIGN(crash_size);
> > > @@ -1201,16 +1246,26 @@ static void __init reserve_crashkernel(void)
> > > */
> > > crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > > search_start,
> > > - min(search_end, (unsigned long) SZ_4G));
> > > + min(search_end, (unsigned long)dma32_phys_limit));
> > > if (crash_base == 0) {
> > The above conditional check isn't right. If crashkernel=size@offset
> > specified, the reservation failure won't trigger retry. This seems to be
> > originally introduced by old commit, while this need be fixed firstly.
>
> Just a little curious about the rule to cope with this specific case. If
> "crashkernel=size@offset" was passed
>
> but reserve failed, should try again to allocate in high memory, regardless
> the specified size@offset,
>
> or just throw a warning and return? Since I noticed the current logic here
> on Arm64 is to check if !fixed_base first
Yeah, we need mark the "crashkernel=size@offset" case and avoid to
retry. Because you won't succeed if memblock has already failed to
reserve an unavailable memory region, retry is meaningless. This has
been done in x86, arm64.
WARNING: multiple messages have this Message-ID (diff)
From: Baoquan He <bhe@redhat.com>
To: "chenjiahao (C)" <chenjiahao16@huawei.com>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com,
conor.dooley@microchip.com, guoren@kernel.org, heiko@sntech.de,
bjorn@rivosinc.com, alex@ghiti.fr, akpm@linux-foundation.org,
atishp@rivosinc.com, thunder.leizhen@huawei.com,
horms@kernel.org, linux-kernel@vger.kernel.org,
linux-riscv@lists.infradead.org, kexec@lists.infradead.org,
linux-doc@vger.kernel.org
Subject: Re: [PATCH -next v2 1/2] riscv: kdump: Implement crashkernel=X,[high,low]
Date: Fri, 31 Mar 2023 07:32:54 +0800 [thread overview]
Message-ID: <ZCYcJos4MLBvpP9/@MiWiFi-R3L-srv> (raw)
In-Reply-To: <b8a32e3c-c083-20de-16d1-f628b02b739b@huawei.com>
On 03/30/23 at 09:40pm, chenjiahao (C) wrote:
......
> Agreed, I will clean this up later in next version.
> > > + if (ret || !crash_size)
> > > + return;
> > > +
> > > + /*
> > > + * crashkernel=Y,low is valid only when crashkernel=X,high
> > > + * is passed and high memory is reserved successful.
> > > + */
> > > + ret = parse_crashkernel_low(boot_command_line, 0, &crash_low_size, &crash_base);
> > > + if (ret == -ENOENT)
> > > + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
> > > + else if (ret)
> > > + return;
> > > +
> > > + search_start = dma32_phys_limit;
> > > + } else if (ret || !crash_size) {
> > > + /* Invalid argument value specified */
> > > return;
> > > + }
> > > crash_size = PAGE_ALIGN(crash_size);
> > > @@ -1201,16 +1246,26 @@ static void __init reserve_crashkernel(void)
> > > */
> > > crash_base = memblock_phys_alloc_range(crash_size, PMD_SIZE,
> > > search_start,
> > > - min(search_end, (unsigned long) SZ_4G));
> > > + min(search_end, (unsigned long)dma32_phys_limit));
> > > if (crash_base == 0) {
> > The above conditional check isn't right. If crashkernel=size@offset
> > specified, the reservation failure won't trigger retry. This seems to be
> > originally introduced by old commit, while this need be fixed firstly.
>
> Just a little curious about the rule to cope with this specific case. If
> "crashkernel=size@offset" was passed
>
> but reserve failed, should try again to allocate in high memory, regardless
> the specified size@offset,
>
> or just throw a warning and return? Since I noticed the current logic here
> on Arm64 is to check if !fixed_base first
Yeah, we need mark the "crashkernel=size@offset" case and avoid to
retry. Because you won't succeed if memblock has already failed to
reserve an unavailable memory region, retry is meaningless. This has
been done in x86, arm64.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-03-30 23:33 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-28 11:51 [PATCH -next v2 0/2] support allocating crashkernel above 4G explicitly on riscv Chen Jiahao
2023-03-28 11:51 ` Chen Jiahao
2023-03-28 11:51 ` Chen Jiahao
2023-03-28 11:51 ` [PATCH -next v2 1/2] riscv: kdump: Implement crashkernel=X,[high,low] Chen Jiahao
2023-03-28 11:51 ` Chen Jiahao
2023-03-28 11:51 ` Chen Jiahao
2023-03-29 11:19 ` Baoquan He
2023-03-29 11:19 ` Baoquan He
2023-03-29 11:19 ` Baoquan He
2023-03-30 13:40 ` chenjiahao (C)
2023-03-30 13:40 ` chenjiahao (C)
2023-03-30 13:40 ` chenjiahao (C)
2023-03-30 23:32 ` Baoquan He [this message]
2023-03-30 23:32 ` Baoquan He
2023-03-30 23:32 ` Baoquan He
2023-03-31 11:36 ` chenjiahao (C)
2023-03-31 11:36 ` chenjiahao (C)
2023-03-31 11:36 ` chenjiahao (C)
2023-03-28 11:51 ` [PATCH -next v2 2/2] docs: kdump: Update the crashkernel description for riscv Chen Jiahao
2023-03-28 11:51 ` Chen Jiahao
2023-03-28 11:51 ` Chen Jiahao
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=ZCYcJos4MLBvpP9/@MiWiFi-R3L-srv \
--to=bhe@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=alex@ghiti.fr \
--cc=atishp@rivosinc.com \
--cc=bjorn@rivosinc.com \
--cc=chenjiahao16@huawei.com \
--cc=conor.dooley@microchip.com \
--cc=guoren@kernel.org \
--cc=heiko@sntech.de \
--cc=horms@kernel.org \
--cc=kexec@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=thunder.leizhen@huawei.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.