This patch removes the need of the crashkernel=...@offset parameter to define
a fixed offset for crashkernel reservation. That feature can be used together
with a relocatable kernel where the kexec-tools relocate the kernel and
get the actual offset from /proc/iomem.
The use case is a kernel where the .text+.data+.bss is after 16M physical
memory (debug kernel with lockdep on x86_64 can cause that) which caused a
major pain in autoconfiguration in our distribution.
Also, that patch unifies crashdump architectures a bit since IA64 has
that semantics from the very beginning of the kdump port.
Please provide feedback!
Signed-off-by: Bernhard Walle <[email protected]>
---
arch/x86/kernel/setup.c | 70 +++++++++++++++++++++++++++++++++++------------
1 files changed, 52 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index a81d82c..c30bb7b 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -435,6 +435,34 @@ static inline unsigned long long get_total_mem(void)
}
#ifdef CONFIG_KEXEC
+
+/**
+ * Reserve @size bytes of crashkernel memory at any suitable offset.
+ *
+ * @size: Size of the crashkernel memory to reserve.
+ * Returns the base address on success, and -1ULL on failure.
+ */
+unsigned long long find_and_reserve_crashkernel(unsigned long long size)
+{
+ const unsigned long long alignment = 16<<20; /* 16M */
+ unsigned long long start = 0LL;
+
+ while (1) {
+ int ret;
+
+ start = find_e820_area(start, ULONG_MAX, size, alignment);
+ if (start == -1ULL)
+ return start;
+
+ /* try to reserve it */
+ ret = reserve_bootmem_generic(start, size, BOOTMEM_EXCLUSIVE);
+ if (ret >= 0)
+ return start;
+
+ start += alignment;
+ }
+}
+
void __init reserve_crashkernel(void)
{
unsigned long long total_mem;
@@ -445,30 +473,36 @@ void __init reserve_crashkernel(void)
ret = parse_crashkernel(boot_command_line, total_mem,
&crash_size, &crash_base);
- if (ret == 0 && crash_size > 0) {
- if (crash_base <= 0) {
- printk(KERN_INFO "crashkernel reservation failed - "
- "you have to specify a base address\n");
+ if (ret != 0 || crash_size <= 0)
+ return;
+
+ /* 0 means: find the address automatically */
+ if (crash_base <= 0) {
+ crash_base = find_and_reserve_crashkernel(crash_size);
+ if (crash_base == -1ULL) {
+ pr_info("crashkernel reservation failed. "
+ "No suitable area found.\n");
return;
}
-
- if (reserve_bootmem_generic(crash_base, crash_size,
- BOOTMEM_EXCLUSIVE) < 0) {
- printk(KERN_INFO "crashkernel reservation failed - "
- "memory is in use\n");
+ } else {
+ ret = reserve_bootmem_generic(crash_base, crash_size,
+ BOOTMEM_EXCLUSIVE);
+ if (ret < 0) {
+ pr_info("crashkernel reservation failed - "
+ "memory is in use\n");
return;
}
+ }
- printk(KERN_INFO "Reserving %ldMB of memory at %ldMB "
- "for crashkernel (System RAM: %ldMB)\n",
- (unsigned long)(crash_size >> 20),
- (unsigned long)(crash_base >> 20),
- (unsigned long)(total_mem >> 20));
+ printk(KERN_INFO "Reserving %ldMB of memory at %ldMB "
+ "for crashkernel (System RAM: %ldMB)\n",
+ (unsigned long)(crash_size >> 20),
+ (unsigned long)(crash_base >> 20),
+ (unsigned long)(total_mem >> 20));
- crashk_res.start = crash_base;
- crashk_res.end = crash_base + crash_size - 1;
- insert_resource(&iomem_resource, &crashk_res);
- }
+ crashk_res.start = crash_base;
+ crashk_res.end = crash_base + crash_size - 1;
+ insert_resource(&iomem_resource, &crashk_res);
}
#else
void __init reserve_crashkernel(void)
--
1.5.6
On Thu, Jun 26, 2008 at 09:54:08PM +0200, Bernhard Walle wrote:
> This patch removes the need of the crashkernel=...@offset parameter to define
> a fixed offset for crashkernel reservation. That feature can be used together
> with a relocatable kernel where the kexec-tools relocate the kernel and
> get the actual offset from /proc/iomem.
>
> The use case is a kernel where the .text+.data+.bss is after 16M physical
> memory (debug kernel with lockdep on x86_64 can cause that) which caused a
> major pain in autoconfiguration in our distribution.
>
> Also, that patch unifies crashdump architectures a bit since IA64 has
> that semantics from the very beginning of the kdump port.
>
> Please provide feedback!
>
Hi Bernhard,
This looks like a good idea. That means distributions don't have to
hardcode the crashbase at 16MB and the decision to find a free memory
can be left on kernel. Users will also find it easy that way.
>
> Signed-off-by: Bernhard Walle <[email protected]>
> ---
> arch/x86/kernel/setup.c | 70 +++++++++++++++++++++++++++++++++++------------
> 1 files changed, 52 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index a81d82c..c30bb7b 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -435,6 +435,34 @@ static inline unsigned long long get_total_mem(void)
> }
>
> #ifdef CONFIG_KEXEC
> +
> +/**
> + * Reserve @size bytes of crashkernel memory at any suitable offset.
> + *
> + * @size: Size of the crashkernel memory to reserve.
> + * Returns the base address on success, and -1ULL on failure.
> + */
> +unsigned long long find_and_reserve_crashkernel(unsigned long long size)
> +{
> + const unsigned long long alignment = 16<<20; /* 16M */
> + unsigned long long start = 0LL;
> +
> + while (1) {
> + int ret;
> +
> + start = find_e820_area(start, ULONG_MAX, size, alignment);
> + if (start == -1ULL)
> + return start;
> +
> + /* try to reserve it */
> + ret = reserve_bootmem_generic(start, size, BOOTMEM_EXCLUSIVE);
> + if (ret >= 0)
> + return start;
> +
> + start += alignment;
> + }
I think both i386 and x86_64 relocatable kernels had some upper limits
where these could be loaded (Eric had mentioned those in the patch. I
don't remember these). It might be a good idea to capture it here
making sure "start" does not cross those limits otherwise don't reserve
the memory.
Thanks
Vivek
On Fri, Jun 27, 2008 at 09:32:56AM -0400, Vivek Goyal wrote:
> On Thu, Jun 26, 2008 at 09:54:08PM +0200, Bernhard Walle wrote:
> > This patch removes the need of the crashkernel=...@offset parameter to define
> > a fixed offset for crashkernel reservation. That feature can be used together
> > with a relocatable kernel where the kexec-tools relocate the kernel and
> > get the actual offset from /proc/iomem.
> >
> > The use case is a kernel where the .text+.data+.bss is after 16M physical
> > memory (debug kernel with lockdep on x86_64 can cause that) which caused a
> > major pain in autoconfiguration in our distribution.
> >
> > Also, that patch unifies crashdump architectures a bit since IA64 has
> > that semantics from the very beginning of the kdump port.
> >
> > Please provide feedback!
> >
>
> Hi Bernhard,
>
> This looks like a good idea. That means distributions don't have to
> hardcode the crashbase at 16MB and the decision to find a free memory
> can be left on kernel. Users will also find it easy that way.
>
> >
> > Signed-off-by: Bernhard Walle <[email protected]>
> > ---
> > arch/x86/kernel/setup.c | 70 +++++++++++++++++++++++++++++++++++------------
> > 1 files changed, 52 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index a81d82c..c30bb7b 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -435,6 +435,34 @@ static inline unsigned long long get_total_mem(void)
> > }
> >
> > #ifdef CONFIG_KEXEC
> > +
> > +/**
> > + * Reserve @size bytes of crashkernel memory at any suitable offset.
> > + *
> > + * @size: Size of the crashkernel memory to reserve.
> > + * Returns the base address on success, and -1ULL on failure.
> > + */
> > +unsigned long long find_and_reserve_crashkernel(unsigned long long size)
> > +{
> > + const unsigned long long alignment = 16<<20; /* 16M */
> > + unsigned long long start = 0LL;
> > +
> > + while (1) {
> > + int ret;
> > +
> > + start = find_e820_area(start, ULONG_MAX, size, alignment);
> > + if (start == -1ULL)
> > + return start;
> > +
> > + /* try to reserve it */
> > + ret = reserve_bootmem_generic(start, size, BOOTMEM_EXCLUSIVE);
> > + if (ret >= 0)
> > + return start;
> > +
> > + start += alignment;
> > + }
>
> I think both i386 and x86_64 relocatable kernels had some upper limits
> where these could be loaded (Eric had mentioned those in the patch. I
> don't remember these). It might be a good idea to capture it here
> making sure "start" does not cross those limits otherwise don't reserve
> the memory.
>
Thinking more about. Let me step back. I think it is not good idea to
take this kernel take decision about the capability of kernel being
loaded. There is no way we can find out now that if a kernel is capable
of running from this memory location or not. This is highly variable. So,
please ignore above comment.
This patch looks good to me.
Acked-by: Vivek Goyal <[email protected]>
Thanks
Vivek
* Vivek Goyal [2008-06-27 09:42]:
>
> Thinking more about. Let me step back. I think it is not good idea to
> take this kernel take decision about the capability of kernel being
> loaded. There is no way we can find out now that if a kernel is capable
> of running from this memory location or not. This is highly variable. So,
> please ignore above comment.
Maybe below 4G makes sense. Because you need some 32 bit memory for DMA.
That's mostly an architecture limitation, so that could make sense to
check here.
But I don't have any strong opinion about that.
Bernhard
--
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development
On Fri, Jun 27, 2008 at 04:06:56PM +0200, Bernhard Walle wrote:
> * Vivek Goyal [2008-06-27 09:42]:
> >
> > Thinking more about. Let me step back. I think it is not good idea to
> > take this kernel take decision about the capability of kernel being
> > loaded. There is no way we can find out now that if a kernel is capable
> > of running from this memory location or not. This is highly variable. So,
> > please ignore above comment.
>
> Maybe below 4G makes sense. Because you need some 32 bit memory for DMA.
> That's mostly an architecture limitation, so that could make sense to
> check here.
>
Even if you need some 32bit memory in the lower regions, kexec can handle
it with backup segment mechanism (currently 640K). We can always increse
the backup region size.
So I would think that it is best to leave it without any checking and
then let kexec-tools handle it.
Thanks
Vivek
* Vivek Goyal [2008-06-27 10:19]:
> On Fri, Jun 27, 2008 at 04:06:56PM +0200, Bernhard Walle wrote:
> > * Vivek Goyal [2008-06-27 09:42]:
> > >
> > > Thinking more about. Let me step back. I think it is not good idea to
> > > take this kernel take decision about the capability of kernel being
> > > loaded. There is no way we can find out now that if a kernel is capable
> > > of running from this memory location or not. This is highly variable. So,
> > > please ignore above comment.
> >
> > Maybe below 4G makes sense. Because you need some 32 bit memory for DMA.
> > That's mostly an architecture limitation, so that could make sense to
> > check here.
>
> Even if you need some 32bit memory in the lower regions, kexec can handle
> it with backup segment mechanism (currently 640K). We can always increse
> the backup region size.
>
> So I would think that it is best to leave it without any checking and
> then let kexec-tools handle it.
Ah, that's true. Only on x86, right? (That would be an alternative for
ia64, too ...)
But in general policy should go in userspace (if possible), so I agree
with you that kexec-tools can handle that.
Bernhard
--
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development
Bernhard Walle <[email protected]> writes:
> Ah, that's true. Only on x86, right? (That would be an alternative for
> ia64, too ...)
>
> But in general policy should go in userspace (if possible), so I agree
> with you that kexec-tools can handle that.
At a quick skim the patch looks good. I thought I had initially implemented
the code to work this way but apparently in all of the churn that aspect of it
got lost.
Let's start with trying to allocate the memory as low as possible for
a crash kernel (I think you are already doing that).
If we need something more we can add another meta character to specify a limit.
/sbin/kexec should balk at loading if the reserved region is too high.
Ramdisks in particular are a problem because they need to be in low mem.
Eric
* [email protected] (Eric W. Biederman) [2008-06-27 11:00]:
> Bernhard Walle <[email protected]> writes:
>
> > Ah, that's true. Only on x86, right? (That would be an alternative for
> > ia64, too ...)
> >
> > But in general policy should go in userspace (if possible), so I agree
> > with you that kexec-tools can handle that.
>
> At a quick skim the patch looks good. I thought I had initially implemented
> the code to work this way but apparently in all of the churn that aspect of it
> got lost.
>
> Let's start with trying to allocate the memory as low as possible for
> a crash kernel (I think you are already doing that).
Yes, in 95 % of the cases the crashkernel still gets reserved at 16M.
Bernhard
--
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development
* Bernhard Walle <[email protected]> wrote:
> > > But in general policy should go in userspace (if possible), so I
> > > agree with you that kexec-tools can handle that.
> >
> > At a quick skim the patch looks good. I thought I had initially
> > implemented the code to work this way but apparently in all of the
> > churn that aspect of it got lost.
> >
> > Let's start with trying to allocate the memory as low as possible
> > for a crash kernel (I think you are already doing that).
>
> Yes, in 95 % of the cases the crashkernel still gets reserved at 16M.
ok - i've applied it to tip/x86/crashdump - thanks Bernhard.
Ingo
On Fri, Jun 27, 2008 at 6:32 AM, Vivek Goyal <[email protected]> wrote:
> On Thu, Jun 26, 2008 at 09:54:08PM +0200, Bernhard Walle wrote:
>> This patch removes the need of the crashkernel=...@offset parameter to define
>> a fixed offset for crashkernel reservation. That feature can be used together
>> with a relocatable kernel where the kexec-tools relocate the kernel and
>> get the actual offset from /proc/iomem.
>>
>> The use case is a kernel where the .text+.data+.bss is after 16M physical
>> memory (debug kernel with lockdep on x86_64 can cause that) which caused a
>> major pain in autoconfiguration in our distribution.
>>
>> Also, that patch unifies crashdump architectures a bit since IA64 has
>> that semantics from the very beginning of the kdump port.
>>
>> Please provide feedback!
>>
>
> Hi Bernhard,
>
> This looks like a good idea. That means distributions don't have to
> hardcode the crashbase at 16MB and the decision to find a free memory
> can be left on kernel. Users will also find it easy that way.
>
>>
>> Signed-off-by: Bernhard Walle <[email protected]>
>> ---
>> arch/x86/kernel/setup.c | 70 +++++++++++++++++++++++++++++++++++------------
>> 1 files changed, 52 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index a81d82c..c30bb7b 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -435,6 +435,34 @@ static inline unsigned long long get_total_mem(void)
>> }
>>
>> #ifdef CONFIG_KEXEC
>> +
>> +/**
>> + * Reserve @size bytes of crashkernel memory at any suitable offset.
>> + *
>> + * @size: Size of the crashkernel memory to reserve.
>> + * Returns the base address on success, and -1ULL on failure.
>> + */
>> +unsigned long long find_and_reserve_crashkernel(unsigned long long size)
>> +{
>> + const unsigned long long alignment = 16<<20; /* 16M */
>> + unsigned long long start = 0LL;
>> +
>> + while (1) {
>> + int ret;
>> +
>> + start = find_e820_area(start, ULONG_MAX, size, alignment);
should use min_t(u64, 1ULL<<32, max_low_pfn<<PAGE_SHIFT) replace ULONG_MAX
YH
* Yinghai Lu [2008-07-14 00:11]:
>
> should use min_t(u64, 1ULL<<32, max_low_pfn<<PAGE_SHIFT) replace ULONG_MAX
Shouldn't we use min_t(u64, ULLONG_MAX, max_low_pfn<<PAGE_SHIFT), i.e.
should we really limit the crashkernel to a 32 bit address on a 64 bit
system?
Bernhard
--
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development
Bernhard Walle <[email protected]> writes:
> * Yinghai Lu [2008-07-14 00:11]:
>>
>> should use min_t(u64, 1ULL<<32, max_low_pfn<<PAGE_SHIFT) replace ULONG_MAX
>
> Shouldn't we use min_t(u64, ULLONG_MAX, max_low_pfn<<PAGE_SHIFT), i.e.
> should we really limit the crashkernel to a 32 bit address on a 64 bit
> system?
We should use the lowest physical address that meets our size and
alignment constraints. However there is no reason to make this be <
4G or even < 1G. The worst case is that we reserve an area the kdump
kernel can't run out of. However /sbin/kexec should check for that as
the kexec on panic code is not necessarily a linux kernel.
Eric
On Mon, Jul 14, 2008 at 2:44 AM, Eric W. Biederman
<[email protected]> wrote:
> Bernhard Walle <[email protected]> writes:
>
>> * Yinghai Lu [2008-07-14 00:11]:
>>>
>>> should use min_t(u64, 1ULL<<32, max_low_pfn<<PAGE_SHIFT) replace ULONG_MAX
>>
>> Shouldn't we use min_t(u64, ULLONG_MAX, max_low_pfn<<PAGE_SHIFT), i.e.
>> should we really limit the crashkernel to a 32 bit address on a 64 bit
>> system?
>
> We should use the lowest physical address that meets our size and
> alignment constraints. However there is no reason to make this be <
> 4G or even < 1G. The worst case is that we reserve an area the kdump
> kernel can't run out of. However /sbin/kexec should check for that as
> the kexec on panic code is not necessarily a linux kernel.
for 64bit, kdump can start from address above 4g with bzImage?
YH
"Yinghai Lu" <[email protected]> writes:
> for 64bit, kdump can start from address above 4g with bzImage?
I think the boot protocol was properly updated for that, it was in the
original drafts. A relocatable kernel with a 64bit entry point.
I do know that the 64bit code is ready to do that.
Eric
On Mon, Jul 14, 2008 at 10:30 AM, Eric W. Biederman
<[email protected]> wrote:
> "Yinghai Lu" <[email protected]> writes:
>
>> for 64bit, kdump can start from address above 4g with bzImage?
>
> I think the boot protocol was properly updated for that, it was in the
> original drafts. A relocatable kernel with a 64bit entry point.
>
> I do know that the 64bit code is ready to do that.
but bzImage for 64bit still use 32bit entries, except kexec could
uncompress the bzImage like xen domain builder.
YH
"Yinghai Lu" <[email protected]> writes:
> On Mon, Jul 14, 2008 at 10:30 AM, Eric W. Biederman
> <[email protected]> wrote:
>> "Yinghai Lu" <[email protected]> writes:
>>
>>> for 64bit, kdump can start from address above 4g with bzImage?
>>
>> I think the boot protocol was properly updated for that, it was in the
>> original drafts. A relocatable kernel with a 64bit entry point.
>>
>> I do know that the 64bit code is ready to do that.
>
> but bzImage for 64bit still use 32bit entries, except kexec could
> uncompress the bzImage like xen domain builder.
>From the standpoint of the crashdump option it doesn't matter.
As the vmlinux supports it, and bzImage could easily support it,
the decompresser is 64bit code.
If bzImage doesn't support it that is a bug we will have to
correct one of these years when it actually matters.
Eric
* Eric W. Biederman [2008-07-14 11:41]:
>
> "Yinghai Lu" <[email protected]> writes:
>
> > On Mon, Jul 14, 2008 at 10:30 AM, Eric W. Biederman
> > <[email protected]> wrote:
> >> "Yinghai Lu" <[email protected]> writes:
> >>
> >>> for 64bit, kdump can start from address above 4g with bzImage?
> >>
> >> I think the boot protocol was properly updated for that, it was in the
> >> original drafts. A relocatable kernel with a 64bit entry point.
> >>
> >> I do know that the 64bit code is ready to do that.
> >
> > but bzImage for 64bit still use 32bit entries, except kexec could
> > uncompress the bzImage like xen domain builder.
>
> From the standpoint of the crashdump option it doesn't matter.
> As the vmlinux supports it, and bzImage could easily support it,
> the decompresser is 64bit code.
>
> If bzImage doesn't support it that is a bug we will have to
> correct one of these years when it actually matters.
But such checks belong into kexec itself, not into the kernel.
Bernhard
--
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development
Bernhard Walle <[email protected]> writes:
> But such checks belong into kexec itself, not into the kernel.
Exactly.
All we should do in the kernel is bias the search for a low address.
As that is more usable.
Eric
On Mon, Jul 14, 2008 at 11:55 AM, Eric W. Biederman
<[email protected]> wrote:
> Bernhard Walle <[email protected]> writes:
>
>> But such checks belong into kexec itself, not into the kernel.
>
> Exactly.
>
> All we should do in the kernel is bias the search for a low address.
> As that is more usable.
let kexec to uncompress the bzImage (need to check if header version
is 2.0.8(?) above?) , if the kdump_base is above 4g.
YH