[PATCH 0/3] Some cleanups and fixes

[PATCH 0/3] Some cleanups and fixes

[Yuntao Wang]

This series includes two cleanups and one fix.

Yuntao Wang (3):
  x86/crash: remove the unused image parameter from
    prepare_elf_headers()
  x86/crash: use SZ_1M macro instead of hardcoded value
  crash_core: fix and simplify the logic of crash_exclude_mem_range()

 arch/x86/kernel/crash.c | 12 +++----
 kernel/crash_core.c     | 70 +++++++++++------------------------------
 2 files changed, 25 insertions(+), 57 deletions(-)

-- 
2.43.0

[PATCH 1/3] x86/crash: remove the unused image parameter from prepare_elf_headers()

[Yuntao Wang]

The image parameter is no longer in use, remove it. Also, tidy up the code
formatting.

Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 arch/x86/kernel/crash.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..792231a56d11 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -198,8 +198,8 @@ static int prepare_elf64_ram_headers_callback(struct resource *res, void *arg)
 }
 
 /* Prepare elf headers. Return addr and size */
-static int prepare_elf_headers(struct kimage *image, void **addr,
-					unsigned long *sz, unsigned long *nr_mem_ranges)
+static int prepare_elf_headers(void **addr, unsigned long *sz,
+			       unsigned long *nr_mem_ranges)
 {
 	struct crash_mem *cmem;
 	int ret;
@@ -221,7 +221,7 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
 	*nr_mem_ranges = cmem->nr_ranges;
 
 	/* By default prepare 64bit headers */
-	ret =  crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
+	ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
 
 out:
 	vfree(cmem);
@@ -349,7 +349,7 @@ int crash_load_segments(struct kimage *image)
 				  .buf_max = ULONG_MAX, .top_down = false };
 
 	/* Prepare elf headers and add a segment */
-	ret = prepare_elf_headers(image, &kbuf.buffer, &kbuf.bufsz, &pnum);
+	ret = prepare_elf_headers(&kbuf.buffer, &kbuf.bufsz, &pnum);
 	if (ret)
 		return ret;
 
@@ -452,7 +452,7 @@ void arch_crash_handle_hotplug_event(struct kimage *image)
 	 * Create the new elfcorehdr reflecting the changes to CPU and/or
 	 * memory resources.
 	 */
