[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Jan Stancek]

Li reported, that some oom tests on aarch64 exit with TCONF
during is_numa() call, because get_mempolicy syscall is not
implemented. And because numa_helper used ltp_syscall() it
terminated the test in all instances.

Switch to syscall() and let the tests decide what should happen
if system doesn't support NUMA.

Reported-by: Li Wang <liwang@redhat.com>
Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/lib/numa_helper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/lib/numa_helper.c b/testcases/kernel/lib/numa_helper.c
index 0074180991ec..91214fcd85d9 100644
--- a/testcases/kernel/lib/numa_helper.c
+++ b/testcases/kernel/lib/numa_helper.c
@@ -76,14 +76,22 @@ static int filter_nodemask_mem(nodemask_t * nodemask, unsigned long max_node)
 {
 #if MPOL_F_MEMS_ALLOWED
 	unsigned long nodemask_size = max_node / 8;
+	int ret;
+
 	memset(nodemask, 0, nodemask_size);
 	/*
 	 * avoid numa_get_mems_allowed(), because of bug in getpol()
 	 * utility function in older versions:
 	 * http://www.spinics.net/lists/linux-numa/msg00849.html
 	 */
-	if (ltp_syscall(__NR_get_mempolicy, NULL, nodemask->n,
-		    max_node, 0, MPOL_F_MEMS_ALLOWED) < 0)
+	ret = syscall(__NR_get_mempolicy, NULL, nodemask->n,
+		    max_node, 0, MPOL_F_MEMS_ALLOWED);
+	/*
+	 * If we don't have __NR_get_mempolicy assume we can use all
+	 * present nodes. It is likely this is a system, that doesn't
+	 * support NUMA and there's just 1 node.
+	 */
+	if (ret < 0 && errno != ENOSYS)
 		return -2;
 #else
 	int i;
-- 
1.8.3.1

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Cyril Hrubis]

Hi!
> Li reported, that some oom tests on aarch64 exit with TCONF
> during is_numa() call, because get_mempolicy syscall is not
> implemented. And because numa_helper used ltp_syscall() it
> terminated the test in all instances.
> 
> Switch to syscall() and let the tests decide what should happen
> if system doesn't support NUMA.

Looking at the code the return value from filter_nodemask_mem() can be
propagated to get_allowed_nodes() and doing git grep "get_allowed_nodes"
suggets that most of the testcases does tst_brkm(TBROK | TERRNO, ...) if
the call returned non-zero. Have you checked that they exit with TCONF
on non-numa system before they reach the call to get_allowed_nodes()?

And the same for is_numa(), it calls tst_brkm(TBROK | TERRNO, ...) in
case that get_allowed_nodes_arr() returned non-zero.

Or am I missing something?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Monday, 22 February, 2016 12:36:15 PM
> Subject: Re: [LTP] [PATCH] numa_helper: don't break is_numa() with TCONF
> 
> Hi!
> > Li reported, that some oom tests on aarch64 exit with TCONF
> > during is_numa() call, because get_mempolicy syscall is not
> > implemented. And because numa_helper used ltp_syscall() it
> > terminated the test in all instances.
> > 
> > Switch to syscall() and let the tests decide what should happen
> > if system doesn't support NUMA.
> 
> Looking at the code the return value from filter_nodemask_mem() can be
> propagated to get_allowed_nodes() and doing git grep "get_allowed_nodes"
> suggets that most of the testcases does tst_brkm(TBROK | TERRNO, ...) if
> the call returned non-zero.

This patch hides ENOSYS returned from get_mempolicy. So the callers
won't get any more failures than before. Callers should still check
how many nodes have been returned and if that numbers works for
the test or not.

> Have you checked that they exit with TCONF
> on non-numa system before they reach the call to get_allowed_nodes()?

They should be free to call get_allowed_nodes(), at worst all they get
back is empty set.

We could add extra call of numa_available() to setup() before call to
get_allowed_nodes(), but since its implementation relies on same
syscall I'm not sure it's worth it:

int numa_available(void)
{
        if (get_mempolicy(NULL, NULL, 0, 0, 0) < 0 && errno == ENOSYS)
                return -1;
        return 0;
}

> 
> And the same for is_numa(), it calls tst_brkm(TBROK | TERRNO, ...) in
> case that get_allowed_nodes_arr() returned non-zero.

If get_allowed_nodes_arr() returned non-zero because of get_mempolicy,
that means get_mempolicy is implemented but failed. I think that
justifies TBROK.

Regards,
Jan

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Cyril Hrubis]

