Re: [PATCH 2/2] mm: thp: fix transparent_hugepage/defrag = madvise || always

["Zi Yan" <zi.yan@cs.rutgers.edu>]

[-- Attachment #1: Type: text/plain, Size: 3745 bytes --]

On 29 Aug 2018, at 10:35, Michal Hocko wrote:

> On Wed 29-08-18 16:28:16, Michal Hocko wrote:
>> On Wed 29-08-18 09:28:21, Zi Yan wrote:
>> [...]
>>> This patch triggers WARN_ON_ONCE() in policy_node() when MPOL_BIND is used and THP is on.
>>> Should this WARN_ON_ONCE be removed?
>>>
>>>
>>> /*
>>> * __GFP_THISNODE shouldn't even be used with the bind policy
>>> * because we might easily break the expectation to stay on the
>>> * requested node and not break the policy.
>>> */
>>> WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
>>
>> This is really interesting. It seems to be me who added this warning but
>> I cannot simply make any sense of it. Let me try to dig some more.
>
> OK, I get it now. The warning seems to be incomplete. It is right to
> complain when __GFP_THISNODE disagrees with MPOL_BIND policy but that is
> not what we check here. Does this heal the warning?
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index da858f794eb6..7bb9354b1e4c 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1728,7 +1728,10 @@ static int policy_node(gfp_t gfp, struct mempolicy *policy,
>  		 * because we might easily break the expectation to stay on the
>  		 * requested node and not break the policy.
>  		 */
> -		WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
> +		if (policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE)) {
> +			nodemask_t *nmask = policy_nodemask(gfp, policy);
> +			WARN_ON_ONCE(!node_isset(nd, *nmask));
> +		}
>  	}
>
>  	return nd;

Unfortunately no. I simply ran “memhog -r3 1g membind 1” to test and the warning still showed up.

The reason is that nd is just a hint about which node to prefer for allocation and
can be ignored if it does not conform to mempolicy.
Taking my test as an example, if an application is only memory bound to node 1 but can run on any CPU
nodes and it launches on node 0, alloc_pages_vma() will see 0 as its node parameter
and passes 0 to policy_node()’s nd parameter. This should be OK, but your patches
would give a warning, because nd=0 is not set in nmask=1.

Now I get your comment “__GFP_THISNODE shouldn't even be used with the bind policy”,
since they are indeed incompatible. __GFP_THISNODE wants to use the node,
which can be ignored by MPOL_BIND policy.

IMHO, we could get rid of __GFP_THISNODE when MPOL_BIND is set, like

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 0d2be5786b0c..a0fcb998d277 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1722,14 +1722,6 @@ static int policy_node(gfp_t gfp, struct mempolicy *policy,
 {
        if (policy->mode == MPOL_PREFERRED && !(policy->flags & MPOL_F_LOCAL))
                nd = policy->v.preferred_node;
-       else {
-               /*
-                * __GFP_THISNODE shouldn't even be used with the bind policy
-                * because we might easily break the expectation to stay on the
-                * requested node and not break the policy.
-                */
-               WARN_ON_ONCE(policy->mode == MPOL_BIND && (gfp & __GFP_THISNODE));
-       }

        return nd;
 }
@@ -2026,6 +2018,13 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
                goto out;
        }

+       /*
+        * __GFP_THISNODE shouldn't even be used with the bind policy
+        * because we might easily break the expectation to stay on the
+        * requested node and not break the policy.
+        */
+       if (pol->mode == MPOL_BIND)
+               gfp &= ~__GFP_THISNODE;

        nmask = policy_nodemask(gfp, pol);
        preferred_nid = policy_node(gfp, pol, node);

What do you think?

—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]