Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on offlined nodes

[Michal Hocko <mhocko@suse.com>]

On Mon 06-12-21 13:43:27, David Hildenbrand wrote:
[...]
> > Now practically speaking !node_online should not apear node_online (note
> > I am attentionally avoiding to say offline and online as that has a
> > completely different semantic) shouldn't really happen for some
> > architectures. x86 should allocate pgdat for each possible node. I do
> > not know what was the architecture in this case but we already have
> > another report for x86 that remains unexplained.
> 
> So we'd allocate the pgdat although all we want is just a zonelist. The
> obvious alternative is to implement the fallback where reasonable -- for
> example, in the page allocator. It knows the fallback order:
> build_zonelists(). That's pretty much all we need the preferred_nid for.
> 
> So just making prepare_alloc_pages()/node_zonelist() deal with a missing
> pgdat could make sense as well. Something like:
> 
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index b976c4177299..2d2649e78766 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -508,9 +508,14 @@ static inline int gfp_zonelist(gfp_t flags)
>   *
>   * For the case of non-NUMA systems the NODE_DATA() gets optimized to
>   * &contig_page_data at compile-time.
> + *
> + * If the node does not have a pgdat yet, returns the zonelist of the
> + * first online node.
>   */
>  static inline struct zonelist *node_zonelist(int nid, gfp_t flags)
>  {
> +       if (unlikely(!NODE_DATA(nid)))
> +               nid = first_online_node;
>         return NODE_DATA(nid)->node_zonelists + gfp_zonelist(flags);
>  }

This is certainly possible. But it a) adds a branch to the hotpath and
b) it doesn't solve any other potential dereference of garbage node.
 
> But of course, there might be value in a proper node-aware fallback list
> as we have in build_zonelists() -- but it remains questionable if the
> difference for these corner cases would be relevant in practice.

Only the platform knows the proper node topology and that includes
memory less nodes. So they should be setting up a node properly and we
shouldn't be dealing with this at the allocator nor any other code.

> Further, if we could have thousands of nodes, we'd have to update each
> and every one when building zone lists ...

Why would that be a practical problem?

> Removing the hotadd_new_pgdat() stuff does sound appealing, though.

Yes our hotplug code could be simplified as well. All we really need is
an arch code to initialize all the possible nodes.
-- 
Michal Hocko
SUSE Labs