Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461606 > unrolled thread

[PATCH] kexec: Account crashk_low_res to kexec_crash_size

Back to article view | Back to linux.kernel

Contents

  [PATCH] kexec: Account crashk_low_res to kexec_crash_size Xunlei Pang <xlpang@redhat.com> - 2016-08-13 10:30 +0200
    Re: [PATCH] kexec: Account crashk_low_res to kexec_crash_size Dave Young <dyoung@redhat.com> - 2016-08-15 09:20 +0200
      Re: [PATCH] kexec: Account crashk_low_res to kexec_crash_size Xunlei Pang <xpang@redhat.com> - 2016-08-15 10:10 +0200
        Re: [PATCH] kexec: Account crashk_low_res to kexec_crash_size Dave Young <dyoung@redhat.com> - 2016-08-16 12:50 +0200

#1461606 — [PATCH] kexec: Account crashk_low_res to kexec_crash_size

"/sys/kernel/kexec_crash_size" only includes crashk_res, it
is fine in most cases, but sometimes we have crashk_low_res.
For example, when "crashkernel=size[KMG],high" combined with
"crashkernel=size[KMG],low" is used for 64-bit x86.

Let "/sys/kernel/kexec_crash_size" reflect all the reserved
memory including crashk_low_res, this is more understandable
from its naming.

Although we can get all the crash memory from "/proc/iomem"
by filtering all "Crash kernel" keyword, it is more convenient
to utilize this file, and the two ways should stay consistent.

Note that write to "/sys/kernel/kexec_crash_size" is to shrink
the reserved memory, and we want to shrink crashk_res only.
So we add some additional check in crash_shrink_memory() since
crashk_low_res now is involved.

Signed-off-by: Xunlei Pang <xlpang@redhat.com>
---
 kernel/kexec_core.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 5616755..d5ae780 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -932,6 +932,8 @@ size_t crash_get_memory_size(void)
 	mutex_lock(&kexec_mutex);
 	if (crashk_res.end != crashk_res.start)
 		size = resource_size(&crashk_res);
+	if (crashk_low_res.end != crashk_low_res.start)
+		size += resource_size(&crashk_low_res);
 	mutex_unlock(&kexec_mutex);
 	return size;
 }
@@ -949,7 +951,7 @@ int crash_shrink_memory(unsigned long new_size)
 {
 	int ret = 0;
 	unsigned long start, end;
-	unsigned long old_size;
+	unsigned long low_size, old_size;
 	struct resource *ram_res;
 
 	mutex_lock(&kexec_mutex);
@@ -958,6 +960,17 @@ int crash_shrink_memory(unsigned long new_size)
 		ret = -ENOENT;
 		goto unlock;
 	}
+
+	start = crashk_low_res.start;
+	end = crashk_low_res.end;
+	low_size = (end == 0) ? 0 : end - start + 1;
+	/* Do not shrink crashk_low_res. */
+	if (new_size <= low_size) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	new_size -= low_size;
 	start = crashk_res.start;
 	end = crashk_res.end;
 	old_size = (end == 0) ? 0 : end - start + 1;
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1462592

Hi Xunlei,

On 08/13/16 at 04:26pm, Xunlei Pang wrote:
> "/sys/kernel/kexec_crash_size" only includes crashk_res, it
> is fine in most cases, but sometimes we have crashk_low_res.
> For example, when "crashkernel=size[KMG],high" combined with
> "crashkernel=size[KMG],low" is used for 64-bit x86.
> 
> Let "/sys/kernel/kexec_crash_size" reflect all the reserved
> memory including crashk_low_res, this is more understandable
> from its naming.

Maybe export another file for the kexec_crash_low_size so that
we can clearly get how much the low area is.

> 
> Although we can get all the crash memory from "/proc/iomem"
> by filtering all "Crash kernel" keyword, it is more convenient
> to utilize this file, and the two ways should stay consistent.

Shrink low area does not make much sense, one may either use it or
shrink it to 0.

Actually think more about it, the crashk_low is only for x86,
it might be even better to move it to x86 code instead of in
common code.

Opinion?