-	if (prepare_elf_headers(image, &elfbuf, &elfsz, &nr_mem_ranges)) {
+	if (prepare_elf_headers(&elfbuf, &elfsz, &nr_mem_ranges)) {
 		pr_err("unable to create new elfcorehdr");
 		goto out;
 	}
-- 
2.43.0

Re: [PATCH 1/3] x86/crash: remove the unused image parameter from prepare_elf_headers()

[Baoquan He]

On 12/15/23 at 12:38am, Yuntao Wang wrote:
> The image parameter is no longer in use, remove it. Also, tidy up the code
> formatting.
> 
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
>  arch/x86/kernel/crash.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Acked-by: Baoquan He <bhe@redhat.com>

> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index c92d88680dbf..792231a56d11 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -198,8 +198,8 @@ static int prepare_elf64_ram_headers_callback(struct resource *res, void *arg)
>  }
>  
>  /* Prepare elf headers. Return addr and size */
> -static int prepare_elf_headers(struct kimage *image, void **addr,
> -					unsigned long *sz, unsigned long *nr_mem_ranges)
> +static int prepare_elf_headers(void **addr, unsigned long *sz,
> +			       unsigned long *nr_mem_ranges)
>  {
>  	struct crash_mem *cmem;
>  	int ret;
> @@ -221,7 +221,7 @@ static int prepare_elf_headers(struct kimage *image, void **addr,
>  	*nr_mem_ranges = cmem->nr_ranges;
>  
>  	/* By default prepare 64bit headers */
> -	ret =  crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
> +	ret = crash_prepare_elf64_headers(cmem, IS_ENABLED(CONFIG_X86_64), addr, sz);
>  
>  out:
>  	vfree(cmem);
> @@ -349,7 +349,7 @@ int crash_load_segments(struct kimage *image)
>  				  .buf_max = ULONG_MAX, .top_down = false };
>  
>  	/* Prepare elf headers and add a segment */
> -	ret = prepare_elf_headers(image, &kbuf.buffer, &kbuf.bufsz, &pnum);
> +	ret = prepare_elf_headers(&kbuf.buffer, &kbuf.bufsz, &pnum);
>  	if (ret)
>  		return ret;
>  
> @@ -452,7 +452,7 @@ void arch_crash_handle_hotplug_event(struct kimage *image)
>  	 * Create the new elfcorehdr reflecting the changes to CPU and/or
>  	 * memory resources.
>  	 */
> -	if (prepare_elf_headers(image, &elfbuf, &elfsz, &nr_mem_ranges)) {
> +	if (prepare_elf_headers(&elfbuf, &elfsz, &nr_mem_ranges)) {
>  		pr_err("unable to create new elfcorehdr");
>  		goto out;
>  	}
> -- 
> 2.43.0
> 

[PATCH 2/3] x86/crash: use SZ_1M macro instead of hardcoded value

[Yuntao Wang]

Use SZ_1M macro instead of hardcoded 1<<20 to make code more readable.

Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 arch/x86/kernel/crash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 792231a56d11..249b5876e7ec 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -170,7 +170,7 @@ static int elf_header_exclude_ranges(struct crash_mem *cmem)
 	int ret = 0;
 
 	/* Exclude the low 1M because it is always reserved */
-	ret = crash_exclude_mem_range(cmem, 0, (1<<20)-1);
+	ret = crash_exclude_mem_range(cmem, 0, SZ_1M - 1);
 	if (ret)
 		return ret;
 
-- 
2.43.0

Re: [PATCH 2/3] x86/crash: use SZ_1M macro instead of hardcoded value

[Baoquan He]

On 12/15/23 at 12:38am, Yuntao Wang wrote:
> Use SZ_1M macro instead of hardcoded 1<<20 to make code more readable.
> 
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
>  arch/x86/kernel/crash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 792231a56d11..249b5876e7ec 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c

Acked-by: Baoquan He <bhe@redhat.com>

> @@ -170,7 +170,7 @@ static int elf_header_exclude_ranges(struct crash_mem *cmem)
>  	int ret = 0;
>  
>  	/* Exclude the low 1M because it is always reserved */
> -	ret = crash_exclude_mem_range(cmem, 0, (1<<20)-1);
> +	ret = crash_exclude_mem_range(cmem, 0, SZ_1M - 1);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.43.0
> 

[PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Yuntao Wang]

The purpose of crash_exclude_mem_range() is to remove all memory ranges
that overlap with [mstart-mend]. However, the current logic only removes
the first overlapping memory range.

Commit a2e9a95d2190 ("kexec: Improve & fix crash_exclude_mem_range() to
handle overlapping ranges") attempted to address this issue, but it did not
fix all error cases.

Let's fix and simplify the logic of crash_exclude_mem_range().

Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
---
 kernel/crash_core.c | 70 ++++++++++++---------------------------------
 1 file changed, 19 insertions(+), 51 deletions(-)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index efe87d501c8c..0292a4c8bb2f 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -565,9 +565,8 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, int need_kernel_map,
 int crash_exclude_mem_range(struct crash_mem *mem,
 			    unsigned long long mstart, unsigned long long mend)
 {
-	int i, j;
+	int i;
 	unsigned long long start, end, p_start, p_end;
-	struct range temp_range = {0, 0};
 
 	for (i = 0; i < mem->nr_ranges; i++) {
 		start = mem->ranges[i].start;
@@ -575,72 +574,41 @@ int crash_exclude_mem_range(struct crash_mem *mem,
 		p_start = mstart;
 		p_end = mend;
 
-		if (mstart > end || mend < start)
+		if (p_start > end || p_end < start)
 			continue;
 
 		/* Truncate any area outside of range */
-		if (mstart < start)
+		if (p_start < start)
 			p_start = start;
-		if (mend > end)
+		if (p_end > end)
 			p_end = end;
 
 		/* Found completely overlapping range */
 		if (p_start == start && p_end == end) {
-			mem->ranges[i].start = 0;
-			mem->ranges[i].end = 0;
-			if (i < mem->nr_ranges - 1) {
-				/* Shift rest of the ranges to left */
-				for (j = i; j < mem->nr_ranges - 1; j++) {
-					mem->ranges[j].start =
-						mem->ranges[j+1].start;
-					mem->ranges[j].end =
-							mem->ranges[j+1].end;
-				}
-
-				/*
-				 * Continue to check if there are another overlapping ranges
-				 * from the current position because of shifting the above
-				 * mem ranges.
-				 */
-				i--;
-				mem->nr_ranges--;
-				continue;
-			}
+			memmove(&mem->ranges[i], &mem->ranges[i + 1],
+				(mem->nr_ranges - (i + 1)) * sizeof(mem->ranges[i]));
+			i--;
 			mem->nr_ranges--;
-			return 0;
-		}
-
-		if (p_start > start && p_end < end) {
+		} else if (p_start > start && p_end < end) {
 			/* Split original range */
+			if (mem->nr_ranges >= mem->max_nr_ranges)
+				return -ENOMEM;
+
+			memmove(&mem->ranges[i + 2], &mem->ranges[i + 1],
+				(mem->nr_ranges - (i + 1)) * sizeof(mem->ranges[i]));
+
 			mem->ranges[i].end = p_start - 1;
-			temp_range.start = p_end + 1;
-			temp_range.end = end;
+			mem->ranges[i + 1].start = p_end + 1;
+			mem->ranges[i + 1].end = end;
+
+			i++;
+			mem->nr_ranges++;
 		} else if (p_start != start)
 			mem->ranges[i].end = p_start - 1;
 		else
 			mem->ranges[i].start = p_end + 1;
-		break;
-	}
-
-	/* If a split happened, add the split to array */
-	if (!temp_range.end)
-		return 0;
-
-	/* Split happened */
-	if (i == mem->max_nr_ranges - 1)
-		return -ENOMEM;
-
-	/* Location where new range should go */
-	j = i + 1;
-	if (j < mem->nr_ranges) {
-		/* Move over all ranges one slot towards the end */
-		for (i = mem->nr_ranges - 1; i >= j; i--)
-			mem->ranges[i + 1] = mem->ranges[i];
 	}
 
-	mem->ranges[j].start = temp_range.start;
-	mem->ranges[j].end = temp_range.end;
-	mem->nr_ranges++;
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Baoquan He]

On 12/15/23 at 12:38am, Yuntao Wang wrote:
> The purpose of crash_exclude_mem_range() is to remove all memory ranges
> that overlap with [mstart-mend]. However, the current logic only removes
> the first overlapping memory range.
> 
> Commit a2e9a95d2190 ("kexec: Improve & fix crash_exclude_mem_range() to
> handle overlapping ranges") attempted to address this issue, but it did not
> fix all error cases.

Hmm, this is a specific function for kdump kernel loading. So far it's
sufficiently meet demands. Say so because we only need to exclude
crashk_res and crashk_low_res when constructing elfcorehdr. region
crashk_res/crashk_low_res are digged out from system RAM region. That's
why the break is taken in the for loop in the current code. X86 needs
exclude low 1M, the low 1M could span several system RAM regions because
BIOS under low 1M reserved some spaces. And the elfcorehdr exluding from
crashkernel region taken in x86 is also a splitting.

Generally speaking, crashk_res/crashk_low_res is inside a big chunk of
continuous region. On x86, low 1M spans several complete region on x86,
elfcorehdr region is inside continuous crashk_res region.

You can see why crash_exclude_mem_range() looks like now it is. This patch
makes crash_exclude_mem_range() be a generic region removing function. I do
see the memmove can improve code readbility, while I have concern about the
while loop.

Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
Then after excluding the 256M from a region, it should stop. But now, this patch
will make it continue scanning. Not sure if it's all in my mind.

> 
> Let's fix and simplify the logic of crash_exclude_mem_range().
> 
> Signed-off-by: Yuntao Wang <ytcoode@gmail.com>
> ---
>  kernel/crash_core.c | 70 ++++++++++++---------------------------------
>  1 file changed, 19 insertions(+), 51 deletions(-)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index efe87d501c8c..0292a4c8bb2f 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -565,9 +565,8 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, int need_kernel_map,
>  int crash_exclude_mem_range(struct crash_mem *mem,
>  			    unsigned long long mstart, unsigned long long mend)
>  {
> -	int i, j;
> +	int i;
>  	unsigned long long start, end, p_start, p_end;
> -	struct range temp_range = {0, 0};
>  
>  	for (i = 0; i < mem->nr_ranges; i++) {
>  		start = mem->ranges[i].start;
> @@ -575,72 +574,41 @@ int crash_exclude_mem_range(struct crash_mem *mem,
>  		p_start = mstart;
>  		p_end = mend;
>  
> -		if (mstart > end || mend < start)
> +		if (p_start > end || p_end < start)
>  			continue;
>  
>  		/* Truncate any area outside of range */
> -		if (mstart < start)
> +		if (p_start < start)
>  			p_start = start;
> -		if (mend > end)
> +		if (p_end > end)
>  			p_end = end;
>  
>  		/* Found completely overlapping range */
>  		if (p_start == start && p_end == end) {
> -			mem->ranges[i].start = 0;
> -			mem->ranges[i].end = 0;
> -			if (i < mem->nr_ranges - 1) {
> -				/* Shift rest of the ranges to left */
> -				for (j = i; j < mem->nr_ranges - 1; j++) {
> -					mem->ranges[j].start =
> -						mem->ranges[j+1].start;
> -					mem->ranges[j].end =
> -							mem->ranges[j+1].end;
> -				}
> -
> -				/*
> -				 * Continue to check if there are another overlapping ranges
> -				 * from the current position because of shifting the above
> -				 * mem ranges.
> -				 */
> -				i--;
> -				mem->nr_ranges--;
> -				continue;
> -			}
> +			memmove(&mem->ranges[i], &mem->ranges[i + 1],
> +				(mem->nr_ranges - (i + 1)) * sizeof(mem->ranges[i]));
> +			i--;
>  			mem->nr_ranges--;
> -			return 0;
> -		}
> -
> -		if (p_start > start && p_end < end) {
> +		} else if (p_start > start && p_end < end) {
>  			/* Split original range */
> +			if (mem->nr_ranges >= mem->max_nr_ranges)
> +				return -ENOMEM;
> +
> +			memmove(&mem->ranges[i + 2], &mem->ranges[i + 1],
> +				(mem->nr_ranges - (i + 1)) * sizeof(mem->ranges[i]));
> +
>  			mem->ranges[i].end = p_start - 1;
> -			temp_range.start = p_end + 1;
> -			temp_range.end = end;
> +			mem->ranges[i + 1].start = p_end + 1;
> +			mem->ranges[i + 1].end = end;
> +
> +			i++;
> +			mem->nr_ranges++;
>  		} else if (p_start != start)
>  			mem->ranges[i].end = p_start - 1;
>  		else
>  			mem->ranges[i].start = p_end + 1;
> -		break;
> -	}
> -
> -	/* If a split happened, add the split to array */
> -	if (!temp_range.end)
> -		return 0;
> -
> -	/* Split happened */
> -	if (i == mem->max_nr_ranges - 1)
> -		return -ENOMEM;
> -
> -	/* Location where new range should go */
> -	j = i + 1;
> -	if (j < mem->nr_ranges) {
> -		/* Move over all ranges one slot towards the end */
> -		for (i = mem->nr_ranges - 1; i >= j; i--)
> -			mem->ranges[i + 1] = mem->ranges[i];
>  	}
>  
> -	mem->ranges[j].start = temp_range.start;
> -	mem->ranges[j].end = temp_range.end;
> -	mem->nr_ranges++;
>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Yuntao Wang]

On Fri, 15 Dec 2023 23:15:10 +0800, Baoquan He wrote:
> On 12/15/23 at 12:38am, Yuntao Wang wrote:
> > The purpose of crash_exclude_mem_range() is to remove all memory ranges
> > that overlap with [mstart-mend]. However, the current logic only removes
> > the first overlapping memory range.
> > 
> > Commit a2e9a95d2190 ("kexec: Improve & fix crash_exclude_mem_range() to
> > handle overlapping ranges") attempted to address this issue, but it did not
> > fix all error cases.
> 
> Hmm, this is a specific function for kdump kernel loading. So far it's
> sufficiently meet demands. Say so because we only need to exclude
> crashk_res and crashk_low_res when constructing elfcorehdr. region
> crashk_res/crashk_low_res are digged out from system RAM region. That's
> why the break is taken in the for loop in the current code. X86 needs
> exclude low 1M, the low 1M could span several system RAM regions because
> BIOS under low 1M reserved some spaces. And the elfcorehdr exluding from
> crashkernel region taken in x86 is also a splitting.
> 
> Generally speaking, crashk_res/crashk_low_res is inside a big chunk of
> continuous region. On x86, low 1M spans several complete region on x86,
> elfcorehdr region is inside continuous crashk_res region.
> 
> You can see why crash_exclude_mem_range() looks like now it is. This patch
> makes crash_exclude_mem_range() be a generic region removing function. I do
> see the memmove can improve code readbility, while I have concern about the
> while loop.
> 
> Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
> Then after excluding the 256M from a region, it should stop. But now, this patch
> will make it continue scanning. Not sure if it's all in my mind.

Hi Baoquan,

Thank you for such a detailed reply. Now I finally understand why the code is
written this way.

However, if we can guarantee its correctness, wouldn't it be better to use the
generic region removing logic? At least it is more concise and clear, and other
people reading this code for the first time wouldn't get confused like me.

As for your concern about the while loop, I think it wouldn't affect performance
much because the total number of loops is small.

Sincerely,
Yuntao

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Baoquan He]

On 12/16/23 at 09:54am, Yuntao Wang wrote:
> On Fri, 15 Dec 2023 23:15:10 +0800, Baoquan He wrote:
> > On 12/15/23 at 12:38am, Yuntao Wang wrote:
> > > The purpose of crash_exclude_mem_range() is to remove all memory ranges
> > > that overlap with [mstart-mend]. However, the current logic only removes
> > > the first overlapping memory range.
> > > 
> > > Commit a2e9a95d2190 ("kexec: Improve & fix crash_exclude_mem_range() to
> > > handle overlapping ranges") attempted to address this issue, but it did not
> > > fix all error cases.
> > 
> > Hmm, this is a specific function for kdump kernel loading. So far it's
> > sufficiently meet demands. Say so because we only need to exclude
> > crashk_res and crashk_low_res when constructing elfcorehdr. region
> > crashk_res/crashk_low_res are digged out from system RAM region. That's
> > why the break is taken in the for loop in the current code. X86 needs
> > exclude low 1M, the low 1M could span several system RAM regions because
> > BIOS under low 1M reserved some spaces. And the elfcorehdr exluding from
> > crashkernel region taken in x86 is also a splitting.
> > 
> > Generally speaking, crashk_res/crashk_low_res is inside a big chunk of
> > continuous region. On x86, low 1M spans several complete region on x86,
> > elfcorehdr region is inside continuous crashk_res region.
> > 
> > You can see why crash_exclude_mem_range() looks like now it is. This patch
> > makes crash_exclude_mem_range() be a generic region removing function. I do
> > see the memmove can improve code readbility, while I have concern about the
> > while loop.
> > 
> > Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
> > Then after excluding the 256M from a region, it should stop. But now, this patch
> > will make it continue scanning. Not sure if it's all in my mind.
> 
> Hi Baoquan,
> 
> Thank you for such a detailed reply. Now I finally understand why the code is
> written this way.
> 
> However, if we can guarantee its correctness, wouldn't it be better to use the
> generic region removing logic? At least it is more concise and clear, and other
> people reading this code for the first time wouldn't get confused like me.
> 
> As for your concern about the while loop, I think it wouldn't affect performance
> much because the total number of loops is small.

Well, see below kexec-tools commit, you wouldn't say that. And when you
understand the code, you will feel a little uncomfortable about the
sustaining useless scanning. At least, we should stop scanning after
needed exluding is done.

Or, we may need add a generic region removing function so that it
can be shared, e.g e820 memory region removing, memblock region removing.
Otherwise, I can't see why a specific region excluding need a generic 
region removing function.

commit 4a6d67d9e938a7accf128aff23f8ad4bda67f729
Author: Xunlei Pang <xlpang@redhat.com>
Date:   Thu Mar 23 19:16:59 2017 +0800

    x86: Support large number of memory ranges
    
    We got a problem on one SGI 64TB machine, the current kexec-tools
    failed to work due to the insufficient ranges(MAX_MEMORY_RANGES)
    allowed which is defined as 1024(less than the ranges on the machine).
    The kcore header is insufficient due to the same reason as well.
    
    To solve this, this patch simply doubles "MAX_MEMORY_RANGES" and
    "KCORE_ELF_HEADERS_SIZE".
    
    Signed-off-by: Xunlei Pang <xlpang@redhat.com>
    Tested-by: Frank Ramsay <frank.ramsay@hpe.com>
    Signed-off-by: Simon Horman <horms@verge.net.au>

diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
index 33df3524f4e2..51855f8db762 100644
--- a/kexec/arch/i386/kexec-x86.h
+++ b/kexec/arch/i386/kexec-x86.h
@@ -1,7 +1,7 @@
 #ifndef KEXEC_X86_H
 #define KEXEC_X86_H
 
-#define MAX_MEMORY_RANGES 1024
+#define MAX_MEMORY_RANGES 2048
 
 enum coretype {
        CORE_TYPE_UNDEF = 0,
> 

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Andrew Morton]

On Sat, 16 Dec 2023 11:31:04 +0800 Baoquan He <bhe@redhat.com> wrote:

> > > Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
> > > Then after excluding the 256M from a region, it should stop. But now, this patch
> > > will make it continue scanning. Not sure if it's all in my mind.
> > 
> > Hi Baoquan,
> > 
> > Thank you for such a detailed reply. Now I finally understand why the code is
> > written this way.
> > 
> > However, if we can guarantee its correctness, wouldn't it be better to use the
> > generic region removing logic? At least it is more concise and clear, and other
> > people reading this code for the first time wouldn't get confused like me.
> > 
> > As for your concern about the while loop, I think it wouldn't affect performance
> > much because the total number of loops is small.
> 
> Well, see below kexec-tools commit, you wouldn't say that. And when you
> understand the code, you will feel a little uncomfortable about the
> sustaining useless scanning. At least, we should stop scanning after
> needed exluding is done.
> 
> Or, we may need add a generic region removing function so that it
> can be shared, e.g e820 memory region removing, memblock region removing.
> Otherwise, I can't see why a specific region excluding need a generic 
> region removing function.

So where do we now stand on this patchset?

Thanks.

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Baoquan He]

On 12/29/23 at 12:10pm, Andrew Morton wrote:
> On Sat, 16 Dec 2023 11:31:04 +0800 Baoquan He <bhe@redhat.com> wrote:
> 
> > > > Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
> > > > Then after excluding the 256M from a region, it should stop. But now, this patch
> > > > will make it continue scanning. Not sure if it's all in my mind.
> > > 
> > > Hi Baoquan,
> > > 
> > > Thank you for such a detailed reply. Now I finally understand why the code is
> > > written this way.
> > > 
> > > However, if we can guarantee its correctness, wouldn't it be better to use the
> > > generic region removing logic? At least it is more concise and clear, and other
> > > people reading this code for the first time wouldn't get confused like me.
> > > 
> > > As for your concern about the while loop, I think it wouldn't affect performance
> > > much because the total number of loops is small.
> > 
> > Well, see below kexec-tools commit, you wouldn't say that. And when you
> > understand the code, you will feel a little uncomfortable about the
> > sustaining useless scanning. At least, we should stop scanning after
> > needed exluding is done.
> > 
> > Or, we may need add a generic region removing function so that it
> > can be shared, e.g e820 memory region removing, memblock region removing.
> > Otherwise, I can't see why a specific region excluding need a generic 
> > region removing function.
> 
> So where do we now stand on this patchset?

The patch 1 and 2 are good clean up. The patch 3 plus below one, the
entire is a good code improvement patch.

[PATCH] crash_core: optimize crash_exclude_mem_range()
https://lore.kernel.org/all/20231219163418.108591-1-ytcoode@gmail.com/T/#u

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Baoquan He]

Hi Yuntao,

On 12/30/23 at 06:16pm, Baoquan He wrote:
> On 12/29/23 at 12:10pm, Andrew Morton wrote:
> > On Sat, 16 Dec 2023 11:31:04 +0800 Baoquan He <bhe@redhat.com> wrote:
> > 
> > > > > Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
> > > > > Then after excluding the 256M from a region, it should stop. But now, this patch
> > > > > will make it continue scanning. Not sure if it's all in my mind.
> > > > 
> > > > Hi Baoquan,
> > > > 
> > > > Thank you for such a detailed reply. Now I finally understand why the code is
> > > > written this way.
> > > > 
> > > > However, if we can guarantee its correctness, wouldn't it be better to use the
> > > > generic region removing logic? At least it is more concise and clear, and other
> > > > people reading this code for the first time wouldn't get confused like me.
> > > > 
> > > > As for your concern about the while loop, I think it wouldn't affect performance
> > > > much because the total number of loops is small.
> > > 
> > > Well, see below kexec-tools commit, you wouldn't say that. And when you
> > > understand the code, you will feel a little uncomfortable about the
> > > sustaining useless scanning. At least, we should stop scanning after
> > > needed exluding is done.
> > > 
> > > Or, we may need add a generic region removing function so that it
> > > can be shared, e.g e820 memory region removing, memblock region removing.
> > > Otherwise, I can't see why a specific region excluding need a generic 
> > > region removing function.
> > 
> > So where do we now stand on this patchset?
> 
> The patch 1 and 2 are good clean up. The patch 3 plus below one, the
> entire is a good code improvement patch.
> 
> [PATCH] crash_core: optimize crash_exclude_mem_range()
> https://lore.kernel.org/all/20231219163418.108591-1-ytcoode@gmail.com/T/#u

Can you repost this patchset with some updating, e.g adding ack in patch
1 and 2, and squash below patch into patch 3? This will be easier for
patch merging.

[PATCH] crash_core: optimize crash_exclude_mem_range()
https://lore.kernel.org/all/20231219163418.108591-1-ytcoode@gmail.com/T/#u

And, you may need to drop below patchset since patch 2 conflicts with
this patchset, and patch 1 is conflicting with fuqiang's patch.

[PATCH 0/2] crash: fix potential cmem->ranges array overflow

Thanks
Baoquan

Re: [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()

[Yuntao Wang]

On Tue, 2 Jan 2024 16:41:17 +0800, Baoquan He <bhe@redhat.com> wrote:

> Hi Yuntao,
> 
> On 12/30/23 at 06:16pm, Baoquan He wrote:
> > On 12/29/23 at 12:10pm, Andrew Morton wrote:
> > > On Sat, 16 Dec 2023 11:31:04 +0800 Baoquan He <bhe@redhat.com> wrote:
> > > 
> > > > > > Imagine we have a crashkernel region 256M reserved under 4G, say [2G, 2G+256M].
> > > > > > Then after excluding the 256M from a region, it should stop. But now, this patch
> > > > > > will make it continue scanning. Not sure if it's all in my mind.
> > > > > 
> > > > > Hi Baoquan,
> > > > > 
> > > > > Thank you for such a detailed reply. Now I finally understand why the code is
> > > > > written this way.
> > > > > 
> > > > > However, if we can guarantee its correctness, wouldn't it be better to use the
> > > > > generic region removing logic? At least it is more concise and clear, and other
> > > > > people reading this code for the first time wouldn't get confused like me.
> > > > > 
> > > > > As for your concern about the while loop, I think it wouldn't affect performance
> > > > > much because the total number of loops is small.
> > > > 
> > > > Well, see below kexec-tools commit, you wouldn't say that. And when you
> > > > understand the code, you will feel a little uncomfortable about the
> > > > sustaining useless scanning. At least, we should stop scanning after
> > > > needed exluding is done.
> > > > 
> > > > Or, we may need add a generic region removing function so that it
> > > > can be shared, e.g e820 memory region removing, memblock region removing.
> > > > Otherwise, I can't see why a specific region excluding need a generic 
> > > > region removing function.
> > > 
> > > So where do we now stand on this patchset?
> > 
> > The patch 1 and 2 are good clean up. The patch 3 plus below one, the
> > entire is a good code improvement patch.
> > 
> > [PATCH] crash_core: optimize crash_exclude_mem_range()
> > https://lore.kernel.org/all/20231219163418.108591-1-ytcoode@gmail.com/T/#u
> 
> Can you repost this patchset with some updating, e.g adding ack in patch
> 1 and 2, and squash below patch into patch 3? This will be easier for
> patch merging.
> 
> [PATCH] crash_core: optimize crash_exclude_mem_range()
> https://lore.kernel.org/all/20231219163418.108591-1-ytcoode@gmail.com/T/#u
> 
> And, you may need to drop below patchset since patch 2 conflicts with
> this patchset, and patch 1 is conflicting with fuqiang's patch.
> 
> [PATCH 0/2] crash: fix potential cmem->ranges array overflow
> 
> Thanks
> Baoquan

Hi Baoquan,

I've reposted this patchset, the link to the v2 version of this patchset is:

https://lore.kernel.org/lkml/20240102144905.110047-1-ytcoode@gmail.com/t/#u