Re: [PATCH 2/3] mm/memory_hotplug: Reset node's state when empty during offline

[Oscar Salvador]

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