diff mbox series

Make __memblock_free_early a wrapper of memblock_free rather dup it

Message ID C8ECE1B7A767434691FEEFA3A01765D72AFB8E78@MX203CL03.corp.emc.com (mailing list archive)
State New, archived
Headers show
Series Make __memblock_free_early a wrapper of memblock_free rather dup it | expand

Commit Message

Wang, Matt Nov. 22, 2018, 4:01 a.m. UTC 
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(-)

--
1.8.3.1

Testing:
Build with memblock, system bootup normally and works well.

Regards,
Wentao

Comments

Andrew Morton Nov. 22, 2018, 5:27 a.m. UTC | #1 
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.
Mike Rapoport Nov. 25, 2018, 10:29 a.m. UTC | #2 
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
Wang, Matt Nov. 26, 2018, 2:25 a.m. UTC | #3 
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
Mike Rapoport Nov. 26, 2018, 7:13 a.m. UTC | #4 
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.
>
diff mbox series

Patch

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);
}

/**