Re: [PATCH 1/2] mm: Move page struct poisoning from CONFIG_DEBUG_VM to CONFIG_DEBUG_VM_PGFLAGS

[Dave Hansen <dave.hansen@intel.com>]

On 09/04/2018 11:33 AM, Alexander Duyck wrote:
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1444,7 +1444,7 @@ void * __init memblock_virt_alloc_try_nid_raw(
>  
>  	ptr = memblock_virt_alloc_internal(size, align,
>  					   min_addr, max_addr, nid);
> -#ifdef CONFIG_DEBUG_VM
> +#ifdef CONFIG_DEBUG_VM_PGFLAGS
>  	if (ptr && size > 0)
>  		memset(ptr, PAGE_POISON_PATTERN, size);
>  #endif
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 10b07eea9a6e..0fd9ad5021b0 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -696,7 +696,7 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		goto out;
>  	}
>  
> -#ifdef CONFIG_DEBUG_VM
> +#ifdef CONFIG_DEBUG_VM_PGFLAGS
>  	/*
>  	 * Poison uninitialized struct pages in order to catch invalid flags
>  	 * combinations.

I think this is the wrong way to do this.  It keeps the setting and
checking still rather tenuously connected.  If you were to leave it this
way, it needs commenting.  It's also rather odd that we're memsetting
the entire 'struct page' for a config option that's supposedly dealing
with page->flags.  That deserves _some_ addressing in a comment or
changelog.

How about:

#ifdef CONFIG_DEBUG_VM_PGFLAGS
#define VM_BUG_ON_PGFLAGS(cond, page) VM_BUG_ON_PAGE(cond, page)
+static inline void poison_struct_pages(struct page *pages, int nr)
+{
+	memset(pages, PAGE_POISON_PATTERN, size * sizeof(...));
+}
#else
#define VM_BUG_ON_PGFLAGS(cond, page) BUILD_BUG_ON_INVALID(cond)
static inline void poison_struct_pages(struct page *pages, int nr) {}
#endif

That puts the setting and checking in one spot, and also removes a
couple of #ifdefs from .c files.