[PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[Sergey Senozhatsky]

cpumask_next_and() is looking for cpumask_next() in src1 in a loop and
tests if found cpu is also present in src2. remove that loop, perform
cpumask_and() of src1 and src2 first and use that new mask to find
cpumask_next().

Apart from removing while loop, ./bloat-o-meter on x86_64 shows
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-8 (-8)
function                                     old     new   delta
cpumask_next_and                              62      54      -8

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 lib/cpumask.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/cpumask.c b/lib/cpumask.c
index b6513a9..5ab1553 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -37,10 +37,11 @@ EXPORT_SYMBOL(__next_cpu_nr);
 int cpumask_next_and(int n, const struct cpumask *src1p,
 		     const struct cpumask *src2p)
 {
-	while ((n = cpumask_next(n, src1p)) < nr_cpu_ids)
-		if (cpumask_test_cpu(n, src2p))
-			break;
-	return n;
+	struct cpumask tmp;
+
+	if (cpumask_and(&tmp, src1p, src2p))
+		return cpumask_next(n, &tmp);
+	return nr_cpu_ids;
 }
 EXPORT_SYMBOL(cpumask_next_and);
 
-- 
2.3.1.167.g7f4ba4b

Re: [PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[Peter Zijlstra]

On Mon, Mar 02, 2015 at 12:22:03AM +0900, Sergey Senozhatsky wrote:

> +++ b/lib/cpumask.c
> @@ -37,10 +37,11 @@ EXPORT_SYMBOL(__next_cpu_nr);
>  int cpumask_next_and(int n, const struct cpumask *src1p,
>  		     const struct cpumask *src2p)
>  {
> +	struct cpumask tmp;
> +
> +	if (cpumask_and(&tmp, src1p, src2p))
> +		return cpumask_next(n, &tmp);
> +	return nr_cpu_ids;
>  }
>  EXPORT_SYMBOL(cpumask_next_and);

Just ran into this; I though we were not supposed to put cpumasks on the
stack because $BIG. ?!

explain.

Re: [PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[Borislav Petkov]

On Mon, Jun 15, 2015 at 03:12:21PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 02, 2015 at 12:22:03AM +0900, Sergey Senozhatsky wrote:
> 
> > +++ b/lib/cpumask.c
> > @@ -37,10 +37,11 @@ EXPORT_SYMBOL(__next_cpu_nr);
> >  int cpumask_next_and(int n, const struct cpumask *src1p,
> >  		     const struct cpumask *src2p)
> >  {
> > +	struct cpumask tmp;
> > +
> > +	if (cpumask_and(&tmp, src1p, src2p))
> > +		return cpumask_next(n, &tmp);
> > +	return nr_cpu_ids;
> >  }
> >  EXPORT_SYMBOL(cpumask_next_and);
> 
> Just ran into this; I though we were not supposed to put cpumasks on the
> stack because $BIG. ?!
> 
> explain.

That's some fat stack with 8K CPUs:

cpumask_next_and:
        pushq   %rbp    #
        movq    %rsp, %rbp      #,
        pushq   %rbx    #
        movl    %edi, %ebx      # n, n
        leaq    -1040(%rbp), %rdi       #, tmp106
        subq    $1032, %rsp     #,
		^^^^^

Lovely.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[Sergey Senozhatsky]

Hello,

On (06/15/15 15:12), Peter Zijlstra wrote:
> > +++ b/lib/cpumask.c
> > @@ -37,10 +37,11 @@ EXPORT_SYMBOL(__next_cpu_nr);
> >  int cpumask_next_and(int n, const struct cpumask *src1p,
> >  		     const struct cpumask *src2p)
> >  {
> > +	struct cpumask tmp;
> > +
> > +	if (cpumask_and(&tmp, src1p, src2p))
> > +		return cpumask_next(n, &tmp);
> > +	return nr_cpu_ids;
> >  }
> >  EXPORT_SYMBOL(cpumask_next_and);
> 
> Just ran into this; I though we were not supposed to put cpumasks on the
> stack because $BIG. ?!

Gosh, I didn't think $BIG enough. So, on a _big_ 4096 x86_64 it's like...
64 bytes on stack. That's bad. alloc_cpumask_var()/free_cpumask_var()
version just doesn't look like a win (inlined below) so I guess I'll
ask to revert. It makes sense on smaller systems, but loses on huge
ones.

---

 lib/cpumask.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/cpumask.c b/lib/cpumask.c
index 5f62708..95ce89a 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -16,11 +16,17 @@
 int cpumask_next_and(int n, const struct cpumask *src1p,
 		     const struct cpumask *src2p)
 {
-	struct cpumask tmp;
+	int ret = nr_cpu_ids;
+	cpumask_var_t tmp;
 
-	if (cpumask_and(&tmp, src1p, src2p))
-		return cpumask_next(n, &tmp);
-	return nr_cpu_ids;
+	if (!alloc_cpumask_var(&tmp, GFP_KERNEL))
+		return ret;
+
+	if (cpumask_and(tmp, src1p, src2p))
+		ret = cpumask_next(n, tmp);
+
+	free_cpumask_var(tmp);
+	return ret;
 }
 EXPORT_SYMBOL(cpumask_next_and);
 

Re: [PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[Sergey Senozhatsky]

On (06/15/15 15:40), Borislav Petkov wrote:
[..]
> 
> That's some fat stack with 8K CPUs:
> 
> cpumask_next_and:
>         pushq   %rbp    #
>         movq    %rsp, %rbp      #,
>         pushq   %rbx    #
>         movl    %edi, %ebx      # n, n
>         leaq    -1040(%rbp), %rdi       #, tmp106
>         subq    $1032, %rsp     #,
> 		^^^^^
> 
> Lovely.
>

Oh, wow, 8K cpus...

No excuse.

	-ss

Re: [PATCH resend] cpumask: don't perform while loop in cpumask_next_and()

[Borislav Petkov]

On Mon, Jun 15, 2015 at 11:33:52PM +0900, Sergey Senozhatsky wrote:
> Oh, wow, 8K cpus...

Standard CONFIG_MAXSMP setting. I believe distros set that. At least
SLES on x86_64 does and I'd bet RHEL does that too.

config NR_CPUS
        int "Maximum number of CPUs" if SMP && !MAXSMP
        range 2 8 if SMP && X86_32 && !X86_BIGSMP
        range 2 512 if SMP && !MAXSMP && !CPUMASK_OFFSTACK
        range 2 8192 if SMP && !MAXSMP && CPUMASK_OFFSTACK && X86_64
        default "1" if !SMP
        default "8192" if MAXSMP
		^^^^^^

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--