| Message ID | 20190213063127.28703-7-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixmap support and MM cleanups | expand |
On Wed, Feb 13, 2019 at 06:32:24AM +0000, Anup Patel wrote: > index 9cd583b6d1cd..c22b873de856 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -97,8 +97,9 @@ static void __init setup_initrd(void) > initrd_end = 0; > } > > -void free_initrd_mem(unsigned long start, unsigned long end) > +void __init free_initrd_mem(unsigned long start, unsigned long end) > { > + memblock_free(__pa(start), end - start); I'm pretty sure this should be a call to free_reserved_area instead. All regions reserved using memblock_reserved and not freed before initializing the MM are marked reserved and don't have valid page counts, etc. So we need the actions in free_reserved_area to actually make the memory useful. Now every other architecture except for arm64 seems to do fine without a memblock_free. I'm not an expert on memblock (but I've CCed one), but I guess the reason is that once the kernel has booted we don't really care about freeing memblock area.
Hi, On Tue, Feb 12, 2019 at 10:44:19PM -0800, Christoph Hellwig wrote: > On Wed, Feb 13, 2019 at 06:32:24AM +0000, Anup Patel wrote: > > index 9cd583b6d1cd..c22b873de856 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -97,8 +97,9 @@ static void __init setup_initrd(void) > > initrd_end = 0; > > } > > > > -void free_initrd_mem(unsigned long start, unsigned long end) > > +void __init free_initrd_mem(unsigned long start, unsigned long end) > > { > > + memblock_free(__pa(start), end - start); > > I'm pretty sure this should be a call to free_reserved_area instead. > > All regions reserved using memblock_reserved and not freed before > initializing the MM are marked reserved and don't have valid page > counts, etc. > > So we need the actions in free_reserved_area to actually make the > memory useful. Now every other architecture except for arm64 > seems to do fine without a memblock_free. I'm not an expert on > memblock (but I've CCed one), but I guess the reason is that once > the kernel has booted we don't really care about freeing memblock > area. This late in the boot process there should be a call to free_reserved_area() to give pages to the buddy allocator. memblock_free() is has no real effect at this point, no idea why arm64 calls it.
> -----Original Message----- > From: Mike Rapoport [mailto:rppt@linux.ibm.com] > Sent: Wednesday, February 13, 2019 1:09 PM > To: Christoph Hellwig <hch@infradead.org> > Cc: Anup Patel <Anup.Patel@wdc.com>; Palmer Dabbelt > <palmer@sifive.com>; Albert Ou <aou@eecs.berkeley.edu>; Atish Patra > <Atish.Patra@wdc.com>; Paul Walmsley <paul.walmsley@sifive.com>; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v4 6/6] RISC-V: Free-up initrd in free_initrd_mem() > > Hi, > > On Tue, Feb 12, 2019 at 10:44:19PM -0800, Christoph Hellwig wrote: > > On Wed, Feb 13, 2019 at 06:32:24AM +0000, Anup Patel wrote: > > > index 9cd583b6d1cd..c22b873de856 100644 > > > --- a/arch/riscv/kernel/setup.c > > > +++ b/arch/riscv/kernel/setup.c > > > @@ -97,8 +97,9 @@ static void __init setup_initrd(void) > > > initrd_end = 0; > > > } > > > > > > -void free_initrd_mem(unsigned long start, unsigned long end) > > > +void __init free_initrd_mem(unsigned long start, unsigned long end) > > > { > > > + memblock_free(__pa(start), end - start); > > > > I'm pretty sure this should be a call to free_reserved_area instead. > > > > All regions reserved using memblock_reserved and not freed before > > initializing the MM are marked reserved and don't have valid page > > counts, etc. > > > > So we need the actions in free_reserved_area to actually make the > > memory useful. Now every other architecture except for arm64 seems to > > do fine without a memblock_free. I'm not an expert on memblock (but > > I've CCed one), but I guess the reason is that once the kernel has > > booted we don't really care about freeing memblock area. > > This late in the boot process there should be a call to > free_reserved_area() to give pages to the buddy allocator. > > memblock_free() is has no real effect at this point, no idea why arm64 calls it. Thanks for the info. I will update this patch to use free_reserved_area(). Regards, Anup
On Wed, Feb 13, 2019 at 09:38:36AM +0200, Mike Rapoport wrote: > memblock_free() is has no real effect at this point, no idea why arm64 > calls it. Looks like the call was added fairly recently by: commit 05c58752f9dce11e396676eb731a620541590ed0 Author: CHANDAN VN <chandan.vn@samsung.com> Date: Mon Apr 30 09:50:18 2018 +0530 arm64: To remove initrd reserved area entry from memblock which claims it is to work around the initrd being displayed in /sys/kernel/debug/memblock/reserved. I really think we need to have common behavior there - either do this for all architectures or none. I've just sent a series that consolidates all but a handful of the free_initrd_mem, so implementing any common behavior on top of that would be good.
On Wed, Feb 13, 2019 at 09:54:15AM -0800, Christoph Hellwig wrote: > On Wed, Feb 13, 2019 at 09:38:36AM +0200, Mike Rapoport wrote: > > memblock_free() is has no real effect at this point, no idea why arm64 > > calls it. > > Looks like the call was added fairly recently by: > > commit 05c58752f9dce11e396676eb731a620541590ed0 > Author: CHANDAN VN <chandan.vn@samsung.com> > Date: Mon Apr 30 09:50:18 2018 +0530 > > arm64: To remove initrd reserved area entry from memblock > > which claims it is to work around the initrd being displayed in > /sys/kernel/debug/memblock/reserved. > > I really think we need to have common behavior there - either do this > for all architectures or none. I've just sent a series that > consolidates all but a handful of the free_initrd_mem, so implementing > any common behavior on top of that would be good. I've just started to look into it today :) I'll reply on that thread.
Hi, > On Wed, Feb 13, 2019 at 09:54:15AM -0800, Christoph Hellwig wrote: > > On Wed, Feb 13, 2019 at 09:38:36AM +0200, Mike Rapoport wrote: > > > memblock_free() is has no real effect at this point, no idea why arm64 > > > calls it. > > > > Looks like the call was added fairly recently by: > > > > commit 05c58752f9dce11e396676eb731a620541590ed0 > > Author: CHANDAN VN <chandan.vn@samsung.com> > > Date: Mon Apr 30 09:50:18 2018 +0530 > > > > arm64: To remove initrd reserved area entry from memblock > > > > which claims it is to work around the initrd being displayed in > > /sys/kernel/debug/memblock/reserved. > > > > I really think we need to have common behavior there - either do this > > for all architectures or none. I've just sent a series that > > consolidates all but a handful of the free_initrd_mem, so implementing > > any common behavior on top of that would be good. > > I've just started to look into it today :) > I'll reply on that thread. INITRD reserved area entry is not removed from memblock even though initrd reserved area is freed. The same can be checked from /sys/kernel/debug/memblock/reserved. We did not face this issue on arm32 architecture. Though the changes which i had submitted does not fix any memory leak, it does make sure that the entries freed from memblock are actually removed from the sys entry as well. Also the implementation of arm64 is quite different from arm32. I feel a generic implementation can be taken up only if its a real necessity.
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 9cd583b6d1cd..c22b873de856 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -97,8 +97,9 @@ static void __init setup_initrd(void) initrd_end = 0; } -void free_initrd_mem(unsigned long start, unsigned long end) +void __init free_initrd_mem(unsigned long start, unsigned long end) { + memblock_free(__pa(start), end - start); } #endif /* CONFIG_BLK_DEV_INITRD */
We should free-up initrd memory in free_initrd_mem() instead of doing nothing. Signed-off-by: Anup Patel <anup.patel@wdc.com> --- arch/riscv/kernel/setup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)