Re: [PATCH] mm: initialize deferred pages with interrupts enabled

[Pavel Tatashin]

On Wed, Apr 1, 2020 at 4:10 PM Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> wrote:
>
> On Wed, Apr 01, 2020 at 04:00:27PM -0400, Daniel Jordan wrote:
> > On Wed, Apr 01, 2020 at 03:32:38PM -0400, Pavel Tatashin wrote:
> > > Initializing struct pages is a long task and keeping interrupts disabled
> > > for the duration of this operation introduces a number of problems.
> > >
> > > 1. jiffies are not updated for long period of time, and thus incorrect time
> > >    is reported. See proposed solution and discussion here:
> > >    lkml/20200311123848.118638-1-shile.zhang@xxxxxxxxxxxxxxxxx
> > > 2. It prevents farther improving deferred page initialization by allowing
> >
> >                                                                    not allowing
> > >    inter-node multi-threading.
> >
> >      intra-node
> >
> > ...
> > > After:
> > > [    1.632580] node 0 initialised, 12051227 pages in 436ms
> >
> > Fixes: 3a2d7fa8a3d5 ("mm: disable interrupts while initializing deferred pages")
> > Reported-by: Shile Zhang <shile.zhang@xxxxxxxxxxxxxxxxx>
> >
> > > Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> >
> > Freezing jiffies for a while during boot sounds like stable to me, so
> >
> > Cc: <stable@xxxxxxxxxxxxxxx>    [4.17.x+]
> >
> >
> > Can you please add a comment to mmzone.h above node_size_lock, something like
> >
> >          * Must be held any time you expect node_start_pfn,
> >          * node_present_pages, node_spanned_pages or nr_zones to stay constant.
> > +        * Also synchronizes pgdat->first_deferred_pfn during deferred page
> > +        * init.
> >          ...
> >         spinlock_t node_size_lock;
> >
> > > @@ -1854,18 +1859,6 @@ deferred_grow_zone(struct zone *zone, unsigned int order)
> > >             return false;
> > >
> > >     pgdat_resize_lock(pgdat, &flags);
> > > -
> > > -   /*
> > > -    * If deferred pages have been initialized while we were waiting for
> > > -    * the lock, return true, as the zone was grown.  The caller will retry
> > > -    * this zone.  We won't return to this function since the caller also
> > > -    * has this static branch.
> > > -    */
> > > -   if (!static_branch_unlikely(&deferred_pages)) {
> > > -           pgdat_resize_unlock(pgdat, &flags);
> > > -           return true;
> > > -   }
> > > -
> >
> > Huh, looks like this wasn't needed even before this change.
> >
> >
> > The rest looks fine.
> >
> > Reviewed-by: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>
>
> ...except for I forgot about the touch_nmi_watchdog() calls.  I think you'd
> need something kind of like this before your patch.

Thank you for review. You are right, I will add your patch, and modify
my to change touch_nmi_watchdog() to cond_resched().

Pasha