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

[David Hildenbrand (Red Hat)]

> > > +};
> >
> > 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