Hi!
> > And the same for is_numa(), it calls tst_brkm(TBROK | TERRNO, ...) in
> > case that get_allowed_nodes_arr() returned non-zero.
> 
> If get_allowed_nodes_arr() returned non-zero because of get_mempolicy,
> that means get_mempolicy is implemented but failed. I think that
> justifies TBROK.

Now I'm confused. You said that the syscall can exit -1 and ENOSYS. In
that case the filter_nodemask_mem() returns -2 which is propagated from
get_allowed_nodes_arr() to is_numa() where the test ends with TBROK
rather than with TCONF. Or did I miss something?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Monday, 22 February, 2016 2:25:22 PM
> Subject: Re: [LTP] [PATCH] numa_helper: don't break is_numa() with TCONF
> 
> Hi!
> > > And the same for is_numa(), it calls tst_brkm(TBROK | TERRNO, ...) in
> > > case that get_allowed_nodes_arr() returned non-zero.
> > 
> > If get_allowed_nodes_arr() returned non-zero because of get_mempolicy,
> > that means get_mempolicy is implemented but failed. I think that
> > justifies TBROK.
> 
> Now I'm confused. You said that the syscall can exit -1 and ENOSYS.

Correct, get_mempolicy is not implemented on aarch64.

> In that case the filter_nodemask_mem() returns -2 which is propagated from
> get_allowed_nodes_arr() to is_numa() where the test ends with TBROK
> rather than with TCONF.

It won't return -2 for ENOSYS, because of this line:
+        if (ret < 0 && errno != ENOSYS)
                 return -2;

> Or did I miss something?

I didn't make it clear in commit message that patch now silently ignores ENOSYS
from get_mempolicy. In this case get_allowed_nodes should return success, but
returned node set should be empty (I'll double check that this is the case on aarch64).

> 
> --
> Cyril Hrubis
> chrubis@suse.cz
> 

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Cyril Hrubis]

Hi!
> > Now I'm confused. You said that the syscall can exit -1 and ENOSYS.
> 
> Correct, get_mempolicy is not implemented on aarch64.
> 
> > In that case the filter_nodemask_mem() returns -2 which is propagated from
> > get_allowed_nodes_arr() to is_numa() where the test ends with TBROK
> > rather than with TCONF.
> 
> It won't return -2 for ENOSYS, because of this line:
> +        if (ret < 0 && errno != ENOSYS)
>                  return -2;
> 
> > Or did I miss something?
> 
> I didn't make it clear in commit message that patch now silently ignores ENOSYS
> from get_mempolicy. In this case get_allowed_nodes should return success, but
> returned node set should be empty (I'll double check that this is the case on aarch64).

Ah right I somehow missed that (I blame it on not enough coffe in
bloodstream). So is_numa() returns 0 in that case, which is fine.

If node set is empty the get_allowed_nodes() will fail since the
num_nodes will end up set to 0 and get_allowed_nodes() will return with
-3 and errno EINVAL since we cannot set the requested node. And looks
like at least oom04 and oom05 call TBROK if there isn't one allowed
node.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] numa_helper: don't break is_numa() with TCONF

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Monday, 22 February, 2016 3:26:33 PM
> Subject: Re: [LTP] [PATCH] numa_helper: don't break is_numa() with TCONF
> 
> Hi!
> > > Now I'm confused. You said that the syscall can exit -1 and ENOSYS.
> > 
> > Correct, get_mempolicy is not implemented on aarch64.
> > 
> > > In that case the filter_nodemask_mem() returns -2 which is propagated
> > > from
> > > get_allowed_nodes_arr() to is_numa() where the test ends with TBROK
> > > rather than with TCONF.
> > 
> > It won't return -2 for ENOSYS, because of this line:
> > +        if (ret < 0 && errno != ENOSYS)
> >                  return -2;
> > 
> > > Or did I miss something?
> > 
> > I didn't make it clear in commit message that patch now silently ignores
> > ENOSYS
> > from get_mempolicy. In this case get_allowed_nodes should return success,
> > but
> > returned node set should be empty (I'll double check that this is the case
> > on aarch64).
> 
> Ah right I somehow missed that (I blame it on not enough coffe in
> bloodstream). So is_numa() returns 0 in that case, which is fine.
> 
> If node set is empty the get_allowed_nodes() will fail since the
> num_nodes will end up set to 0 and get_allowed_nodes() will return with
> -3 and errno EINVAL since we cannot set the requested node. And looks
> like at least oom04 and oom05 call TBROK if there isn't one allowed
> node.

True, these needs some fixing too. I'll check all instances of get_allowed_nodes().

> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>