[PATCH]sched: completion: use bool in try_wait_for_completion

[PATCH]sched: completion: use bool in try_wait_for_completion

[gaurav jindal]

Variable ret in try_wait_for_completion can have only true/false values. Since
the return type of the function is also bool, variable ret should have data
type as bool in place of int.
Moreover, the size of bool in many platforms is 1 byte whereas size of int is
4 bytes(though architecture dependent). Modifying the data type reduces the 
size consumed for the stack.

Signed-off-by: Gaurav Jindal<gauravjindal1104@gmail.com>

---

diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index 0926aef..3e15e8d 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -283,7 +283,7 @@ int __sched wait_for_completion_killable(struct completion *x)
 bool try_wait_for_completion(struct completion *x)
 {
 	unsigned long flags;
-	int ret = 1;
+	bool ret = true;

 	/*
 	 * Since x->done will need to be locked only
@@ -292,11 +292,11 @@ bool try_wait_for_completion(struct completion *x)
 	 * return early in the blocking case.
 	 */
 	if (!READ_ONCE(x->done))
-		return 0;
+		return false;

 	spin_lock_irqsave(&x->wait.lock, flags);
 	if (!x->done)
-		ret = 0;
+		ret = false;
 	else if (x->done != UINT_MAX)
 		x->done--;
 	spin_unlock_irqrestore(&x->wait.lock, flags);

Re: [PATCH]sched: completion: use bool in try_wait_for_completion

[Peter Zijlstra]

On Wed, Feb 21, 2018 at 06:24:07PM +0530, gaurav jindal wrote:
> Variable ret in try_wait_for_completion can have only true/false values. Since
> the return type of the function is also bool, variable ret should have data
> type as bool in place of int.

Fair enough.

> Moreover, the size of bool in many platforms is 1 byte whereas size of int is
> 4 bytes(though architecture dependent). Modifying the data type reduces the 
> size consumed for the stack.

Absolutely 0 difference in generated assembly here on x86_64-defconfig
gcc Debian 7.2.0-20.

$ objdump -dr defconfig-build/kernel/sched/completion.o | awk '/^$/ {p=0} /<try_wait_for_completion>:$/ {p=1} {if (p) print $0}'

0000000000000090 <try_wait_for_completion>:
  90:   41 54                   push   %r12
  92:   55                      push   %rbp
  93:   31 ed                   xor    %ebp,%ebp
  95:   53                      push   %rbx
  96:   8b 07                   mov    (%rdi),%eax
  98:   85 c0                   test   %eax,%eax
  9a:   75 07                   jne    a3 <try_wait_for_completion+0x13>
  9c:   89 e8                   mov    %ebp,%eax
  9e:   5b                      pop    %rbx
  9f:   5d                      pop    %rbp
  a0:   41 5c                   pop    %r12
  a2:   c3                      retq   
  a3:   4c 8d 67 08             lea    0x8(%rdi),%r12
  a7:   48 89 fb                mov    %rdi,%rbx
  aa:   4c 89 e7                mov    %r12,%rdi
  ad:   e8 00 00 00 00          callq  b2 <try_wait_for_completion+0x22>
                        ae: R_X86_64_PC32       _raw_spin_lock_irqsave-0x4
  b2:   8b 13                   mov    (%rbx),%edx
  b4:   85 d2                   test   %edx,%edx
  b6:   74 0f                   je     c7 <try_wait_for_completion+0x37>
  b8:   83 fa ff                cmp    $0xffffffff,%edx
  bb:   bd 01 00 00 00          mov    $0x1,%ebp
  c0:   74 05                   je     c7 <try_wait_for_completion+0x37>
  c2:   83 ea 01                sub    $0x1,%edx
  c5:   89 13                   mov    %edx,(%rbx)
  c7:   48 89 c6                mov    %rax,%rsi
  ca:   4c 89 e7                mov    %r12,%rdi
  cd:   e8 00 00 00 00          callq  d2 <try_wait_for_completion+0x42>
                        ce: R_X86_64_PC32       _raw_spin_unlock_irqrestore-0x4
  d2:   89 e8                   mov    %ebp,%eax
  d4:   5b                      pop    %rbx
  d5:   5d                      pop    %rbp
  d6:   41 5c                   pop    %r12
  d8:   c3                      retq   
  d9:   0f 1f 80 00 00 00 00    nopl   0x0(%rax)

Note how it keeps the return value in eax and doesn't spill to the
stack. And I would expect this to be true for most architectures that
have register based calling conventions, its an otherwise fairly trivial
function.

I'll take the patch though, but I'll remove that last bit from the
Changelog.

Re: [PATCH]sched: completion: use bool in try_wait_for_completion

[gaurav jindal]

On Wed, Feb 21, 2018 at 02:28:54PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 21, 2018 at 06:24:07PM +0530, gaurav jindal wrote:
> > Variable ret in try_wait_for_completion can have only true/false values. Since
> > the return type of the function is also bool, variable ret should have data
> > type as bool in place of int.
> 
> Fair enough.
> 
> > Moreover, the size of bool in many platforms is 1 byte whereas size of int is
> > 4 bytes(though architecture dependent). Modifying the data type reduces the 
> > size consumed for the stack.
> 
> Absolutely 0 difference in generated assembly here on x86_64-defconfig
> gcc Debian 7.2.0-20.
> 
> $ objdump -dr defconfig-build/kernel/sched/completion.o | awk '/^$/ {p=0} /<try_wait_for_completion>:$/ {p=1} {if (p) print $0}'
> 
> 0000000000000090 <try_wait_for_completion>:
>   90:   41 54                   push   %r12
>   92:   55                      push   %rbp
>   93:   31 ed                   xor    %ebp,%ebp
>   95:   53                      push   %rbx
>   96:   8b 07                   mov    (%rdi),%eax
>   98:   85 c0                   test   %eax,%eax
>   9a:   75 07                   jne    a3 <try_wait_for_completion+0x13>
>   9c:   89 e8                   mov    %ebp,%eax
>   9e:   5b                      pop    %rbx
>   9f:   5d                      pop    %rbp
>   a0:   41 5c                   pop    %r12
>   a2:   c3                      retq   
>   a3:   4c 8d 67 08             lea    0x8(%rdi),%r12
>   a7:   48 89 fb                mov    %rdi,%rbx
>   aa:   4c 89 e7                mov    %r12,%rdi
>   ad:   e8 00 00 00 00          callq  b2 <try_wait_for_completion+0x22>
>                         ae: R_X86_64_PC32       _raw_spin_lock_irqsave-0x4
>   b2:   8b 13                   mov    (%rbx),%edx
>   b4:   85 d2                   test   %edx,%edx
>   b6:   74 0f                   je     c7 <try_wait_for_completion+0x37>
>   b8:   83 fa ff                cmp    $0xffffffff,%edx
>   bb:   bd 01 00 00 00          mov    $0x1,%ebp
>   c0:   74 05                   je     c7 <try_wait_for_completion+0x37>
>   c2:   83 ea 01                sub    $0x1,%edx
>   c5:   89 13                   mov    %edx,(%rbx)
>   c7:   48 89 c6                mov    %rax,%rsi
>   ca:   4c 89 e7                mov    %r12,%rdi
>   cd:   e8 00 00 00 00          callq  d2 <try_wait_for_completion+0x42>
>                         ce: R_X86_64_PC32       _raw_spin_unlock_irqrestore-0x4
>   d2:   89 e8                   mov    %ebp,%eax
>   d4:   5b                      pop    %rbx
>   d5:   5d                      pop    %rbp
>   d6:   41 5c                   pop    %r12
>   d8:   c3                      retq   
>   d9:   0f 1f 80 00 00 00 00    nopl   0x0(%rax)
> 
> Note how it keeps the return value in eax and doesn't spill to the
> stack. And I would expect this to be true for most architectures that
> have register based calling conventions, its an otherwise fairly trivial
> function.
>
I completely agree. I got carried away with sizeof(). Missed the case of using
the local registers.
Thanks a lot for guiding me again.
> I'll take the patch though, but I'll remove that last bit from the
> Changelog.

[tip:sched/core] sched/completions: Use bool in try_wait_for_completion()

[tip-bot for gaurav jindal]

Commit-ID:  d17067e4487adc53bedb43681b3cb5a1714ff6ca
Gitweb:     https://git.kernel.org/tip/d17067e4487adc53bedb43681b3cb5a1714ff6ca
Author:     gaurav jindal <gauravjindal1104@gmail.com>
AuthorDate: Wed, 21 Feb 2018 18:24:07 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:00:18 +0100

sched/completions: Use bool in try_wait_for_completion()

Since the return type of the function is bool, the internal
'ret' variable should be bool too.

Signed-off-by: Gaurav Jindal<gauravjindal1104@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20180221125407.GA14292@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/completion.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index 5d2d56b0817a..e426b0cb9ac6 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -280,7 +280,7 @@ EXPORT_SYMBOL(wait_for_completion_killable_timeout);
 bool try_wait_for_completion(struct completion *x)
 {
 	unsigned long flags;
-	int ret = 1;
+	bool ret = true;
 
 	/*
 	 * Since x->done will need to be locked only
@@ -289,11 +289,11 @@ bool try_wait_for_completion(struct completion *x)
 	 * return early in the blocking case.
 	 */
 	if (!READ_ONCE(x->done))
-		return 0;
+		return false;
 
 	spin_lock_irqsave(&x->wait.lock, flags);
 	if (!x->done)
-		ret = 0;
+		ret = false;
 	else if (x->done != UINT_MAX)
 		x->done--;
 	spin_unlock_irqrestore(&x->wait.lock, flags);