Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 5 May 2022 12:37:46 +0200
From:   Oscar Salvador <osalvador@suse.de>
To:     David Hildenbrand <david@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Michal Hocko <mhocko@kernel.org>,
        Wei Yang <richard.weiyang@gmail.com>,
        Miaohe Lin <linmiaohe@huawei.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] mm/memory_hotplug: Reset node's state when empty
 during offline
Message-ID: <YnOo+nSCqMw0gWXB@localhost.localdomain>
References: <20220307150725.6810-1-osalvador@suse.de>
 <20220307150725.6810-3-osalvador@suse.de>
 <0d2853f6-66e5-251a-2d9e-c229f0ebcd5e@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0d2853f6-66e5-251a-2d9e-c229f0ebcd5e@redhat.com>
Precedence: bulk

On Thu, Apr 28, 2022 at 02:30:20PM +0200, David Hildenbrand wrote:
> On 07.03.22 16:07, Oscar Salvador wrote:
> > +static void node_reset_state(int node)
> > +{
> > +	pg_data_t *pgdat = NODE_DATA(node);
> > +	int cpu;
> > +
> > +	kswapd_stop(node);
> > +	kcompactd_stop(node);
> > +
> > +	reset_node_managed_pages(pgdat);
> > +	reset_node_present_pages(pgdat);
> > +
> > +	pgdat->nr_zones = 0;
> > +	pgdat->kswapd_order = 0;
> > +	pgdat->kswapd_highest_zoneidx = 0;
> > +	pgdat->node_start_pfn = 0;
> 
> 
> I'm confused why we have to mess with
> * present pages
> * managed pages
> * node_start_pfn
> 
> here at all.
> 
> 1) If there would be any present page left, calling node_reset_state()
> would be a BUG.
> 2) If there would be any manged page left, calling node_reset_state()
> would be a BUG.
> 3) node_start_pfn will be properly updated by
> remove_pfn_range_from_zone()->update_pgdat_span()

Yes, you are right, trusting update_pgdat_span() is the right to do
here.

> To make it clearer, I *think* touching node_start_pfn is very wrong.
> 
> What if the node still has ZONE_DEVICE? They don't account towards
> present pages but only towards spanned pages, and we're messing with the
> start range.

Did not think of that scenario, but as you said, we should be leaving
node/zone's pages accounting alone here.

> remove_pfn_range_from_zone()->update_pgdat_span() should be the only
> place that modifies the spanned range when offlining.

Will update the patch.

Thanks for the review David!


-- 
Oscar Salvador
SUSE Labs