linux-kernel - Re: [PATCH v1 1/4] mm/mempolicy: skip nodemask intersect check for 'interleave' when oom

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20210527130501.GC7743@shbuild999.sh.intel.com>
Date:   Thu, 27 May 2021 21:05:01 +0800
From:   Feng Tang <feng.tang@...el.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Ben Widawsky <ben.widawsky@...el.com>,
        linux-kernel@...r.kernel.org,
        Andrea Arcangeli <aarcange@...hat.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Andi Kleen <ak@...ux.intel.com>,
        Dan Williams <dan.j.williams@...el.com>, ying.huang@...el.com
Subject: Re: [PATCH v1 1/4] mm/mempolicy: skip nodemask intersect check for
 'interleave' when oom

On Thu, May 27, 2021 at 09:30:00AM +0200, Michal Hocko wrote:
> On Wed 26-05-21 13:01:39, Feng Tang wrote:
> > mempolicy_nodemask_intersects() is used in oom case to check if a
> > task may have memory allocated on some memory nodes.
> > 
> > Currently, the nodes_intersects() is run for both 'bind' and 'interleave'
> > policies. But they are different regarding memory allocation, the nodemask
> > is a forced requirement for 'bind', while just a hint for 'interleave'.
> > Like in alloc_pages_vma():
> > 
> > 	nmask = policy_nodemask(gfp, pol);
> >         preferred_nid = policy_node(gfp, pol, node);
> >         page = __alloc_pages(gfp, order, preferred_nid, nmask);
> > 
> > in plicy_nodemask(), only 'bind' policy may return its desired nodemask,
> > while others return NULL.  And this 'NULL' enables the 'interleave' policy
> > can get memory from other nodes than its nodemask.
> > 
> > So skip the nodemask intersect check for 'interleave' policy.
> 
> The changelog is not really clear on the actual effect of the
> patch and the above reference to alloc_pages_vma looks misleading to me
> because that path is never called for interleaved policy.

You are right. thanks for pointing it out.

Only the 'bind' policy calls policy_nodemask() and gets its preset
nodemask, while for 'interleave', alloc_page_interleave() calls
__alloc_pages() with NULL nodemask, so the conclusion is the same
that 'bind' policy can only get memory from its preset nodemask,
while 'interleave' can get memory from all nodes.

> This is very likely my fault because I was rather vague. The existing
> code in its current form is confusing but it _works_ properly. The
> problem is that it sounds like a general helper and in that regards
> the function is correct for the interleaved policy and your proposed
> preferred-many. But its only existing caller wants a different semantic.
> 
> Until now this was not a real problem even for OOM context because
> alloc_page_interleave is always used for the interleaving policy
> and that one doesn't use any node mask so the code is not really
> exercised. With your MPOL_PREFERRED this would no longer be the case.
 
Given the 'interleave' task may have memory allocated from all nodes,
shouldn't the mempolicy_nodemask_intersects() return true for 'interleave'?
or I'm still missing something?

> Your patch makes the code more robust for the oom context but it can
> confuse other users who really want to do an intersect logic. So I think
> it would really be best to rename the function and make it oom specific.
> E.g. mempolicy_in_oom_domain(tsk, mask) this would make it clear that
> this is not a general purpose function.

Ok, will rename like this.

Thanks,
Feng

> The changelog should be clear that this is just a code cleanup rather
> than fix.
> 
> -- 
> Michal Hocko
> SUSE Labs