On 22.06.22 06:25, Oscar Salvador wrote:
> 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).
Yeah, I don't think we need to, the nodes+zones should be empty either
way. Maybe a micro-optimization that doesn't really optimize anything
that much?
free_area_init_node() warns if it isn't reset, but my gut feeling is
this is completely unnecessary. We're not dealing with 200 zones ...
>
>> 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;
> }
Yes, some places that are not interested in ZONE_DEVICE might want to
skip that completely. See below.
>
> 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.
That's my understanding.
>
> 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)
I wonder why we would want to use ZONE_DEVICE there ...
move_pfn_range_to_zone()->init_currently_empty_zone() would set
pgdat->nr_zones = ZONE_DEVICE.
Which looks unnecessary.
>
> 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.
I wonder if all we want is:
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index aab70355d64f..905919683025 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -497,8 +497,9 @@ enum zone_type {
* there can be false negatives).
*/
ZONE_MOVABLE,
+ __MAX_NR_BUDDY_ZONES,
#ifdef CONFIG_ZONE_DEVICE
- ZONE_DEVICE,
+ ZONE_DEVICE = __MAX_NR_BUDDY_ZONES,
#endif
__MAX_NR_ZONES
diff --git a/tools/testing/memblock/linux/mmzone.h
b/tools/testing/memblock/linux/mmzone.h
index 7c2eb5c9bb54..2c3492239e45 100644
--- a/tools/testing/memblock/linux/mmzone.h
+++ b/tools/testing/memblock/linux/mmzone.h
@@ -17,6 +17,7 @@ enum zone_type {
};
#define MAX_NR_ZONES __MAX_NR_ZONES
+#define MAX_NR_BUDDY_ZONES __MAX_NR_BUDDY_ZONES
#define MAX_ORDER 11
#define MAX_ORDER_NR_PAGES (1 << (MAX_ORDER - 1))
Then, all relevant buddy-related code (compaction ...) can simply run
for (i = 0; i < MAX_NR_BUDDY_ZONES; i++)
or
for (i = MAX_NR_BUDDY_ZONES - 1; i >= 0; i--)
and check if the relevant zone is empty.
There will still be users of MAX_NR_ZONES, though, that have to consider
ZONE_DEVICE as well.
--
Thanks,
David / dhildenb