Thanks
Dave
> 
> Note that write to "/sys/kernel/kexec_crash_size" is to shrink
> the reserved memory, and we want to shrink crashk_res only.
> So we add some additional check in crash_shrink_memory() since
> crashk_low_res now is involved.
> 
> Signed-off-by: Xunlei Pang <xlpang@redhat.com>
> ---
>  kernel/kexec_core.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 5616755..d5ae780 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -932,6 +932,8 @@ size_t crash_get_memory_size(void)
>  	mutex_lock(&kexec_mutex);
>  	if (crashk_res.end != crashk_res.start)
>  		size = resource_size(&crashk_res);
> +	if (crashk_low_res.end != crashk_low_res.start)
> +		size += resource_size(&crashk_low_res);
>  	mutex_unlock(&kexec_mutex);
>  	return size;
>  }
> @@ -949,7 +951,7 @@ int crash_shrink_memory(unsigned long new_size)
>  {
>  	int ret = 0;
>  	unsigned long start, end;
> -	unsigned long old_size;
> +	unsigned long low_size, old_size;
>  	struct resource *ram_res;
>  
>  	mutex_lock(&kexec_mutex);
> @@ -958,6 +960,17 @@ int crash_shrink_memory(unsigned long new_size)
>  		ret = -ENOENT;
>  		goto unlock;
>  	}
> +
> +	start = crashk_low_res.start;
> +	end = crashk_low_res.end;
> +	low_size = (end == 0) ? 0 : end - start + 1;
> +	/* Do not shrink crashk_low_res. */
> +	if (new_size <= low_size) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	new_size -= low_size;
>  	start = crashk_res.start;
>  	end = crashk_res.end;
>  	old_size = (end == 0) ? 0 : end - start + 1;
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1462622

On 2016/08/15 at 15:17, Dave Young wrote:
> Hi Xunlei,
>
> On 08/13/16 at 04:26pm, Xunlei Pang wrote:
>> "/sys/kernel/kexec_crash_size" only includes crashk_res, it
>> is fine in most cases, but sometimes we have crashk_low_res.
>> For example, when "crashkernel=size[KMG],high" combined with
>> "crashkernel=size[KMG],low" is used for 64-bit x86.
>>
>> Let "/sys/kernel/kexec_crash_size" reflect all the reserved
>> memory including crashk_low_res, this is more understandable
>> from its naming.
> Maybe export another file for the kexec_crash_low_size so that
> we can clearly get how much the low area is.

I'm fine with it.

>> Although we can get all the crash memory from "/proc/iomem"
>> by filtering all "Crash kernel" keyword, it is more convenient
>> to utilize this file, and the two ways should stay consistent.
> Shrink low area does not make much sense, one may either use it or
> shrink it to 0.
>
> Actually think more about it, the crashk_low is only for x86,
> it might be even better to move it to x86 code instead of in
> common code.
>
> Opinion?

crashk_low is defined in kernel/kexec_core.c, it's an architecture independent definition
though it's only used by x86 currently, maybe it can be used by others in the future.
It's why I'm not handling it specifically for x86.

I just tested the original proc interface further, and it can be shrinked to be zero.
So I guess we can ease the restriction on shrinking the low area as well.

What do you think?

Regards,
Xunlei

