[PATCH] mm/nobootmem: Fix unused variable

[Philipp Hachtmann]

This fixes an unused variable warning in nobootmem.c

Signed-off-by: Philipp Hachtmann <pha...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[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

[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 <robin...@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

[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 <robin...@gmail.com>

[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.

[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.

[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".

[Andrew Morton]

On Thu, 16 Jan 2014 14:33:06 +0100 Philipp Hachtmann <pha...@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;

[Philipp Hachtmann]

Am Fri, 17 Jan 2014 13:38:31 -0800
schrieb Andrew Morton <ak...@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