Re: [PATCH v4] mm/memory hotplug/unplug: Optimize zone->contiguous update when changes pfn range

["David Hildenbrand (Red Hat)" <david@kernel.org>]

>>> +};
>>
>> I don't like that the defines don't match the enum name (zone_c... vs.
>> CONT... ).
>>
>> Essentially you want a "yes / no / maybe" tristate. I don't think we
>> have an existing type for that, unfortunately.
>>
>> enum zone_contig_state {
>>      ZONE_CONTIG_YES,
>>      ZONE_CONTIG_NO,
>>      ZONE_CONTIG_MAYBE,
>> };
>>
>> Maybe someone reading along has a better idea.
>>
> 
> I agree it's better. Will wait for a day or two to make the change.
> 

Yes, good idea. No needs to rush at this point because the merge window 
just opened up.

> 
>>> +
>>> +void set_zone_contiguous(struct zone *zone, enum
>>> zone_contiguous_state state);
>>>    bool pfn_range_intersects_zones(int nid, unsigned long start_pfn,
>>>                   unsigned long nr_pages);
>>>    diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 0be83039c3b5..b74e558ce822 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -544,6 +544,32 @@ static void update_pgdat_span(struct pglist_data
>>> *pgdat)
>>>        pgdat->node_spanned_pages = node_end_pfn - node_start_pfn;
>>>    }
>>>    +static enum zone_contiguous_state __meminit
>>> clear_zone_contiguous_for_shrinking(
>>> +        struct zone *zone, unsigned long start_pfn, unsigned long
>>> nr_pages)
>>> +{
>>> +    const unsigned long end_pfn = start_pfn + nr_pages;
>>> +    enum zone_contiguous_state result = CONTIGUOUS_UNDETERMINED;
>>> +
>>> +    /*
>>> +     * If the removed pfn range inside the original zone span, the
>>> contiguous
>>> +     * property is surely false.
>>> +     */
>>> +    if (start_pfn > zone->zone_start_pfn && end_pfn <
>>> zone_end_pfn(zone))
>>> +        result = CONTIGUOUS_DEFINITELY_NOT;
>>> +
>>> +    /*
>>> +     * If the removed pfn range is at the beginning or end of the
>>> +     * original zone span, the contiguous property is preserved when
>>> +     * the original zone is contiguous.
>>> +     */
>>> +    else if (start_pfn == zone->zone_start_pfn || end_pfn ==
>>> zone_end_pfn(zone))
>>> +        result = zone->contiguous ?
>>> +            CONTIGUOUS_DEFINITELY : CONTIGUOUS_UNDETERMINED;
>>> +
>>
>> See my comment below on how to make this readable.
>>
>>> +    clear_zone_contiguous(zone);
>>> +    return result;
>>> +}
>>> +
>>>    void remove_pfn_range_from_zone(struct zone *zone,
>>>                          unsigned long start_pfn,
>>>                          unsigned long nr_pages)
>>> @@ -551,6 +577,7 @@ void remove_pfn_range_from_zone(struct zone *zone,
>>>        const unsigned long end_pfn = start_pfn + nr_pages;
>>>        struct pglist_data *pgdat = zone->zone_pgdat;
>>>        unsigned long pfn, cur_nr_pages;
>>> +    enum zone_contiguous_state contiguous_state =
>>> CONTIGUOUS_UNDETERMINED;
>>>          /* Poison struct pages because they are now uninitialized
>>> again. */
>>>        for (pfn = start_pfn; pfn < end_pfn; pfn += cur_nr_pages) {
>>> @@ -571,12 +598,13 @@ void remove_pfn_range_from_zone(struct zone *zone,
>>>        if (zone_is_zone_device(zone))
>>>            return;
>>>    -    clear_zone_contiguous(zone);
>>> +    contiguous_state = clear_zone_contiguous_for_shrinking(
>>> +                zone, start_pfn, nr_pages);
>>
>> Reading this again, I wonder whether it would be nicer to have
>> something like:
>>
>> new_contig_state = zone_contig_state_after_shrinking();
>> clear_zone_contiguous(zone);
>>
>> or sth like that. Similar for the growing case.
>>
> 
> In both shrinking and growing case, separate the clear_zone_contiguous
> from the logic of zone state check, right?

Yes, I think that makes it look a bit nicer.

-- 
Cheers

David