Message-ID: <CAPcyv4hB0YPcuvMZSjbDXkhnvHnt49jzi-NvNnE-8--aFiZKwA@mail.gmail.com>
Date:   Tue, 29 Jan 2019 12:04:50 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Keith Busch <keith.busch@...el.com>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mel Gorman <mgorman@...e.de>
Subject: Re: [PATCH v7 1/3] mm: Shuffle initial free memory to improve
 memory-side-cache utilization

Whoops, did not reply to all your feedback, see below:

On Fri, Jan 25, 2019 at 6:21 AM Michal Hocko <mhocko@...nel.org> wrote:
[..]
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index cc4a507d7ca4..8c37a023a790 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1272,6 +1272,10 @@ void sparse_init(void);
> >  #else
> >  #define sparse_init()        do {} while (0)
> >  #define sparse_index_init(_sec, _nid)  do {} while (0)
> > +static inline int pfn_present(unsigned long pfn)
> > +{
> > +     return 1;
> > +}
>
> Does this really make sense? Shouldn't this default to pfn_valid on
> !sparsemem?

Yes, I think it should be pfn_valid()

>
> [...]
> > +config SHUFFLE_PAGE_ALLOCATOR
> > +     bool "Page allocator randomization"
> > +     depends on ACPI_NUMA
> > +     default SLAB_FREELIST_RANDOM
> > +     help
> > +       Randomization of the page allocator improves the average
> > +       utilization of a direct-mapped memory-side-cache. See section
> > +       5.2.27 Heterogeneous Memory Attribute Table (HMAT) in the ACPI
> > +       6.2a specification for an example of how a platform advertises
> > +       the presence of a memory-side-cache. There are also incidental
> > +       security benefits as it reduces the predictability of page
> > +       allocations to compliment SLAB_FREELIST_RANDOM, but the
> > +       default granularity of shuffling on 4MB (MAX_ORDER) pages is
> > +       selected based on cache utilization benefits.
> > +
> > +       While the randomization improves cache utilization it may
> > +       negatively impact workloads on platforms without a cache. For
> > +       this reason, by default, the randomization is enabled only
> > +       after runtime detection of a direct-mapped memory-side-cache.
> > +       Otherwise, the randomization may be force enabled with the
> > +       'page_alloc.shuffle' kernel command line parameter.
> > +
> > +       Say Y if unsure.
>
> Do we really need to make this a choice? Are any of the tiny systems
> going to be NUMA? Why cannot we just make it depend on ACPI_NUMA?

Kees wants to use this on ARM and I removed the ACPI_NUMA dependency
in v8 (you happened to review v7).

Given the setting has performance impact I believe it should allow for
being hard disabled at compile time, but I'll update the default to:

    default SLAB_FREELIST_RANDOM && ACPI_NUMA

>
> > +config SHUFFLE_PAGE_ORDER
> > +     depends on SHUFFLE_PAGE_ALLOCATOR
> > +     int "Page allocator shuffle order"
> > +     range 0 10
> > +     default 10
> > +     help
> > +       Specify the granularity at which shuffling (randomization) is
> > +       performed. By default this is set to MAX_ORDER-1 to minimize
> > +       runtime impact of randomization and with the expectation that
> > +       SLAB_FREELIST_RANDOM mitigates heap attacks on smaller
> > +       object granularities.
> > +
>
> and no, do not make this configurable here as already mentioned.

Will remove.

> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 022d4cbb3618..3602f7a2eab4 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/poison.h>
> >  #include <linux/pfn.h>
> >  #include <linux/debugfs.h>
> > +#include <linux/shuffle.h>
> >  #include <linux/kmemleak.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/memblock.h>
> > @@ -1929,9 +1930,16 @@ static unsigned long __init free_low_memory_core_early(void)
> >        *  low ram will be on Node1
> >        */
> >       for_each_free_mem_range(i, NUMA_NO_NODE, MEMBLOCK_NONE, &start, &end,
> > -                             NULL)
> > +                             NULL) {
> > +             pg_data_t *pgdat;
> > +
> >               count += __free_memory_core(start, end);
> >
> > +             for_each_online_pgdat(pgdat)
> > +                     shuffle_free_memory(pgdat, PHYS_PFN(start),
> > +                                     PHYS_PFN(end));
> > +     }
> > +
> >       return count;
> >  }
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index b9a667d36c55..7caffb9a91ab 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/highmem.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/ioport.h>
> > +#include <linux/shuffle.h>
> >  #include <linux/delay.h>
> >  #include <linux/migrate.h>
> >  #include <linux/page-isolation.h>
> > @@ -895,6 +896,8 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
> >       zone->zone_pgdat->node_present_pages += onlined_pages;
> >       pgdat_resize_unlock(zone->zone_pgdat, &flags);
> >
> > +     shuffle_zone(zone, pfn, zone_end_pfn(zone));
> > +
> >       if (onlined_pages) {
> >               node_states_set_node(nid, &arg);
> >               if (need_zonelists_rebuild)
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index cde5dac6229a..2adcd6da8a07 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -61,6 +61,7 @@
> >  #include <linux/sched/rt.h>
> >  #include <linux/sched/mm.h>
> >  #include <linux/page_owner.h>
> > +#include <linux/shuffle.h>
> >  #include <linux/kthread.h>
> >  #include <linux/memcontrol.h>
> >  #include <linux/ftrace.h>
> > @@ -1634,6 +1635,8 @@ static int __init deferred_init_memmap(void *data)
> >       }
> >       pgdat_resize_unlock(pgdat, &flags);
> >
> > +     shuffle_zone(zone, first_init_pfn, zone_end_pfn(zone));
> > +
> >       /* Sanity check that the next zone really is unpopulated */
> >       WARN_ON(++zid < MAX_NR_ZONES && populated_zone(++zone));
>
> I would prefer if would have less placess to place the shuffling. Why
> cannot we have a single place for the bootup and one for onlining part?
> page_alloc_init_late sounds like a good place for the later. You can
> miss some early allocations but are those of a big interest?

Ok, so you mean reduce the 3 callsites to 2. Replace the
free_low_memory_core_early() and deferred_init_memmap() sites with a
single shuffle call in page_alloc_init_late() after waiting for
deferred_init_memmap() work to complete? I don't see any red flags
with that, I'll give it a try.

> I haven't checked the actual shuffling algorithm, I will trust you on
> that part ;)

The algorithm has proved reliable. The breakage has only arisen from
missing locations that free large amounts of memory to the allocator
and failing to re-randomize within a whole zone, i.e. not just the
pages that were currently being hot-added.

Powered by blists - more mailing lists