Re: [External] Re: [PATCH] mm:page_alloc: fix the NULL ac->nodemask in __alloc_pages_slowpath()

[Zhongkun He]

> > > Are you suggesting that the problem is that should_reclaim_retry is
> > > iterating nodes which are not allowed by cpusets and that makes the
> > > retry loop happening more than unnecessary?
> >
> > Yes, exactly.
> >
> > >
> > > Is there any reason why you haven't done the same that the page
> > > allocator does in this case?
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 28f80daf5c04..cbf09c0e3b8a 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -4098,6 +4098,11 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
> > >                 unsigned long min_wmark = min_wmark_pages(zone);
> > >                 bool wmark;
> > >
> > > +               if (cpusets_enabled() &&
> > > +                       (alloc_flags & ALLOC_CPUSET) &&
> > > +                       !__cpuset_zone_allowed(zone, gfp_mask))
> > > +                               continue;
> > > +
> > >                 available = reclaimable = zone_reclaimable_pages(zone);
> > >                 available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> > >
> >
> > That was my original version,  but I found that the problem exists in
> > other places.
> > Please see the function flow below.
> >
> > __alloc_pages_slowpath:
> >
> >   get_page_from_freelist
> >      __cpuset_zone_allowed /* check the node */
> >
> >   __alloc_pages_direct_reclaim
> >      shrink_zones
> >         cpuset_zone_allowed（）/* check the node */
> >
> >  __alloc_pages_direct_compact
> >    try_to_compact_pages
> >        /* do not check the cpuset_zone_allowed（）*/
> >
> >   should_reclaim_retry
> >   /* do not check the cpuset_zone_allowed（）*/
> >
> >   should_compact_retry
> >       compaction_zonelist_suitable
> >           /* do not check the cpuset_zone_allowed（）*/
> >
> > Should we add __cpuset_zone_allowed() checks in the three functions
> > listed above,
> > or should we set the nodemask in __alloc_pages_slowpath()  if it is empty
> > and the request comes from user space?
>
> cpuset integration into the page allocator is rather complex (check
> ALLOC_CPUSET) use. Reviewing your change is not an easy task to make
> sure all the subtlety is preserved. Therefore I would suggest addressing
> the specific issue you have found.
>

Got it，thanks for your suggestion， i will send the next version soon.

> --
> Michal Hocko
> SUSE Labs