Date:   Mon, 6 Dec 2021 11:54:20 +0100
From:   Michal Hocko <mhocko@suse.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     Nico Pache <npache@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, akpm@linux-foundation.org, shakeelb@google.com,
        ktkhai@virtuozzo.com, shy828301@gmail.com, guro@fb.com,
        vbabka@suse.cz, vdavydov.dev@gmail.com, raquini@redhat.com
Subject: Re: [RFC PATCH 2/2] mm/vmscan.c: Prevent allocating shrinker_info on
 offlined nodes
Message-ID: <Ya3r3C/OpnK7OBPt@dhcp22.suse.cz>
References: <20211206033338.743270-1-npache@redhat.com>
 <20211206033338.743270-3-npache@redhat.com>
 <Ya3WcYKcej8XEI0W@dhcp22.suse.cz>
 <d9d14beb-ee20-7ebb-e007-fbf58fb28535@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d9d14beb-ee20-7ebb-e007-fbf58fb28535@redhat.com>
Precedence: bulk

On Mon 06-12-21 11:45:54, David Hildenbrand wrote:
> > This doesn't seen complete. Slab shrinkers are used in the reclaim
> > context. Previously offline nodes could be onlined later and this would
> > lead to NULL ptr because there is no hook to allocate new shrinker
> > infos. This would be also really impractical because this would have to
> > update all existing memcgs...
> 
> Instead of going through the trouble of updating...
> 
> ...  maybe just keep for_each_node() and check if the target node is
> offline. If it's offline, just allocate from the first online node.
> After all, we're not using __GFP_THISNODE, so there are no guarantees
> either way ...

This looks like another way to paper over a deeper underlying problem
IMHO. Fundamentally we have a problem that some pgdata are not allocated
and that causes a lot of headache. Not to mention that node_online
is just adding to a confusion because it doesn't really tell anything
about the logical state of the node.

I think we really should get rid of this approach rather than play a
whack-a-mole. We should really drop all notion of node_online and
instead allocate pgdat for each possible node. Arch specific code should
make sure that zone lists are properly initialized.
-- 
Michal Hocko
SUSE Labs