Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> writes:
> On Mon, 24 Apr 2023 04:50:37 +0100 Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
>> On Mon, Apr 24, 2023 at 11:07:56AM +0800, Yajun Deng wrote:
>> > Instead of define an index and determining if the zone has memory,
>> > introduce for_each_populated_zone_pgdat() helper that can be used
>> > to iterate over each populated zone in pgdat, and convert the most
>> > obvious users to it.
>>
>> I don't think the complexity of the helper justifies the simplification
>> of the users.
>
> Are you sure?
>
>> > +++ b/include/linux/mmzone.h
>> > @@ -1580,6 +1580,14 @@ extern struct zone *next_zone(struct zone *zone);
>> > ; /* do nothing */ \
>> > else
>> >
>> > +#define for_each_populated_zone_pgdat(zone, pgdat, max) \
>> > + for (zone = pgdat->node_zones; \
>> > + zone < pgdat->node_zones + max; \
>> > + zone++) \
>> > + if (!populated_zone(zone)) \
>> > + ; /* do nothing */ \
>> > + else
>> > +
>
> But each of the call sites is doing this, so at least the complexity is
> now seen in only one place.
>
> btw, do we need to do the test that way? Why won't this work?
>
> #define for_each_populated_zone_pgdat(zone, pgdat, max) \
> for (zone = pgdat->node_zones; \
> zone < pgdat->node_zones + max; \
> zone++) \
> if (populated_zone(zone))
>
> I suspect it was done the original way in order to save a tabstop,
> which is no longer needed.
This may cause unexpected effect when used with "if" statement. For
example,
if (something)
for_each_populated_zone_pgdat(zone, pgdat, max)
total += zone->present_pages;
else
pr_info("something is false!\n");
Best Regards,
Huang, Ying
