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
