This fixes an unused variable warning in nobootmem.c
Signed-off-by: Philipp Hachtmann <[email protected]>
---
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
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
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 <[email protected]> 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 <[email protected]> wrote:
>>
>> This fixes an unused variable warning in nobootmem.c
>>
>> Signed-off-by: Philipp Hachtmann <[email protected]>
>> ---
>> 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
>>
>
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 <[email protected]>
On Thu, Jan 16, 2014 at 9:49 AM, Philipp Hachtmann
<[email protected]> 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
>
> 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.
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
<[email protected]> 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.
>
>
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".
On Thu, 16 Jan 2014 14:33:06 +0100 Philipp Hachtmann <[email protected]> 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;
}
Am Fri, 17 Jan 2014 13:38:31 -0800
schrieb Andrew Morton <[email protected]>:
> 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