| Message ID | 20170426082209.12127-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hello, Am Mittwoch, 26. April 2017, 17:22:09 BRT schrieb 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? > 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
Thiago, Thank you for the comment. On Thu, Apr 27, 2017 at 07:00:04PM -0300, Thiago Jung Bauermann wrote: > Hello, > > Am Mittwoch, 26. April 2017, 17:22:09 BRT schrieb 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? > 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. > > 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 > -- > Thiago Jung Bauermann > IBM Linux Technology Center >
Hi AKASHI On 04/26/17 at 05:22pm, AKASHI Takahiro wrote: > 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. > > This patch fixes the bug, going though all the memory regions anyway. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@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 >
Vivek, can you help to give some comments about the locate hole isssue in kexec_file? On 04/28/17 at 09:51am, AKASHI Takahiro wrote: > Thiago, > > Thank you for the comment. > > On Thu, Apr 27, 2017 at 07:00:04PM -0300, Thiago Jung Bauermann wrote: > > Hello, > > > > Am Mittwoch, 26. April 2017, 17:22:09 BRT schrieb 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? > > > 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. > > > > 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 > > > -- > > Thiago Jung Bauermann > > IBM Linux Technology Center > >
Correct Vivek's email address On 04/28/17 at 01:19pm, Dave Young wrote: > Vivek, can you help to give some comments about the locate hole isssue > in kexec_file? > > On 04/28/17 at 09:51am, AKASHI Takahiro wrote: > > Thiago, > > > > Thank you for the comment. > > > > On Thu, Apr 27, 2017 at 07:00:04PM -0300, Thiago Jung Bauermann wrote: > > > Hello, > > > > > > Am Mittwoch, 26. April 2017, 17:22:09 BRT schrieb 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? > > > > > 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. > > > > > > 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 > > > > > -- > > > Thiago Jung Bauermann > > > IBM Linux Technology Center > > >
Am Freitag, 28. April 2017, 09:51:39 BRT schrieb AKASHI Takahiro: > On Thu, Apr 27, 2017 at 07:00:04PM -0300, Thiago Jung Bauermann wrote: > > Hello, > > > > Am Mittwoch, 26. April 2017, 17:22:09 BRT schrieb 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. > > 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
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; } /**
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 <takahiro.akashi@linaro.org> --- kernel/kexec_file.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) -- 2.11.1