[PATCH] mm/nobootmem: Fix unused variable

[PATCH] mm/nobootmem: Fix unused variable

[Philipp Hachtmann]

This fixes an unused variable warning in nobootmem.c

Signed-off-by: Philipp Hachtmann <phacht@linux.vnet.ibm.com>
---
 mm/nobootmem.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index e2906a5..12cbb04 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -116,9 +116,13 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
 static unsigned long __init free_low_memory_core_early(void)
 {
 	unsigned long count = 0;
-	phys_addr_t start, end, size;
+	phys_addr_t start, end;
 	u64 i;
 
+#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
+	phys_addr_t size;
+#endif
+
 	for_each_free_mem_range(i, NUMA_NO_NODE, &start, &end, NULL)
 		count += __free_memory_core(start, end);
 
-- 
1.8.4.5

Re: [PATCH] mm/nobootmem: Fix unused variable

[Philipp Hachtmann]

Hi Robin,

>  Maybe you are working off a different repo than
> Linus' latest?  Your line 116 is my 114.  Maybe the message needs to
> be a bit more descriptive 

Ah, yes. This fits Andrew's linux-next. 

Regards

Philipp

Re: [PATCH] mm/nobootmem: Fix unused variable

[Robin Holt]

Argh.  Thought I had changed that to plain text mode before sending.

Sorry for the noise,
Robin


On Thu, Jan 16, 2014 at 9:45 AM, Robin Holt <robinmholt@gmail.com> wrote:
>
> I can not see how this works.  How is the return from
> get_allocated_memblock_reserved_regions_info() stored and used without being
> declared?  Maybe you are working off a different repo than Linus' latest?  Your line
> 116 is my 114.  Maybe the message needs to be a bit more descriptive and
> certainly the bit after the '---' should be telling me what this is applying against.
>
> Robin
>
>
> On Thu, Jan 16, 2014 at 7:33 AM, Philipp Hachtmann <phacht@linux.vnet.ibm.com> wrote:
>>
>> This fixes an unused variable warning in nobootmem.c
>>
>> Signed-off-by: Philipp Hachtmann <phacht@linux.vnet.ibm.com>
>> ---
>>  mm/nobootmem.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
>> index e2906a5..12cbb04 100644
>> --- a/mm/nobootmem.c
>> +++ b/mm/nobootmem.c
>> @@ -116,9 +116,13 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
>>  static unsigned long __init free_low_memory_core_early(void)
>>  {
>>         unsigned long count = 0;
>> -       phys_addr_t start, end, size;
>> +       phys_addr_t start, end;
>>         u64 i;
>>
>> +#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
>> +       phys_addr_t size;
>> +#endif
>> +
>>         for_each_free_mem_range(i, NUMA_NO_NODE, &start, &end, NULL)
>>                 count += __free_memory_core(start, end);
>>
>> --
>> 1.8.4.5
>>
>

Re: [PATCH] mm/nobootmem: Fix unused variable

[Robin Holt]

Since your patch set is the _ONLY_ thing that introduces #ifdef's
inside functions within
this file, I would think you would be better off making
get_allocated_memblock_reserved_regions_info() and
get_allocated_memblock_memory_regions_info be static inline functions
when #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK.

That said, I don't have a fundamental objection to #ifdef's inside
functions so...

Acked-by: Robin Holt <robinmholt@gmail.com>

On Thu, Jan 16, 2014 at 9:49 AM, Philipp Hachtmann
<phacht@linux.vnet.ibm.com> wrote:
> Hi Robin,
>
>>  Maybe you are working off a different repo than
>> Linus' latest?  Your line 116 is my 114.  Maybe the message needs to
>> be a bit more descriptive
>
> Ah, yes. This fits Andrew's linux-next.
>
> Regards
>
> Philipp
>

Re: [PATCH] mm/nobootmem: Fix unused variable

[Philipp Hachtmann]

> I would think you would be better off making
> get_allocated_memblock_reserved_regions_info() and
> get_allocated_memblock_memory_regions_info be static inline functions
> when #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK.
Possible, of course.
But the size variable has still to be #ifdef'd. And that's what the
patch is about. It's just an addition to another patch. 

Re: [PATCH] mm/nobootmem: Fix unused variable

[Robin Holt]

If the definition of the
get_allocated_memblock_reserved_regions_info() function when
CONFIG_ARCH_DISCARD_MEMBLOCK simply returns 0, the compiler will see
that size is defined, the optimizer will see that it is always 0 and
that the if(0) is always false.  The net result will be no code will
be produced and the function will be less cluttered.

On Thu, Jan 16, 2014 at 11:41 AM, Philipp Hachtmann
<phacht@linux.vnet.ibm.com> wrote:
>
>> I would think you would be better off making
>> get_allocated_memblock_reserved_regions_info() and
>> get_allocated_memblock_memory_regions_info be static inline functions
>> when #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK.
> Possible, of course.
> But the size variable has still to be #ifdef'd. And that's what the
> patch is about. It's just an addition to another patch.
>
>

Re: [PATCH] mm/nobootmem: Fix unused variable

[David Rientjes]

On Thu, 16 Jan 2014, Philipp Hachtmann wrote:

> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index e2906a5..12cbb04 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -116,9 +116,13 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
>  static unsigned long __init free_low_memory_core_early(void)
>  {
>  	unsigned long count = 0;
> -	phys_addr_t start, end, size;
> +	phys_addr_t start, end;
>  	u64 i;
>  
> +#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> +	phys_addr_t size;
> +#endif
> +
>  	for_each_free_mem_range(i, NUMA_NO_NODE, &start, &end, NULL)
>  		count += __free_memory_core(start, end);
>  

Two options: (1) add a helper function declared for 
CONFIG_ARCH_DISCARD_MEMBLOCK that returns the count to add and is empty 
otherwise, or (2) initialize size to zero in its definition.  It's much 
better than #ifdef's inside the function for a variable declaration.

Also, since this is already in -mm, you'll want this fix folded into the 
original patch, "mm/nobootmem: free_all_bootmem again", so it's probably 
best to name it "mm/nobootmem: free_all_bootmem again fix".

Re: [PATCH] mm/nobootmem: Fix unused variable

[Andrew Morton]

On Thu, 16 Jan 2014 14:33:06 +0100 Philipp Hachtmann <phacht@linux.vnet.ibm.com> wrote:

> This fixes an unused variable warning in nobootmem.c
> 
> ...
>
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -116,9 +116,13 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
>  static unsigned long __init free_low_memory_core_early(void)
>  {
>  	unsigned long count = 0;
> -	phys_addr_t start, end, size;
> +	phys_addr_t start, end;
>  	u64 i;
>  
> +#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> +	phys_addr_t size;
> +#endif
> +
>  	for_each_free_mem_range(i, NUMA_NO_NODE, &start, &end, NULL)
>  		count += __free_memory_core(start, end);

Yes, that is a bit of an eyesore.  We often approach the problem this
way, which is nicer:

static unsigned long __init free_low_memory_core_early(void)
{
	unsigned long count = 0;
	phys_addr_t start, end;
	u64 i;

	for_each_free_mem_range(i, NUMA_NO_NODE, &start, &end, NULL)
		count += __free_memory_core(start, end);

#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
	{
		phys_addr_t size;

		/* Free memblock.reserved array if it was allocated */
		size = get_allocated_memblock_reserved_regions_info(&start);
		if (size)
			count += __free_memory_core(start, start + size);

		/* Free memblock.memory array if it was allocated */
		size = get_allocated_memblock_memory_regions_info(&start);
		if (size)
			count += __free_memory_core(start, start + size);
	}
#endif

	return count;
}

Re: [PATCH] mm/nobootmem: Fix unused variable

[Philipp Hachtmann]

Am Fri, 17 Jan 2014 13:38:31 -0800
schrieb Andrew Morton <akpm@linux-foundation.org>:

> Yes, that is a bit of an eyesore.  We often approach the problem this
> way, which is nicer:

> [ ... ]
> #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> 	{
> 		phys_addr_t size;
> 
> 		[ ... ]
>	}
> #endif

This is a very nice idea! I have updated my fix. This leads to a V5
patch series (which I will post now) because the following to patches
had to be slightly updated
to fit on top of the fix.

Kind regards

Philipp