Re: [PATCH] mm/nobootmem: Fix unused variable
Am Fri, 17 Jan 2014 13:38:31 -0800
schrieb Andrew Morton :
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
Am Fri, 17 Jan 2014 13:38:31 -0800
schrieb Andrew Morton a...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
On Thu, 16 Jan 2014 14:33:06 +0100 Philipp Hachtmann
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, , , 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, , , 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();
if (size)
count += __free_memory_core(start, start + size);
/* Free memblock.memory array if it was allocated */
size = get_allocated_memblock_memory_regions_info();
if (size)
count += __free_memory_core(start, start + size);
}
#endif
return count;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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, , , 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".
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 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. > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 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
> wrote:
>>
>> This fixes an unused variable warning in nobootmem.c
>>
>> Signed-off-by: Philipp Hachtmann
>> ---
>> 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, , , 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 On Thu, Jan 16, 2014 at 9:49 AM, Philipp Hachtmann 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mm/nobootmem: Fix unused variable
This fixes an unused variable warning in nobootmem.c
Signed-off-by: Philipp Hachtmann
---
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, , , 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mm/nobootmem: Fix unused variable
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 robinmh...@gmail.com On Thu, Jan 16, 2014 at 9:49 AM, Philipp Hachtmann pha...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 robinmh...@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
pha...@linux.vnet.ibm.com wrote:
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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 pha...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm/nobootmem: Fix unused variable
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
