On Tue, Sep 4, 2018 at 12:25 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> 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.
So the only issue with this is the fact that the code here is wrapped
in a check for CONFIG_DEBUG_VM, so if that isn't defined we end up
with build errors.
If the goal is to consolidate things I could probably look at adding a
function in include/linux/page-flags.h, probably next to PagePoisoned.
I could then probably just look at wrapping the memset call itself
with the CONFIG_DEBUG_VM_PGFLAGS instead of the entire function. I
could then place some code documentation in there explaining why it is
wrapped.
- Alex
