[v6,15/15] mm: debug for raw alloctor

Commit Message

Pavel Tatashin Aug. 7, 2017, 8:38 p.m. UTC

When CONFIG_DEBUG_VM is enabled, this patch sets all the memory that is
returned by memblock_virt_alloc_try_nid_raw() to ones to ensure that no
places excpect zeroed memory.

Signed-off-by: Pavel Tatashin <[e-mail]>
Reviewed-by: Steven Sistare <[e-mail]>
Reviewed-by: Daniel Jordan <[e-mail]>
Reviewed-by: Bob Picco <[e-mail]>
---
 mm/memblock.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Michal Hocko Aug. 11, 2017, 1:08 p.m. UTC | #1

On Mon 07-08-17 16:38:49, Pavel Tatashin wrote:
> When CONFIG_DEBUG_VM is enabled, this patch sets all the memory that is
> returned by memblock_virt_alloc_try_nid_raw() to ones to ensure that no
> places excpect zeroed memory.

Please fold this into the patch which introduces
memblock_virt_alloc_try_nid_raw. I am not sure CONFIG_DEBUG_VM is the
best config because that tends to be enabled quite often. Maybe
CONFIG_MEMBLOCK_DEBUG? Or even make it kernel command line parameter?

> Signed-off-by: Pavel Tatashin <[e-mail]>
> Reviewed-by: Steven Sistare <[e-mail]>
> Reviewed-by: Daniel Jordan <[e-mail]>
> Reviewed-by: Bob Picco <[e-mail]>
> ---
>  mm/memblock.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 3fbf3bcb52d9..29fcb1dd8a81 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1363,12 +1363,19 @@ void * __init memblock_virt_alloc_try_nid_raw(
>  			phys_addr_t min_addr, phys_addr_t max_addr,
>  			int nid)
>  {
> +	void *ptr;
> +
>  	memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
>  		     __func__, (u64)size, (u64)align, nid, (u64)min_addr,
>  		     (u64)max_addr, (void *)_RET_IP_);
>  
> -	return memblock_virt_alloc_internal(size, align,
> -					    min_addr, max_addr, nid);
> +	ptr = memblock_virt_alloc_internal(size, align,
> +					   min_addr, max_addr, nid);
> +#ifdef CONFIG_DEBUG_VM
> +	if (ptr && size > 0)
> +		memset(ptr, 0xff, size);
> +#endif
> +	return ptr;
>  }
>  
>  /**
> -- 
> 2.14.0

Pavel Tatashin Aug. 11, 2017, 4:18 p.m. UTC | #2

>> When CONFIG_DEBUG_VM is enabled, this patch sets all the memory that is
>> returned by memblock_virt_alloc_try_nid_raw() to ones to ensure that no
>> places excpect zeroed memory.
> 
> Please fold this into the patch which introduces
> memblock_virt_alloc_try_nid_raw.

OK

  I am not sure CONFIG_DEBUG_VM is the
> best config because that tends to be enabled quite often. Maybe
> CONFIG_MEMBLOCK_DEBUG? Or even make it kernel command line parameter?
> 

Initially, I did not want to make it CONFIG_MEMBLOCK_DEBUG because we 
really benefit from this debugging code when VM debug is enabled, and 
especially struct page debugging asserts which also depend on 
CONFIG_DEBUG_VM.

However, now thinking about it, I will change it to 
CONFIG_MEMBLOCK_DEBUG, and let users decide what other debugging configs 
need to be enabled, as this is also OK.

Thank you,
Pasha

Michal Hocko Aug. 14, 2017, 11:50 a.m. UTC | #3

On Fri 11-08-17 12:18:24, Pasha Tatashin wrote:
> >>When CONFIG_DEBUG_VM is enabled, this patch sets all the memory that is
> >>returned by memblock_virt_alloc_try_nid_raw() to ones to ensure that no
> >>places excpect zeroed memory.
> >
> >Please fold this into the patch which introduces
> >memblock_virt_alloc_try_nid_raw.
> 
> OK
> 
>  I am not sure CONFIG_DEBUG_VM is the
> >best config because that tends to be enabled quite often. Maybe
> >CONFIG_MEMBLOCK_DEBUG? Or even make it kernel command line parameter?
> >
> 
> Initially, I did not want to make it CONFIG_MEMBLOCK_DEBUG because we really
> benefit from this debugging code when VM debug is enabled, and especially
> struct page debugging asserts which also depend on CONFIG_DEBUG_VM.
> 
> However, now thinking about it, I will change it to CONFIG_MEMBLOCK_DEBUG,
> and let users decide what other debugging configs need to be enabled, as
> this is also OK.

Actually the more I think about it the more I am convinced that a kernel
boot parameter would be better because it doesn't need the kernel to be
recompiled and it is a single branch in not so hot path.

Pavel Tatashin Aug. 14, 2017, 2:01 p.m. UTC | #4

>> However, now thinking about it, I will change it to CONFIG_MEMBLOCK_DEBUG,
>> and let users decide what other debugging configs need to be enabled, as
>> this is also OK.
> 
> Actually the more I think about it the more I am convinced that a kernel
> boot parameter would be better because it doesn't need the kernel to be
> recompiled and it is a single branch in not so hot path.

The main reason I do not like kernel parameter is that automated test 
suits for every platform would need to be updated to include this new 
parameter in order to test it.

Yet, I think it is important at least initially to test it on every 
platform unconditionally when certain debug configs are enabled.

This patch series allows boot allocator to return uninitialized memory, 
this behavior Linux never had before, but way too often firmware 
explicitly zero all the memory before starting OS. Therefore, it would 
be hard to debug issues that might be only seen during kinit type of 
reboots.

In the future, when memory sizes will increase so that this memset will 
become unacceptable even on debug kernels, it can always be removed, but 
at least at that time we will know that the code has been tested for 
many years.

Michal Hocko Aug. 15, 2017, 9:36 a.m. UTC | #5

On Mon 14-08-17 10:01:52, Pasha Tatashin wrote:
> >>However, now thinking about it, I will change it to CONFIG_MEMBLOCK_DEBUG,
> >>and let users decide what other debugging configs need to be enabled, as
> >>this is also OK.
> >
> >Actually the more I think about it the more I am convinced that a kernel
> >boot parameter would be better because it doesn't need the kernel to be
> >recompiled and it is a single branch in not so hot path.
> 
> The main reason I do not like kernel parameter is that automated test suits
> for every platform would need to be updated to include this new parameter in
> order to test it.

How does this differ from the enabling a config option and building a
separate kernel?

My primary point of the kernel option was to have something available to
debug without recompiling the kernel which is more tedious than simply
adding one option to the kernel command line.

Patch

diff --git a/mm/memblock.c b/mm/memblock.c
index 3fbf3bcb52d9..29fcb1dd8a81 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1363,12 +1363,19 @@  void * __init memblock_virt_alloc_try_nid_raw(
 			phys_addr_t min_addr, phys_addr_t max_addr,
 			int nid)
 {
+	void *ptr;
+
 	memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
 		     __func__, (u64)size, (u64)align, nid, (u64)min_addr,
 		     (u64)max_addr, (void *)_RET_IP_);
 
-	return memblock_virt_alloc_internal(size, align,
-					    min_addr, max_addr, nid);
+	ptr = memblock_virt_alloc_internal(size, align,
+					   min_addr, max_addr, nid);
+#ifdef CONFIG_DEBUG_VM
+	if (ptr && size > 0)
+		memset(ptr, 0xff, size);
+#endif
+	return ptr;
 }
 
 /**