linux-kernel - Re: [External] Re: [PATCH] mm:page_alloc: fix the NULL ac->nodemask in __alloc_pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACSyD1OjnPU_CLbu9QNkbN41cGFmdwuzT8Kg4340Fk0MuUk+0w@mail.gmail.com>
Date: Thu, 22 Aug 2024 14:39:47 +0800
From: Zhongkun He <hezhongkun.hzk@...edance.com>
To: Michal Hocko <mhocko@...e.com>
Cc: akpm@...ux-foundation.org, mgorman@...hsingularity.net, hannes@...xchg.org, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, lizefan.x@...edance.com
Subject: Re: [External] Re: [PATCH] mm:page_alloc: fix the NULL ac->nodemask
 in __alloc_pages_slowpath()

> > > 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