Discussion:
[PATCH] kexec: allocate buffer in top-down, if specified, correctly
AKASHI Takahiro
2017-04-26 08:22:09 UTC
Permalink
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
This behavior is not consistent with locate_hole(hole_end == -1) function
of kexec-tools.

This patch fixes the bug, going though all the memory regions anyway.

Signed-off-by: AKASHI Takahiro <***@linaro.org>
---
kernel/kexec_file.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b118735fea9d..2f131c0d9017 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -373,8 +373,8 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
/* If we are here, we found a suitable memory range */
kbuf->mem = temp_start;

- /* Success, stop navigating through remaining System RAM ranges */
- return 1;
+ /* always return zero, going through all the System RAM ranges */
+ return 0;
}

static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
@@ -439,18 +439,27 @@ static int locate_mem_hole_callback(u64 start, u64 end, void *arg)
*
* Return: The memory walk will stop when func returns a non-zero value
* and that value will be returned. If all free regions are visited without
- * func returning non-zero, then zero will be returned.
+ * func returning non-zero, then kbuf->mem will be additionally checked
+ * for top-down search.
+ * After all, zero will be returned if none of regions fits.
*/
int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
int (*func)(u64, u64, void *))
{
+ int ret;
+
+ kbuf->mem = 0;
if (kbuf->image->type == KEXEC_TYPE_CRASH)
- return walk_iomem_res_desc(crashk_res.desc,
+ ret = walk_iomem_res_desc(crashk_res.desc,
IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
crashk_res.start, crashk_res.end,
kbuf, func);
else
- return walk_system_ram_res(0, ULONG_MAX, kbuf, func);
+ ret = walk_system_ram_res(0, ULONG_MAX, kbuf, func);
+
+ if (!ret && kbuf->mem)
+ ret = 1; /* found for top-down search */
+ return ret;
}

/**
--
2.11.1
Thiago Jung Bauermann
2017-04-27 22:00:04 UTC
Permalink
Hello,
Post by AKASHI Takahiro
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
kexec_locate_mem_hole expects arch_kexec_walk_mem to walk memory from top to
bottom if top_down is true. That is what powerpc's version does.

Isn't it possible to walk resources from top to bottom?
Post by AKASHI Takahiro
This behavior is not consistent with locate_hole(hole_end == -1) function
of kexec-tools.
This patch fixes the bug, going though all the memory regions anyway.
This patch would break powerpc, because at the end of the memory walk kbuf
would have the lowest memory hole.

If it's not possible to walk resources in reverse order, then this patch needs
to change powerpc to always walk memory from bottom to top.
--
Thiago Jung Bauermann
IBM Linux Technology Center
AKASHI Takahiro
2017-04-28 00:51:39 UTC
Permalink
Thiago,

Thank you for the comment.
Post by Thiago Jung Bauermann
Hello,
Post by AKASHI Takahiro
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
kexec_locate_mem_hole expects arch_kexec_walk_mem to walk memory from top to
bottom if top_down is true. That is what powerpc's version does.
Ah, I haven't noticed that, but x86 doesn't have arch_kexec_walk_mem and
how can it work for x86?
Post by Thiago Jung Bauermann
Isn't it possible to walk resources from top to bottom?
Yes, it will be, but it seems to me that such a behavior is not intuitive
and even confusing if it doesn't come with explicit explanation.
Post by Thiago Jung Bauermann
Post by AKASHI Takahiro
This behavior is not consistent with locate_hole(hole_end == -1) function
of kexec-tools.
This patch fixes the bug, going though all the memory regions anyway.
This patch would break powerpc, because at the end of the memory walk kbuf
would have the lowest memory hole.
If it's not possible to walk resources in reverse order, then this patch needs
to change powerpc to always walk memory from bottom to top.
So I would like to hear from x86 guys.

Thanks
-Takahiro AKASHI
Post by Thiago Jung Bauermann
--
Thiago Jung Bauermann
IBM Linux Technology Center
Dave Young
2017-04-28 05:19:50 UTC
Permalink
Vivek, can you help to give some comments about the locate hole isssue
in kexec_file?
Post by AKASHI Takahiro
Thiago,
Thank you for the comment.
Post by Thiago Jung Bauermann
Hello,
Post by AKASHI Takahiro
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
kexec_locate_mem_hole expects arch_kexec_walk_mem to walk memory from top to
bottom if top_down is true. That is what powerpc's version does.
Ah, I haven't noticed that, but x86 doesn't have arch_kexec_walk_mem and
how can it work for x86?
Post by Thiago Jung Bauermann
Isn't it possible to walk resources from top to bottom?
Yes, it will be, but it seems to me that such a behavior is not intuitive
and even confusing if it doesn't come with explicit explanation.
Thing need to make clear is why do we need the change, it might be a
problem for crashkernel=xM,low since that is for softiotlb in case
crashkernel=xM,high being used, otherwise seems current code is fine.

Need seeking for old memory from Vivek to confirm.
Post by AKASHI Takahiro
Post by Thiago Jung Bauermann
Post by AKASHI Takahiro
This behavior is not consistent with locate_hole(hole_end == -1) function
of kexec-tools.
This patch fixes the bug, going though all the memory regions anyway.
This patch would break powerpc, because at the end of the memory walk kbuf
would have the lowest memory hole.
If it's not possible to walk resources in reverse order, then this patch needs
to change powerpc to always walk memory from bottom to top.
So I would like to hear from x86 guys.
Thanks
-Takahiro AKASHI
Post by Thiago Jung Bauermann
--
Thiago Jung Bauermann
IBM Linux Technology Center
Dave Young
2017-04-28 05:23:26 UTC
Permalink
Correct Vivek's email address
Post by Dave Young
Vivek, can you help to give some comments about the locate hole isssue
in kexec_file?
Post by AKASHI Takahiro
Thiago,
Thank you for the comment.
Post by Thiago Jung Bauermann
Hello,
Post by AKASHI Takahiro
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
kexec_locate_mem_hole expects arch_kexec_walk_mem to walk memory from top to
bottom if top_down is true. That is what powerpc's version does.
Ah, I haven't noticed that, but x86 doesn't have arch_kexec_walk_mem and
how can it work for x86?
Post by Thiago Jung Bauermann
Isn't it possible to walk resources from top to bottom?
Yes, it will be, but it seems to me that such a behavior is not intuitive
and even confusing if it doesn't come with explicit explanation.
Thing need to make clear is why do we need the change, it might be a
problem for crashkernel=xM,low since that is for softiotlb in case
crashkernel=xM,high being used, otherwise seems current code is fine.
Need seeking for old memory from Vivek to confirm.
Post by AKASHI Takahiro
Post by Thiago Jung Bauermann
Post by AKASHI Takahiro
This behavior is not consistent with locate_hole(hole_end == -1) function
of kexec-tools.
This patch fixes the bug, going though all the memory regions anyway.
This patch would break powerpc, because at the end of the memory walk kbuf
would have the lowest memory hole.
If it's not possible to walk resources in reverse order, then this patch needs
to change powerpc to always walk memory from bottom to top.
So I would like to hear from x86 guys.
Thanks
-Takahiro AKASHI
Post by Thiago Jung Bauermann
--
Thiago Jung Bauermann
IBM Linux Technology Center
Thiago Jung Bauermann
2017-04-28 19:33:34 UTC
Permalink
Post by AKASHI Takahiro
Post by Thiago Jung Bauermann
Hello,
Post by AKASHI Takahiro
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
kexec_locate_mem_hole expects arch_kexec_walk_mem to walk memory from top
to bottom if top_down is true. That is what powerpc's version does.
Ah, I haven't noticed that, but x86 doesn't have arch_kexec_walk_mem and
how can it work for x86?
Looking at v4.9's kexec_add_buffer, the logic has been this way before I
factored kexec_locate_mem_hole out of it. So x86 has been behaving this way
for a while.
Post by AKASHI Takahiro
Post by Thiago Jung Bauermann
Isn't it possible to walk resources from top to bottom?
Yes, it will be, but it seems to me that such a behavior is not intuitive
and even confusing if it doesn't come with explicit explanation.
Yes, I should have put a comment pointing out that assumption.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Dave Young
2017-04-28 04:46:19 UTC
Permalink
Hi AKASHI
Post by AKASHI Takahiro
The current kexec_locate_mem_hole(kbuf.top_down == 1) stops searching at
the first memory region that has enough space for requested size even if
some of higher regions may also have.
This behavior is not consistent with locate_hole(hole_end == -1) function
of kexec-tools.
Have you seen actual bug happened or just observing this during code
review?

Till now seems we do not see any reports about this.
Post by AKASHI Takahiro
This patch fixes the bug, going though all the memory regions anyway.
---
kernel/kexec_file.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b118735fea9d..2f131c0d9017 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -373,8 +373,8 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
/* If we are here, we found a suitable memory range */
kbuf->mem = temp_start;
- /* Success, stop navigating through remaining System RAM ranges */
- return 1;
+ /* always return zero, going through all the System RAM ranges */
+ return 0;
}
static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
@@ -439,18 +439,27 @@ static int locate_mem_hole_callback(u64 start, u64 end, void *arg)
*
* Return: The memory walk will stop when func returns a non-zero value
* and that value will be returned. If all free regions are visited without
- * func returning non-zero, then zero will be returned.
+ * func returning non-zero, then kbuf->mem will be additionally checked
+ * for top-down search.
+ * After all, zero will be returned if none of regions fits.
*/
int __weak arch_kexec_walk_mem(struct kexec_buf *kbuf,
int (*func)(u64, u64, void *))
{
+ int ret;
+
+ kbuf->mem = 0;
if (kbuf->image->type == KEXEC_TYPE_CRASH)
- return walk_iomem_res_desc(crashk_res.desc,
+ ret = walk_iomem_res_desc(crashk_res.desc,
IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
crashk_res.start, crashk_res.end,
kbuf, func);
else
- return walk_system_ram_res(0, ULONG_MAX, kbuf, func);
+ ret = walk_system_ram_res(0, ULONG_MAX, kbuf, func);
+
+ if (!ret && kbuf->mem)
+ ret = 1; /* found for top-down search */
+ return ret;
}
/**
--
2.11.1
Loading...