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
