Message-ID: <0c5a157f-82cb-0ac8-9955-8f19469454bb@redhat.com>
Date:   Tue, 7 Dec 2021 11:15:33 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Content-Language: en-US
To:     Yang Shi <shy828301@gmail.com>
Cc:     Kirill Tkhai <ktkhai@virtuozzo.com>,
        Michal Hocko <mhocko@suse.com>, Nico Pache <npache@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Shakeel Butt <shakeelb@google.com>,
        Roman Gushchin <guro@fb.com>, Vlastimil Babka <vbabka@suse.cz>,
        Vladimir Davydov <vdavydov.dev@gmail.com>, raquini@redhat.com
References: <20211206033338.743270-1-npache@redhat.com>
 <20211206033338.743270-3-npache@redhat.com> <Ya3WcYKcej8XEI0W@dhcp22.suse.cz>
 <d9d14beb-ee20-7ebb-e007-fbf58fb28535@redhat.com>
 <24b4455c-aff9-ca9f-e29f-350833e7a0d1@virtuozzo.com>
 <CAHbLzko0UeNadswXEnwr6EtuKAZT4T-fnC5F7xnFcH4RbjhAiA@mail.gmail.com>
 <a48c16d6-07df-ff44-67e6-f0942672ec28@redhat.com>
 <CAHbLzkrfU3SQ8r4FyhumDHr02DSKd8oWbhwwVbBUHF7GCGY2Hg@mail.gmail.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on
 offlined nodes
In-Reply-To: <CAHbLzkrfU3SQ8r4FyhumDHr02DSKd8oWbhwwVbBUHF7GCGY2Hg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

>>
>> Short term I tend to like [2], because it avoids having to mess with all
>> such instances to eventually get it right and the temporary overhead
>> until we have the code reworked should be really negligible ...
> 
> Thanks, David. Basically either option looks fine to me. But I'm a
> little bit concerned about [2]. It silently changes the node requested
> by the callers. It actually papers over potential bugs? And what if

Hi,

It's just a preferred node, so we do have a node fallback already via
the zonelist to other nodes for proper online nodes -- and would have
the proper node fallback when preallcoating all pgdat.

*Not* doing the fallback with a preferred node that is not online could
be considered a BUG instead.

Note that [2] was just a quick draft. We might have to do some minor
adjustments to handle __GFP_THISNODE properly.

But after all, we have:

VM_WARN_ON((gfp_mask & __GFP_THISNODE) && !node_online(nid));

in __alloc_pages_node() and __folio_alloc_node(). So it might not be
worth adjusting at all.

> the callers specify __GFP_THISNODE (I didn't search if such callers
> really exist in the current code)?
> 
> How's about a helper function, for example, called
> kvmalloc_best_node()? It does:
> 
> void * kvmalloc_best_node(unsigned long size, int flag, int nid)
> {
>     bool onlined = node_online(nid);
> 
>     WARN_ON_ONCE((flag & __GFP_THISNODE) && !onlined);
> 
>     if (!onlined)
>         nid = -1;
> 
>     return kvmalloc_node(size, GFP_xxx, nid);
> }

We still have to "fix" each and every affected for_each_node() ... code
until we have preallcoation of pgdat for all possible nodes.

-- 
Thanks,

David / dhildenb