* Make __memblock_free_early a wrapper of memblock_free rather dup it @ 2018-11-22 4:01 Wang, Matt 2018-11-22 5:27 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Wang, Matt @ 2018-11-22 4:01 UTC (permalink / raw) To: akpm; +Cc: linux-mm [-- Attachment #1.1: Type: text/plain, Size: 1452 bytes --] Hi Andrew, I noticed that __memblock_free_early and memblock_free has the same code. At first I think we can delete __memblock_free_early till __memblock_free_late remind me __memblock_free_early is meaningful. It's a note to call this before struct page was initialized. So I choose to make __memblock_free_early a wrapper of memblock_free. Here is the patch (see attachment file): >From 5f21fb0409e91b42373832627e44cd0a8275c820 Mon Sep 17 00:00:00 2001 From: Wentao Wang <witallwang@gmail.com> Date: Thu, 22 Nov 2018 11:35:59 +0800 Subject: [PATCH] Make __memblock_free_early a wrapper of memblock_free rather than dup it Signed-off-by: Wentao Wang <witallwang@gmail.com> --- mm/memblock.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/mm/memblock.c b/mm/memblock.c index 9a2d5ae..08bf136 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1546,12 +1546,7 @@ void * __init memblock_alloc_try_nid( */ void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) { - phys_addr_t end = base + size - 1; - - memblock_dbg("%s: [%pa-%pa] %pF\n", - __func__, &base, &end, (void *)_RET_IP_); - kmemleak_free_part_phys(base, size); - memblock_remove_range(&memblock.reserved, base, size); + memblock_free(base, size); } /** -- 1.8.3.1 Testing: Build with memblock, system bootup normally and works well. Regards, Wentao [-- Attachment #1.2: Type: text/html, Size: 5178 bytes --] [-- Attachment #2: 0001-Make-__memblock_free_early-a-wrapper-of-memblock_fre.patch --] [-- Type: application/octet-stream, Size: 900 bytes --] From 5f21fb0409e91b42373832627e44cd0a8275c820 Mon Sep 17 00:00:00 2001 From: Wentao Wang <witallwang@gmail.com> Date: Thu, 22 Nov 2018 11:35:59 +0800 Subject: [PATCH] Make __memblock_free_early a wrapper of memblock_free rather than dup it Signed-off-by: Wentao Wang <witallwang@gmail.com> --- mm/memblock.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/mm/memblock.c b/mm/memblock.c index 9a2d5ae..08bf136 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1546,12 +1546,7 @@ void * __init memblock_alloc_try_nid( */ void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) { - phys_addr_t end = base + size - 1; - - memblock_dbg("%s: [%pa-%pa] %pF\n", - __func__, &base, &end, (void *)_RET_IP_); - kmemleak_free_part_phys(base, size); - memblock_remove_range(&memblock.reserved, base, size); + memblock_free(base, size); } /** -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Make __memblock_free_early a wrapper of memblock_free rather dup it 2018-11-22 4:01 Make __memblock_free_early a wrapper of memblock_free rather dup it Wang, Matt @ 2018-11-22 5:27 ` Andrew Morton 2018-11-25 10:29 ` Mike Rapoport 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2018-11-22 5:27 UTC (permalink / raw) To: Wang, Matt; +Cc: linux-mm On Thu, 22 Nov 2018 04:01:53 +0000 "Wang, Matt" <Matt.Wang@Dell.com> wrote: > Subject: [PATCH] Make __memblock_free_early a wrapper of memblock_free rather > than dup it > > Signed-off-by: Wentao Wang <witallwang@gmail.com> > --- > mm/memblock.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/mm/memblock.c b/mm/memblock.c > index 9a2d5ae..08bf136 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -1546,12 +1546,7 @@ void * __init memblock_alloc_try_nid( > */ > void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) > { > - phys_addr_t end = base + size - 1; > - > - memblock_dbg("%s: [%pa-%pa] %pF\n", > - __func__, &base, &end, (void *)_RET_IP_); > - kmemleak_free_part_phys(base, size); > - memblock_remove_range(&memblock.reserved, base, size); > + memblock_free(base, size); > } hm, I suppose so. The debug messaging becomes less informative but the duplication is indeed irritating and if we really want to show the different caller info in the messages, we could do it in a smarter fashion. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Make __memblock_free_early a wrapper of memblock_free rather dup it 2018-11-22 5:27 ` Andrew Morton @ 2018-11-25 10:29 ` Mike Rapoport 2018-11-26 2:25 ` Wang, Matt 0 siblings, 1 reply; 5+ messages in thread From: Mike Rapoport @ 2018-11-25 10:29 UTC (permalink / raw) To: Andrew Morton; +Cc: Wang, Matt, linux-mm On Wed, Nov 21, 2018 at 09:27:40PM -0800, Andrew Morton wrote: > On Thu, 22 Nov 2018 04:01:53 +0000 "Wang, Matt" <Matt.Wang@Dell.com> wrote: > > > Subject: [PATCH] Make __memblock_free_early a wrapper of memblock_free rather > > than dup it > > > > Signed-off-by: Wentao Wang <witallwang@gmail.com> > > --- > > mm/memblock.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > index 9a2d5ae..08bf136 100644 > > --- a/mm/memblock.c > > +++ b/mm/memblock.c > > @@ -1546,12 +1546,7 @@ void * __init memblock_alloc_try_nid( > > */ > > void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) > > { > > - phys_addr_t end = base + size - 1; > > - > > - memblock_dbg("%s: [%pa-%pa] %pF\n", > > - __func__, &base, &end, (void *)_RET_IP_); > > - kmemleak_free_part_phys(base, size); > > - memblock_remove_range(&memblock.reserved, base, size); > > + memblock_free(base, size); > > } > > hm, I suppose so. The debug messaging becomes less informative but the > duplication is indeed irritating and if we really want to show the > different caller info in the messages, we could do it in a smarter > fashion. Sorry for jumping late, but I believe the better way would be simply replace the only two calls to __memblock_free_early() with calls to memblock_free(). The patch below is based on the current mmots. >From 4de5a2aabb0b898c6b4add6bf91175fc55725362 Mon Sep 17 00:00:00 2001 From: Mike Rapoport <rppt@linux.ibm.com> Date: Sun, 25 Nov 2018 12:20:46 +0200 Subject: [PATCH] memblock: replace usage of __memblock_free_early() with memblock_free() The __memblock_free_early() function is only used by the convinince wrappers, so essentially we wrap a call to memblock_free() twice. Replace calls of __memblock_free_early() with calls to memblock_free() and drop the former. Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> --- include/linux/memblock.h | 5 ++--- mm/memblock.c | 22 ++++++++-------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/include/linux/memblock.h b/include/linux/memblock.h index 5ba52a7..e9e4017 100644 --- a/include/linux/memblock.h +++ b/include/linux/memblock.h @@ -154,7 +154,6 @@ void __next_mem_range_rev(u64 *idx, int nid, enum memblock_flags flags, void __next_reserved_mem_region(u64 *idx, phys_addr_t *out_start, phys_addr_t *out_end); -void __memblock_free_early(phys_addr_t base, phys_addr_t size); void __memblock_free_late(phys_addr_t base, phys_addr_t size); /** @@ -452,13 +451,13 @@ static inline void * __init memblock_alloc_node_nopanic(phys_addr_t size, static inline void __init memblock_free_early(phys_addr_t base, phys_addr_t size) { - __memblock_free_early(base, size); + memblock_free(base, size); } static inline void __init memblock_free_early_nid(phys_addr_t base, phys_addr_t size, int nid) { - __memblock_free_early(base, size); + memblock_free(base, size); } static inline void __init memblock_free_late(phys_addr_t base, phys_addr_t size) diff --git a/mm/memblock.c b/mm/memblock.c index 0559979..b842ce1 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -800,7 +800,14 @@ int __init_memblock memblock_remove(phys_addr_t base, phys_addr_t size) return memblock_remove_range(&memblock.memory, base, size); } - +/** + * memblock_free - free boot memory block + * @base: phys starting address of the boot memory block + * @size: size of the boot memory block in bytes + * + * Free boot memory block previously allocated by memblock_alloc_xx() API. + * The freeing memory will not be released to the buddy allocator. + */ int __init_memblock memblock_free(phys_addr_t base, phys_addr_t size) { phys_addr_t end = base + size - 1; @@ -1600,19 +1607,6 @@ void * __init memblock_alloc_try_nid( } /** - * __memblock_free_early - free boot memory block - * @base: phys starting address of the boot memory block - * @size: size of the boot memory block in bytes - * - * Free boot memory block previously allocated by memblock_alloc_xx() API. - * The freeing memory will not be released to the buddy allocator. - */ -void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) -{ - memblock_free(base, size); -} - -/** * __memblock_free_late - free bootmem block pages directly to buddy allocator * @base: phys starting address of the boot memory block * @size: size of the boot memory block in bytes -- 2.7.4 -- Sincerely yours, Mike. ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: Make __memblock_free_early a wrapper of memblock_free rather dup it 2018-11-25 10:29 ` Mike Rapoport @ 2018-11-26 2:25 ` Wang, Matt 2018-11-26 7:13 ` Mike Rapoport 0 siblings, 1 reply; 5+ messages in thread From: Wang, Matt @ 2018-11-26 2:25 UTC (permalink / raw) To: Mike Rapoport, Andrew Morton; +Cc: linux-mm I believe I explained why we choose it to be a wrapper, " I noticed that __memblock_free_early and memblock_free has the same code. At first I think we can delete __memblock_free_early till __memblock_free_late remind me __memblock_free_early is meaningful. It’s a note to call this before struct page was initialized." Andrew may choose plan as he see fit. Regards, Matt -----Original Message----- From: Mike Rapoport [mailto:rppt@linux.ibm.com] Sent: 2018年11月25日 18:30 To: Andrew Morton Cc: Wang, Matt; linux-mm@kvack.org Subject: Re: Make __memblock_free_early a wrapper of memblock_free rather dup it [EXTERNAL EMAIL] On Wed, Nov 21, 2018 at 09:27:40PM -0800, Andrew Morton wrote: > On Thu, 22 Nov 2018 04:01:53 +0000 "Wang, Matt" <Matt.Wang@Dell.com> wrote: > > > Subject: [PATCH] Make __memblock_free_early a wrapper of > > memblock_free rather than dup it > > > > Signed-off-by: Wentao Wang <witallwang@gmail.com> > > --- > > mm/memblock.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/mm/memblock.c b/mm/memblock.c index 9a2d5ae..08bf136 > > 100644 > > --- a/mm/memblock.c > > +++ b/mm/memblock.c > > @@ -1546,12 +1546,7 @@ void * __init memblock_alloc_try_nid( > > */ > > void __init __memblock_free_early(phys_addr_t base, phys_addr_t > > size) { > > - phys_addr_t end = base + size - 1; > > - > > - memblock_dbg("%s: [%pa-%pa] %pF\n", > > - __func__, &base, &end, (void *)_RET_IP_); > > - kmemleak_free_part_phys(base, size); > > - memblock_remove_range(&memblock.reserved, base, size); > > + memblock_free(base, size); > > } > > hm, I suppose so. The debug messaging becomes less informative but > the duplication is indeed irritating and if we really want to show the > different caller info in the messages, we could do it in a smarter > fashion. Sorry for jumping late, but I believe the better way would be simply replace the only two calls to __memblock_free_early() with calls to memblock_free(). The patch below is based on the current mmots. >From 4de5a2aabb0b898c6b4add6bf91175fc55725362 Mon Sep 17 00:00:00 2001 From: Mike Rapoport <rppt@linux.ibm.com> Date: Sun, 25 Nov 2018 12:20:46 +0200 Subject: [PATCH] memblock: replace usage of __memblock_free_early() with memblock_free() The __memblock_free_early() function is only used by the convinince wrappers, so essentially we wrap a call to memblock_free() twice. Replace calls of __memblock_free_early() with calls to memblock_free() and drop the former. Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> --- include/linux/memblock.h | 5 ++--- mm/memblock.c | 22 ++++++++-------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/include/linux/memblock.h b/include/linux/memblock.h index 5ba52a7..e9e4017 100644 --- a/include/linux/memblock.h +++ b/include/linux/memblock.h @@ -154,7 +154,6 @@ void __next_mem_range_rev(u64 *idx, int nid, enum memblock_flags flags, void __next_reserved_mem_region(u64 *idx, phys_addr_t *out_start, phys_addr_t *out_end); -void __memblock_free_early(phys_addr_t base, phys_addr_t size); void __memblock_free_late(phys_addr_t base, phys_addr_t size); /** @@ -452,13 +451,13 @@ static inline void * __init memblock_alloc_node_nopanic(phys_addr_t size, static inline void __init memblock_free_early(phys_addr_t base, phys_addr_t size) { - __memblock_free_early(base, size); + memblock_free(base, size); } static inline void __init memblock_free_early_nid(phys_addr_t base, phys_addr_t size, int nid) { - __memblock_free_early(base, size); + memblock_free(base, size); } static inline void __init memblock_free_late(phys_addr_t base, phys_addr_t size) diff --git a/mm/memblock.c b/mm/memblock.c index 0559979..b842ce1 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -800,7 +800,14 @@ int __init_memblock memblock_remove(phys_addr_t base, phys_addr_t size) return memblock_remove_range(&memblock.memory, base, size); } - +/** + * memblock_free - free boot memory block + * @base: phys starting address of the boot memory block + * @size: size of the boot memory block in bytes + * + * Free boot memory block previously allocated by memblock_alloc_xx() API. + * The freeing memory will not be released to the buddy allocator. + */ int __init_memblock memblock_free(phys_addr_t base, phys_addr_t size) { phys_addr_t end = base + size - 1; @@ -1600,19 +1607,6 @@ void * __init memblock_alloc_try_nid( } /** - * __memblock_free_early - free boot memory block - * @base: phys starting address of the boot memory block - * @size: size of the boot memory block in bytes - * - * Free boot memory block previously allocated by memblock_alloc_xx() API. - * The freeing memory will not be released to the buddy allocator. - */ -void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) -{ - memblock_free(base, size); -} - -/** * __memblock_free_late - free bootmem block pages directly to buddy allocator * @base: phys starting address of the boot memory block * @size: size of the boot memory block in bytes -- 2.7.4 -- Sincerely yours, Mike. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Make __memblock_free_early a wrapper of memblock_free rather dup it 2018-11-26 2:25 ` Wang, Matt @ 2018-11-26 7:13 ` Mike Rapoport 0 siblings, 0 replies; 5+ messages in thread From: Mike Rapoport @ 2018-11-26 7:13 UTC (permalink / raw) To: Wang, Matt; +Cc: Andrew Morton, linux-mm Hi, On Mon, Nov 26, 2018 at 02:25:44AM +0000, Wang, Matt wrote: > I believe I explained why we choose it to be a wrapper, > " I noticed that __memblock_free_early and memblock_free has the same > code. At first I think we can delete __memblock_free_early till > __memblock_free_late remind me __memblock_free_early is meaningful. It’s > a note to call this before struct page was initialized." I've been offline when you've sent your patch, otherwise I would have commented then. First of all, thanks for spotting that memblock_free() and __memblock_free_early() are identical :) Regarding the choice which one should be removed, the __memblock_free_early() is never called directly by the memblock users but rather via memblock_free_early() wrapper. I believe it would be cleaner to make that wrapper call memblock_free() directly without the additional nesting. > Andrew may choose plan as he see fit. > > Regards, > Matt -- Sincerely yours, Mike. > -----Original Message----- > From: Mike Rapoport [mailto:rppt@linux.ibm.com] > Sent: 2018年11月25日 18:30 > To: Andrew Morton > Cc: Wang, Matt; linux-mm@kvack.org > Subject: Re: Make __memblock_free_early a wrapper of memblock_free rather dup it > > > [EXTERNAL EMAIL] > > On Wed, Nov 21, 2018 at 09:27:40PM -0800, Andrew Morton wrote: > > On Thu, 22 Nov 2018 04:01:53 +0000 "Wang, Matt" <Matt.Wang@Dell.com> wrote: > > > > > Subject: [PATCH] Make __memblock_free_early a wrapper of > > > memblock_free rather than dup it > > > > > > Signed-off-by: Wentao Wang <witallwang@gmail.com> > > > --- > > > mm/memblock.c | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/mm/memblock.c b/mm/memblock.c index 9a2d5ae..08bf136 > > > 100644 > > > --- a/mm/memblock.c > > > +++ b/mm/memblock.c > > > @@ -1546,12 +1546,7 @@ void * __init memblock_alloc_try_nid( > > > */ > > > void __init __memblock_free_early(phys_addr_t base, phys_addr_t > > > size) { > > > - phys_addr_t end = base + size - 1; > > > - > > > - memblock_dbg("%s: [%pa-%pa] %pF\n", > > > - __func__, &base, &end, (void *)_RET_IP_); > > > - kmemleak_free_part_phys(base, size); > > > - memblock_remove_range(&memblock.reserved, base, size); > > > + memblock_free(base, size); > > > } > > > > hm, I suppose so. The debug messaging becomes less informative but > > the duplication is indeed irritating and if we really want to show the > > different caller info in the messages, we could do it in a smarter > > fashion. > > Sorry for jumping late, but I believe the better way would be simply replace the only two calls to __memblock_free_early() with calls to memblock_free(). > > The patch below is based on the current mmots. > > From 4de5a2aabb0b898c6b4add6bf91175fc55725362 Mon Sep 17 00:00:00 2001 > From: Mike Rapoport <rppt@linux.ibm.com> > Date: Sun, 25 Nov 2018 12:20:46 +0200 > Subject: [PATCH] memblock: replace usage of __memblock_free_early() with > memblock_free() > > The __memblock_free_early() function is only used by the convinince wrappers, so essentially we wrap a call to memblock_free() twice. > Replace calls of __memblock_free_early() with calls to memblock_free() and drop the former. > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com> > --- > include/linux/memblock.h | 5 ++--- > mm/memblock.c | 22 ++++++++-------------- > 2 files changed, 10 insertions(+), 17 deletions(-) > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h index 5ba52a7..e9e4017 100644 > --- a/include/linux/memblock.h > +++ b/include/linux/memblock.h > @@ -154,7 +154,6 @@ void __next_mem_range_rev(u64 *idx, int nid, enum memblock_flags flags, void __next_reserved_mem_region(u64 *idx, phys_addr_t *out_start, > phys_addr_t *out_end); > > -void __memblock_free_early(phys_addr_t base, phys_addr_t size); void __memblock_free_late(phys_addr_t base, phys_addr_t size); > > /** > @@ -452,13 +451,13 @@ static inline void * __init memblock_alloc_node_nopanic(phys_addr_t size, static inline void __init memblock_free_early(phys_addr_t base, > phys_addr_t size) > { > - __memblock_free_early(base, size); > + memblock_free(base, size); > } > > static inline void __init memblock_free_early_nid(phys_addr_t base, > phys_addr_t size, int nid) > { > - __memblock_free_early(base, size); > + memblock_free(base, size); > } > > static inline void __init memblock_free_late(phys_addr_t base, phys_addr_t size) diff --git a/mm/memblock.c b/mm/memblock.c index 0559979..b842ce1 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -800,7 +800,14 @@ int __init_memblock memblock_remove(phys_addr_t base, phys_addr_t size) > return memblock_remove_range(&memblock.memory, base, size); } > > - > +/** > + * memblock_free - free boot memory block > + * @base: phys starting address of the boot memory block > + * @size: size of the boot memory block in bytes > + * > + * Free boot memory block previously allocated by memblock_alloc_xx() API. > + * The freeing memory will not be released to the buddy allocator. > + */ > int __init_memblock memblock_free(phys_addr_t base, phys_addr_t size) { > phys_addr_t end = base + size - 1; > @@ -1600,19 +1607,6 @@ void * __init memblock_alloc_try_nid( } > > /** > - * __memblock_free_early - free boot memory block > - * @base: phys starting address of the boot memory block > - * @size: size of the boot memory block in bytes > - * > - * Free boot memory block previously allocated by memblock_alloc_xx() API. > - * The freeing memory will not be released to the buddy allocator. > - */ > -void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) -{ > - memblock_free(base, size); > -} > - > -/** > * __memblock_free_late - free bootmem block pages directly to buddy allocator > * @base: phys starting address of the boot memory block > * @size: size of the boot memory block in bytes > -- > 2.7.4 > > > -- > Sincerely yours, > Mike. > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-26 7:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-22 4:01 Make __memblock_free_early a wrapper of memblock_free rather dup it Wang, Matt 2018-11-22 5:27 ` Andrew Morton 2018-11-25 10:29 ` Mike Rapoport 2018-11-26 2:25 ` Wang, Matt 2018-11-26 7:13 ` Mike Rapoport
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.