>
> Thanks
> Dave
>> Note that write to "/sys/kernel/kexec_crash_size" is to shrink
>> the reserved memory, and we want to shrink crashk_res only.
>> So we add some additional check in crash_shrink_memory() since
>> crashk_low_res now is involved.
>>
>> Signed-off-by: Xunlei Pang <xlpang@redhat.com>
>> ---
>>  kernel/kexec_core.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 5616755..d5ae780 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -932,6 +932,8 @@ size_t crash_get_memory_size(void)
>>  	mutex_lock(&kexec_mutex);
>>  	if (crashk_res.end != crashk_res.start)
>>  		size = resource_size(&crashk_res);
>> +	if (crashk_low_res.end != crashk_low_res.start)
>> +		size += resource_size(&crashk_low_res);
>>  	mutex_unlock(&kexec_mutex);
>>  	return size;
>>  }
>> @@ -949,7 +951,7 @@ int crash_shrink_memory(unsigned long new_size)
>>  {
>>  	int ret = 0;
>>  	unsigned long start, end;
>> -	unsigned long old_size;
>> +	unsigned long low_size, old_size;
>>  	struct resource *ram_res;
>>  
>>  	mutex_lock(&kexec_mutex);
>> @@ -958,6 +960,17 @@ int crash_shrink_memory(unsigned long new_size)
>>  		ret = -ENOENT;
>>  		goto unlock;
>>  	}
>> +
>> +	start = crashk_low_res.start;
>> +	end = crashk_low_res.end;
>> +	low_size = (end == 0) ? 0 : end - start + 1;
>> +	/* Do not shrink crashk_low_res. */
>> +	if (new_size <= low_size) {
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	new_size -= low_size;
>>  	start = crashk_res.start;
>>  	end = crashk_res.end;
>>  	old_size = (end == 0) ? 0 : end - start + 1;
>> -- 
>> 1.8.3.1
>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

[toc] | [prev] | [next] | [standalone]

#1463672

Hi,

On 08/15/16 at 04:05pm, Xunlei Pang wrote:
> On 2016/08/15 at 15:17, Dave Young wrote:
> > Hi Xunlei,
> >
> > On 08/13/16 at 04:26pm, Xunlei Pang wrote:
> >> "/sys/kernel/kexec_crash_size" only includes crashk_res, it
> >> is fine in most cases, but sometimes we have crashk_low_res.
> >> For example, when "crashkernel=size[KMG],high" combined with
> >> "crashkernel=size[KMG],low" is used for 64-bit x86.
> >>
> >> Let "/sys/kernel/kexec_crash_size" reflect all the reserved
> >> memory including crashk_low_res, this is more understandable
> >> from its naming.
> > Maybe export another file for the kexec_crash_low_size so that
> > we can clearly get how much the low area is.
> 
> I'm fine with it.
> 
> >> Although we can get all the crash memory from "/proc/iomem"
> >> by filtering all "Crash kernel" keyword, it is more convenient
> >> to utilize this file, and the two ways should stay consistent.
> > Shrink low area does not make much sense, one may either use it or
> > shrink it to 0.
> >
> > Actually think more about it, the crashk_low is only for x86,
> > it might be even better to move it to x86 code instead of in
> > common code.
> >
> > Opinion?
> 
> crashk_low is defined in kernel/kexec_core.c, it's an architecture independent definition
> though it's only used by x86 currently, maybe it can be used by others in the future.
> It's why I'm not handling it specifically for x86.

Ok, we can leave with it since it is in common code from the very
beginning but I doubt that any other arches will use it.

> 
> I just tested the original proc interface further, and it can be shrinked to be zero.
> So I guess we can ease the restriction on shrinking the low area as well.
> 
> What do you think?

Ok, agreed.

Thanks
Dave

> 
> Regards,
> Xunlei
> 
> >
> > Thanks
> > Dave
> >> Note that write to "/sys/kernel/kexec_crash_size" is to shrink
> >> the reserved memory, and we want to shrink crashk_res only.
> >> So we add some additional check in crash_shrink_memory() since
> >> crashk_low_res now is involved.
> >>
> >> Signed-off-by: Xunlei Pang <xlpang@redhat.com>
> >> ---
> >>  kernel/kexec_core.c | 15 ++++++++++++++-
> >>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> >> index 5616755..d5ae780 100644
> >> --- a/kernel/kexec_core.c
> >> +++ b/kernel/kexec_core.c
> >> @@ -932,6 +932,8 @@ size_t crash_get_memory_size(void)
> >>  	mutex_lock(&kexec_mutex);
> >>  	if (crashk_res.end != crashk_res.start)
> >>  		size = resource_size(&crashk_res);
> >> +	if (crashk_low_res.end != crashk_low_res.start)
> >> +		size += resource_size(&crashk_low_res);
> >>  	mutex_unlock(&kexec_mutex);
> >>  	return size;
> >>  }
> >> @@ -949,7 +951,7 @@ int crash_shrink_memory(unsigned long new_size)
> >>  {
> >>  	int ret = 0;
> >>  	unsigned long start, end;
> >> -	unsigned long old_size;
> >> +	unsigned long low_size, old_size;
> >>  	struct resource *ram_res;
> >>  
> >>  	mutex_lock(&kexec_mutex);
> >> @@ -958,6 +960,17 @@ int crash_shrink_memory(unsigned long new_size)
> >>  		ret = -ENOENT;
> >>  		goto unlock;
> >>  	}
> >> +
> >> +	start = crashk_low_res.start;
> >> +	end = crashk_low_res.end;
> >> +	low_size = (end == 0) ? 0 : end - start + 1;
> >> +	/* Do not shrink crashk_low_res. */
> >> +	if (new_size <= low_size) {
> >> +		ret = -EINVAL;
> >> +		goto unlock;
> >> +	}
> >> +
> >> +	new_size -= low_size;
> >>  	start = crashk_res.start;
> >>  	end = crashk_res.end;
> >>  	old_size = (end == 0) ? 0 : end - start + 1;
> >> -- 
> >> 1.8.3.1
> >>
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web