Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Jun 2022 06:25:21 +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>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] mm/memory_hotplug: Reset node's state when empty
 during offline
Message-ID: <YrKZsYNUAbfcC1CF@localhost.localdomain>
References: <20220621041717.6355-1-osalvador@suse.de>
 <20220621041717.6355-3-osalvador@suse.de>
 <139fc140-142f-c467-a5e3-0a0954dca127@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <139fc140-142f-c467-a5e3-0a0954dca127@redhat.com>
Precedence: bulk

On Tue, Jun 21, 2022 at 09:59:07AM +0200, David Hildenbrand wrote:
> > +static void node_reset_state(int node)
> > +{
> > +	pg_data_t *pgdat = NODE_DATA(node);
> > +	int cpu;
> > +
> > +	kswapd_stop(node);
> > +	kcompactd_stop(node);
> > +
> > +	pgdat->nr_zones = 0;
> 
> ^ what is that? it should be "highest_zone_idx" and I don't see any
> reason that we really need this.

Uhm, I thought we need to reset this, otherwise init_currently_empty_zone()
might not set it to a right value:

...
 if (zone_idx > pgdat->nr_zones)
    pgdat->nr_zones = zone_idx
...

At least we set it to 0 in free_area_init_core_hotplug() (before this patch).

> To detect if a node is empty we can use pgdat_is_empty(). To detect if a
> zone is empty we can use zone_is_empty().
> 
> The usage of "pgdat->nr_zones" as an optimization is questionable,
> especially when iterating over our handful of zones where most nodes
> miss the *lower* zones like ZONE_DMA* in practice and have ZONE_NORMAL.
> 
> Can we get rid of that and just check pgdat_is_empty() and
> zone_is_empty() and iterate all applicable zones from 0..X?

So, lemme see if I get you.
You mean to e.g: replace the following (code snippet from set_pgdat_percpu_threshold)

  for (i = 0; i < pgdat->nr_zones; i++) {
           zone = &pgdat->node_zones[i];

		    [some code]
  }

with this:

  for (zid = 0; zid < MAX_NR_ZONES; i++) {
            struct zone *zone = pgdat->node_zones + i;

            if (zone_is_empty(zone))
                    continue; 
  }

I guess we can, and I can see that we have a mix of both usages, so it might be
good to consolidate one.
And actually, I think we do the same amount of work, right? So not really an
optimization in those pieces of code.

The only thing that unsettles me is the compaction part.
We set pgdat->kcompactd_highest_zoneidx by checking pgdat->nr_zones, and use
that as our compact_control->highest_zoneidx. (kcompactd->kcompactd_do_work)

Now, I do not really see any reason we could not adapt that code to not
realy on pgdat->nr_zones, but I would have to check further how this
interacts with highest_zoneidx down the road, and where else should
we rewrite code.


-- 
Oscar Salvador
SUSE Labs