[PATCH] [semaphore] Removed redundant code from semaphore's down family of function

[PATCH] [semaphore] Removed redundant code from semaphore's down family of function

[Satendra Singh Thakur]

-The semaphore code has four funcs
down,
down_interruptible,
down_killable,
down_timeout
-These four funcs have almost similar code except that
they all call lower level function __down_xyz.
-This lower level func in-turn call inline func
__down_common with appropriate arguments.
-This patch creates a common macro for above family of funcs
so that duplicate code is eliminated.
-Also, __down_common has been made noinline so that code is
functionally similar to previous one
-For example, earlier down_killable would call __down_killable
, which in-turn would call inline func __down_common
Now, down_killable calls noinline __down_common directly
through a macro
-The funcs __down_interruptible, __down_killable etc have been
removed as they were just wrapper to __down_common

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
---
 kernel/locking/semaphore.c | 107 +++++++++++++------------------------
 1 file changed, 38 insertions(+), 69 deletions(-)

diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
index d9dd94defc0a..0468bc335908 100644
--- a/kernel/locking/semaphore.c
+++ b/kernel/locking/semaphore.c
@@ -33,11 +33,33 @@
 #include <linux/spinlock.h>
 #include <linux/ftrace.h>

-static noinline void __down(struct semaphore *sem);
-static noinline int __down_interruptible(struct semaphore *sem);
-static noinline int __down_killable(struct semaphore *sem);
-static noinline int __down_timeout(struct semaphore *sem, long timeout);
+static noinline int __sched __down_common(struct semaphore *sem, long state,
+long timeout);
 static noinline void __up(struct semaphore *sem);
+/**
+ * down_common - acquire the semaphore
+ * @sem: the semaphore to be acquired
+ * @state: new state of the task
+ * @timeout: either MAX_SCHEDULE_TIMEOUT or actual specified
+ * timeout
+ * Acquires the semaphore. If no more tasks are allowed to
+ * acquire the semaphore, calling this macro will put the task
+ * to sleep until the semaphore is released.
+ *
+ * This internally calls another func __down_common.
+ */
+#define down_common(sem, state, timeout)\
+({\
+int ret = 0;\
+unsigned long flags;\
+raw_spin_lock_irqsave(&(sem)->lock, flags);\
+if (likely((sem)->count > 0))\
+(sem)->count--;\
+else\
+ret = __down_common(sem, state, timeout);\
+raw_spin_unlock_irqrestore(&(sem)->lock, flags);\
+ret;\
+})

 /**
  * down - acquire the semaphore
@@ -52,14 +74,7 @@ static noinline void __up(struct semaphore *sem);
  */
 void down(struct semaphore *sem)
 {
-unsigned long flags;
-
-raw_spin_lock_irqsave(&sem->lock, flags);
-if (likely(sem->count > 0))
-sem->count--;
-else
-__down(sem);
-raw_spin_unlock_irqrestore(&sem->lock, flags);
+down_common(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down);

@@ -74,17 +89,7 @@ EXPORT_SYMBOL(down);
  */
 int down_interruptible(struct semaphore *sem)
 {
-unsigned long flags;
-int result = 0;
-
-raw_spin_lock_irqsave(&sem->lock, flags);
-if (likely(sem->count > 0))
-sem->count--;
-else
-result = __down_interruptible(sem);
-raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-return result;
+return down_common(sem, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down_interruptible);

@@ -100,17 +105,7 @@ EXPORT_SYMBOL(down_interruptible);
  */
 int down_killable(struct semaphore *sem)
 {
-unsigned long flags;
-int result = 0;
-
-raw_spin_lock_irqsave(&sem->lock, flags);
-if (likely(sem->count > 0))
-sem->count--;
-else
-result = __down_killable(sem);
-raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-return result;
+return down_common(sem, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down_killable);

@@ -154,17 +149,7 @@ EXPORT_SYMBOL(down_trylock);
  */
 int down_timeout(struct semaphore *sem, long timeout)
 {
-unsigned long flags;
-int result = 0;
-
-raw_spin_lock_irqsave(&sem->lock, flags);
-if (likely(sem->count > 0))
-sem->count--;
-else
-result = __down_timeout(sem, timeout);
-raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-return result;
+return down_common(sem, TASK_UNINTERRUPTIBLE, timeout);
 }
 EXPORT_SYMBOL(down_timeout);

@@ -196,12 +181,15 @@ struct semaphore_waiter {
 bool up;
 };

-/*
- * Because this function is inlined, the 'state' parameter will be
- * constant, and thus optimised away by the compiler.  Likewise the
- * 'timeout' parameter for the cases without timeouts.
+/**
+ * __down_common - Adds the current task to wait list
+ * puts the task to sleep until signal, timeout or up flag
+ * @sem: the semaphore to be acquired
+ * @state: the state of the calling task
+ * @timeout: either MAX_SCHEDULE_TIMEOUT or actual specified
+ * timeout
  */
-static inline int __sched __down_common(struct semaphore *sem, long state,
+static noinline int __sched __down_common(struct semaphore *sem, long state,
 long timeout)
 {
 struct semaphore_waiter waiter;
@@ -232,25 +220,6 @@ static inline int __sched __down_common(struct semaphore *sem, long state,
 return -EINTR;
 }

-static noinline void __sched __down(struct semaphore *sem)
-{
-__down_common(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_interruptible(struct semaphore *sem)
-{
-return __down_common(sem, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_killable(struct semaphore *sem)
-{
-return __down_common(sem, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_timeout(struct semaphore *sem, long timeout)
-{
-return __down_common(sem, TASK_UNINTERRUPTIBLE, timeout);
-}

 static noinline void __sched __up(struct semaphore *sem)
 {
--
2.17.1

::DISCLAIMER::
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
The contents of this e-mail and any attachment(s) are confidential and intended for the named recipient(s) only. E-mail transmission is not guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete, or may contain viruses in transmission. The e mail and its contents (with or without referred errors) shall therefore not attach any liability on the originator or HCL or its affiliates. Views or opinions, if any, presented in this email are solely those of the author and may not necessarily reflect the views or opinions of HCL or its affiliates. Any form of reproduction, dissemination, copying, disclosure, modification, distribution and / or publication of this message without the prior written consent of authorized representative of HCL is strictly prohibited. If you have received this email in error please delete it and notify the sender immediately. Before opening any email and/or attachments, please check them for viruses and other defects.
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Re: [PATCH] [semaphore] Removed redundant code from semaphore's down family of function

[Will Deacon]

On Mon, Aug 12, 2019 at 05:31:43AM +0000, Satendra Singh Thakur wrote:
> -The semaphore code has four funcs
> down,
> down_interruptible,
> down_killable,
> down_timeout
> -These four funcs have almost similar code except that
> they all call lower level function __down_xyz.
> -This lower level func in-turn call inline func
> __down_common with appropriate arguments.
> -This patch creates a common macro for above family of funcs
> so that duplicate code is eliminated.
> -Also, __down_common has been made noinline so that code is
> functionally similar to previous one
> -For example, earlier down_killable would call __down_killable
> , which in-turn would call inline func __down_common
> Now, down_killable calls noinline __down_common directly
> through a macro
> -The funcs __down_interruptible, __down_killable etc have been
> removed as they were just wrapper to __down_common
> 
> Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
> ---
>  kernel/locking/semaphore.c | 107 +++++++++++++------------------------
>  1 file changed, 38 insertions(+), 69 deletions(-)

Something has gone very wrong with your mail setup, so this patch is
mangled beyond repair.

Please read Documentation/process/email-clients.rst and also work with
your employer to remove the disclaimer at the end of your email too.

Thanks,

Will

[PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Satendra Singh Thakur]

-The semaphore code has four funcs
down,
down_interruptible,
down_killable,
down_timeout
-These four funcs have almost similar code except that
they all call lower level function __down_xyz.
-This lower level func in-turn call inline func
__down_common with appropriate arguments.
-This patch creates a common macro for above family of funcs
so that duplicate code is eliminated.
-Also, __down_common has been made noinline so that code is
functionally similar to previous one
-For example, earlier down_killable would call __down_killable
, which in-turn would call inline func __down_common
Now, down_killable calls noinline __down_common directly
through a macro
-The funcs __down_interruptible, __down_killable etc have been
removed as they were just wrapper to __down_common

Signed-off-by: Satendra Singh Thakur <satendrasingh.thakur@hcl.com>
---
 v1: removed disclaimer appended automatically by company mail policy

 kernel/locking/semaphore.c | 107 +++++++++++++------------------------
 1 file changed, 38 insertions(+), 69 deletions(-)

diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
index d9dd94defc0a..0468bc335908 100644
--- a/kernel/locking/semaphore.c
+++ b/kernel/locking/semaphore.c
@@ -33,11 +33,33 @@
 #include <linux/spinlock.h>
 #include <linux/ftrace.h>
 
-static noinline void __down(struct semaphore *sem);
-static noinline int __down_interruptible(struct semaphore *sem);
-static noinline int __down_killable(struct semaphore *sem);
-static noinline int __down_timeout(struct semaphore *sem, long timeout);
+static noinline int __sched __down_common(struct semaphore *sem, long state,
+								long timeout);
 static noinline void __up(struct semaphore *sem);
+/**
+ * down_common - acquire the semaphore
+ * @sem: the semaphore to be acquired
+ * @state: new state of the task
+ * @timeout: either MAX_SCHEDULE_TIMEOUT or actual specified
+ * timeout
+ * Acquires the semaphore. If no more tasks are allowed to
+ * acquire the semaphore, calling this macro will put the task
+ * to sleep until the semaphore is released.
+ *
+ * This internally calls another func __down_common.
+ */
+#define down_common(sem, state, timeout)	\
+({	\
+	int ret = 0;	\
+	unsigned long flags;	\
+	raw_spin_lock_irqsave(&(sem)->lock, flags);	\
+	if (likely((sem)->count > 0))	\
+		(sem)->count--;	\
+	else	\
+		ret = __down_common(sem, state, timeout);	\
+	raw_spin_unlock_irqrestore(&(sem)->lock, flags);	\
+	ret;	\
+})
 
 /**
  * down - acquire the semaphore
@@ -52,14 +74,7 @@ static noinline void __up(struct semaphore *sem);
  */
 void down(struct semaphore *sem)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		__down(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
+	down_common(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down);
 
@@ -74,17 +89,7 @@ EXPORT_SYMBOL(down);
  */
 int down_interruptible(struct semaphore *sem)
 {
-	unsigned long flags;
-	int result = 0;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		result = __down_interruptible(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-	return result;
+	return down_common(sem, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down_interruptible);
 
@@ -100,17 +105,7 @@ EXPORT_SYMBOL(down_interruptible);
  */
 int down_killable(struct semaphore *sem)
 {
-	unsigned long flags;
-	int result = 0;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		result = __down_killable(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-	return result;
+	return down_common(sem, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down_killable);
 
@@ -154,17 +149,7 @@ EXPORT_SYMBOL(down_trylock);
  */
 int down_timeout(struct semaphore *sem, long timeout)
 {
-	unsigned long flags;
-	int result = 0;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		result = __down_timeout(sem, timeout);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-	return result;
+	return down_common(sem, TASK_UNINTERRUPTIBLE, timeout);
 }
 EXPORT_SYMBOL(down_timeout);
 
@@ -196,12 +181,15 @@ struct semaphore_waiter {
 	bool up;
 };
 
-/*
- * Because this function is inlined, the 'state' parameter will be
- * constant, and thus optimised away by the compiler.  Likewise the
- * 'timeout' parameter for the cases without timeouts.
+/**
+ * __down_common - Adds the current task to wait list
+ * puts the task to sleep until signal, timeout or up flag
+ * @sem: the semaphore to be acquired
+ * @state: the state of the calling task
+ * @timeout: either MAX_SCHEDULE_TIMEOUT or actual specified
+ * timeout
  */
-static inline int __sched __down_common(struct semaphore *sem, long state,
+static noinline int __sched __down_common(struct semaphore *sem, long state,
 								long timeout)
 {
 	struct semaphore_waiter waiter;
@@ -232,25 +220,6 @@ static inline int __sched __down_common(struct semaphore *sem, long state,
 	return -EINTR;
 }
 
-static noinline void __sched __down(struct semaphore *sem)
-{
-	__down_common(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_interruptible(struct semaphore *sem)
-{
-	return __down_common(sem, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_killable(struct semaphore *sem)
-{
-	return __down_common(sem, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_timeout(struct semaphore *sem, long timeout)
-{
-	return __down_common(sem, TASK_UNINTERRUPTIBLE, timeout);
-}
 
 static noinline void __sched __up(struct semaphore *sem)
 {
-- 
2.17.1

Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Peter Zijlstra]

On Mon, Aug 12, 2019 at 07:18:59PM +0530, Satendra Singh Thakur wrote:
> -The semaphore code has four funcs
> down,
> down_interruptible,
> down_killable,
> down_timeout
> -These four funcs have almost similar code except that
> they all call lower level function __down_xyz.
> -This lower level func in-turn call inline func
> __down_common with appropriate arguments.
> -This patch creates a common macro for above family of funcs
> so that duplicate code is eliminated.
> -Also, __down_common has been made noinline so that code is
> functionally similar to previous one
> -For example, earlier down_killable would call __down_killable
> , which in-turn would call inline func __down_common
> Now, down_killable calls noinline __down_common directly
> through a macro
> -The funcs __down_interruptible, __down_killable etc have been
> removed as they were just wrapper to __down_common

The above is unreadable and seems to lack a reason for this change.

AFAICT from the actual patch, you're destroying the explicit
instantiation of the __down*() functions through constant propagation
into __down_common().

Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Satendra Singh Thakur]

On Thu, 22 Aug 2019 17:51:12 +0200, Peter Zijlstra wrote:
> On Mon, Aug 12, 2019 at 07:18:59PM +0530, Satendra Singh Thakur wrote:
> > -The semaphore code has four funcs
> > down,
> > down_interruptible,
> > down_killable,
> > down_timeout
> > -These four funcs have almost similar code except that
> > they all call lower level function __down_xyz.
> > -This lower level func in-turn call inline func
> > __down_common with appropriate arguments.
> > -This patch creates a common macro for above family of funcs
> > so that duplicate code is eliminated.
> > -Also, __down_common has been made noinline so that code is
> > functionally similar to previous one
> > -For example, earlier down_killable would call __down_killable
> > , which in-turn would call inline func __down_common
> > Now, down_killable calls noinline __down_common directly
> > through a macro
> > -The funcs __down_interruptible, __down_killable etc have been
> > removed as they were just wrapper to __down_common
>
> The above is unreadable and seems to lack a reason for this change.
Hi Mr Peter,
Thanks for the comments.
I will try to explain it further:

The semaphore has four functions named down*.
The call flow of the functions is

down* ----> __down* ----> inline __down_common

The code of down* and __down* is redundant/duplicate except that
the __down_common is called with different arguments from __down*
functions.

This patch defines a macro down_common which contain this common
code of all down* functions.

new call flow is

down* ----> noinline __down_common (through a macro down_common).

> AFAICT from the actual patch, you're destroying the explicit
> instantiation of the __down*() functions
> through constant propagation into __down_common().
Intead of instantiation of __down* functions, we are instaintiating
__down_common, is it a problem ?

Thanks
Satendra

Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Peter Zijlstra]

On Sat, Aug 24, 2019 at 09:20:59AM +0530, Satendra Singh Thakur wrote:
> On Thu, 22 Aug 2019 17:51:12 +0200, Peter Zijlstra wrote:
> > On Mon, Aug 12, 2019 at 07:18:59PM +0530, Satendra Singh Thakur wrote:
> > > -The semaphore code has four funcs
> > > down,
> > > down_interruptible,
> > > down_killable,
> > > down_timeout
> > > -These four funcs have almost similar code except that
> > > they all call lower level function __down_xyz.
> > > -This lower level func in-turn call inline func
> > > __down_common with appropriate arguments.
> > > -This patch creates a common macro for above family of funcs
> > > so that duplicate code is eliminated.
> > > -Also, __down_common has been made noinline so that code is
> > > functionally similar to previous one
> > > -For example, earlier down_killable would call __down_killable
> > > , which in-turn would call inline func __down_common
> > > Now, down_killable calls noinline __down_common directly
> > > through a macro
> > > -The funcs __down_interruptible, __down_killable etc have been
> > > removed as they were just wrapper to __down_common
> >
> > The above is unreadable and seems to lack a reason for this change.
> Hi Mr Peter,
> Thanks for the comments.
> I will try to explain it further:
> 
> The semaphore has four functions named down*.
> The call flow of the functions is
> 
> down* ----> __down* ----> inline __down_common
> 
> The code of down* and __down* is redundant/duplicate except that
> the __down_common is called with different arguments from __down*
> functions.
> 
> This patch defines a macro down_common which contain this common
> code of all down* functions.
> 
> new call flow is
> 
> down* ----> noinline __down_common (through a macro down_common).
> 
> > AFAICT from the actual patch, you're destroying the explicit
> > instantiation of the __down*() functions
> > through constant propagation into __down_common().
> Intead of instantiation of __down* functions, we are instaintiating
> __down_common, is it a problem ?

Then you loose the constant propagation into __down_common and you'll
not DCE the signal and/or timeout code. The function even has a comment
on explaining that. That said; the timeout DCE seems suboptimal.

Also; I'm thinking you can do it without that ugly CPP.

---
Subject: locking/semaphore: Simplify code flow

Ever since we got rid of the per arch assembly the structure of the
semaphore code is more complicated than it needs to be.

Specifically, the slow path calling convention is no more. With that,
Satendara noted that all functions calling __down_common() (aka the slow
path) were basically the same.

Fold the lot.

(XXX, we should probably move the schedule_timeout() thing into its own
patch)

Suggested-by: Satendra Singh Thakur <sst2005@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/sched.h      |  13 +++++-
 kernel/locking/semaphore.c | 105 +++++++++++++--------------------------------
 kernel/time/timer.c        |  44 +++++++------------
 3 files changed, 58 insertions(+), 104 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index f0edee94834a..8f09e8cebe5a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -214,7 +214,18 @@ extern void scheduler_tick(void);
 
 #define	MAX_SCHEDULE_TIMEOUT		LONG_MAX
 
-extern long schedule_timeout(long timeout);
+extern long __schedule_timeout(long timeout);
+
+static inline long schedule_timeout(long timeout)
+{
+	if (timeout == MAX_SCHEDULE_TIMEOUT) {
+		schedule();
+		return timeout;
+	}
+
+	return __schedule_timeout(timeout);
+}
+
 extern long schedule_timeout_interruptible(long timeout);
 extern long schedule_timeout_killable(long timeout);
 extern long schedule_timeout_uninterruptible(long timeout);
diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
index d9dd94defc0a..4585fa3a0984 100644
--- a/kernel/locking/semaphore.c
+++ b/kernel/locking/semaphore.c
@@ -33,11 +33,24 @@
 #include <linux/spinlock.h>
 #include <linux/ftrace.h>
 
-static noinline void __down(struct semaphore *sem);
-static noinline int __down_interruptible(struct semaphore *sem);
-static noinline int __down_killable(struct semaphore *sem);
-static noinline int __down_timeout(struct semaphore *sem, long timeout);
-static noinline void __up(struct semaphore *sem);
+static inline int
+__sched __down_slow(struct semaphore *sem, long state, long timeout);
+static void __up(struct semaphore *sem);
+
+static inline int __down(struct semaphore *sem, long state, long timeout)
+{
+	unsigned long flags;
+	int result = 0;
+
+	raw_spin_lock_irqsave(&sem->lock, flags);
+	if (likely(sem->count > 0))
+		sem->count--;
+	else
+		result = __down_slow(sem, state, timeout);
+	raw_spin_unlock_irqrestore(&sem->lock, flags);
+
+	return result;
+}
 
 /**
  * down - acquire the semaphore
@@ -52,14 +65,7 @@ static noinline void __up(struct semaphore *sem);
  */
 void down(struct semaphore *sem)
 {
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		__down(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
+	__down(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down);
 
@@ -74,17 +80,7 @@ EXPORT_SYMBOL(down);
  */
 int down_interruptible(struct semaphore *sem)
 {
-	unsigned long flags;
-	int result = 0;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		result = __down_interruptible(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-	return result;
+	return __down(sem, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down_interruptible);
 
@@ -100,17 +96,7 @@ EXPORT_SYMBOL(down_interruptible);
  */
 int down_killable(struct semaphore *sem)
 {
-	unsigned long flags;
-	int result = 0;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		result = __down_killable(sem);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-	return result;
+	return __down(sem, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT);
 }
 EXPORT_SYMBOL(down_killable);
 
@@ -154,17 +140,7 @@ EXPORT_SYMBOL(down_trylock);
  */
 int down_timeout(struct semaphore *sem, long timeout)
 {
-	unsigned long flags;
-	int result = 0;
-
-	raw_spin_lock_irqsave(&sem->lock, flags);
-	if (likely(sem->count > 0))
-		sem->count--;
-	else
-		result = __down_timeout(sem, timeout);
-	raw_spin_unlock_irqrestore(&sem->lock, flags);
-
-	return result;
+	return __down(sem, TASK_UNINTERRUPTIBLE, timeout);
 }
 EXPORT_SYMBOL(down_timeout);
 
@@ -201,14 +177,15 @@ struct semaphore_waiter {
  * constant, and thus optimised away by the compiler.  Likewise the
  * 'timeout' parameter for the cases without timeouts.
  */
-static inline int __sched __down_common(struct semaphore *sem, long state,
-								long timeout)
+static inline int 
+__sched __down_slow(struct semaphore *sem, long state, long timeout)
 {
-	struct semaphore_waiter waiter;
+	struct semaphore_waiter waiter = {
+		.task = current,
+		.up = false,
+	};
 
 	list_add_tail(&waiter.list, &sem->wait_list);
-	waiter.task = current;
-	waiter.up = false;
 
 	for (;;) {
 		if (signal_pending_state(state, current))
@@ -223,36 +200,16 @@ static inline int __sched __down_common(struct semaphore *sem, long state,
 			return 0;
 	}
 
- timed_out:
+timed_out:
 	list_del(&waiter.list);
 	return -ETIME;
 
- interrupted:
+interrupted:
 	list_del(&waiter.list);
 	return -EINTR;
 }
 
-static noinline void __sched __down(struct semaphore *sem)
-{
-	__down_common(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_interruptible(struct semaphore *sem)
-{
-	return __down_common(sem, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_killable(struct semaphore *sem)
-{
-	return __down_common(sem, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT);
-}
-
-static noinline int __sched __down_timeout(struct semaphore *sem, long timeout)
-{
-	return __down_common(sem, TASK_UNINTERRUPTIBLE, timeout);
-}
-
-static noinline void __sched __up(struct semaphore *sem)
+static void __up(struct semaphore *sem)
 {
 	struct semaphore_waiter *waiter = list_first_entry(&sem->wait_list,
 						struct semaphore_waiter, list);
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 0e315a2e77ae..13240dcea3e9 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1851,38 +1851,24 @@ static void process_timeout(struct timer_list *t)
  * jiffies will be returned.  In all cases the return value is guaranteed
  * to be non-negative.
  */
-signed long __sched schedule_timeout(signed long timeout)
+signed long __sched __schedule_timeout(signed long timeout)
 {
 	struct process_timer timer;
 	unsigned long expire;
 
-	switch (timeout)
-	{
-	case MAX_SCHEDULE_TIMEOUT:
-		/*
-		 * These two special cases are useful to be comfortable
-		 * in the caller. Nothing more. We could take
-		 * MAX_SCHEDULE_TIMEOUT from one of the negative value
-		 * but I' d like to return a valid offset (>=0) to allow
-		 * the caller to do everything it want with the retval.
-		 */
-		schedule();
-		goto out;
-	default:
-		/*
-		 * Another bit of PARANOID. Note that the retval will be
-		 * 0 since no piece of kernel is supposed to do a check
-		 * for a negative retval of schedule_timeout() (since it
-		 * should never happens anyway). You just have the printk()
-		 * that will tell you if something is gone wrong and where.
-		 */
-		if (timeout < 0) {
-			printk(KERN_ERR "schedule_timeout: wrong timeout "
+	/*
+	 * Another bit of PARANOID. Note that the retval will be
+	 * 0 since no piece of kernel is supposed to do a check
+	 * for a negative retval of schedule_timeout() (since it
+	 * should never happens anyway). You just have the printk()
+	 * that will tell you if something is gone wrong and where.
+	 */
+	if (timeout < 0) {
+		printk(KERN_ERR "schedule_timeout: wrong timeout "
 				"value %lx\n", timeout);
-			dump_stack();
-			current->state = TASK_RUNNING;
-			goto out;
-		}
+		dump_stack();
+		current->state = TASK_RUNNING;
+		goto out;
 	}
 
 	expire = timeout + jiffies;
@@ -1898,10 +1884,10 @@ signed long __sched schedule_timeout(signed long timeout)
 
 	timeout = expire - jiffies;
 
- out:
+out:
 	return timeout < 0 ? 0 : timeout;
 }
-EXPORT_SYMBOL(schedule_timeout);
+EXPORT_SYMBOL(__schedule_timeout);
 
 /*
  * We can use __set_current_state() here because schedule_timeout() calls

Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Peter Zijlstra]

On Mon, Aug 26, 2019 at 04:14:36PM +0200, Peter Zijlstra wrote:
> (XXX, we should probably move the schedule_timeout() thing into its own
> patch)

A better version here...

---
Subject: sched,time: Allow better constprop/DCE for schedule_timeout()

If timeout is constant and MAX_SCHEDULE_TIMEOUT, it would be nice to
allow to optimize away everything timeout.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/sched.h | 13 ++++++++++++-
 kernel/time/timer.c   | 52 ++++++++++++++++++++++++---------------------------
 2 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index f0edee94834a..6003e96bce52 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -214,7 +214,18 @@ extern void scheduler_tick(void);
 
 #define	MAX_SCHEDULE_TIMEOUT		LONG_MAX
 
-extern long schedule_timeout(long timeout);
+extern long __schedule_timeout(long timeout);
+
+static inline long schedule_timeout(long timeout)
+{
+	if (__builtin_constant_p(timeout) && timeout == MAX_SCHEDULE_TIMEOUT) {
+		schedule();
+		return timeout;
+	}
+
+	return __schedule_timeout(timeout);
+}
+
 extern long schedule_timeout_interruptible(long timeout);
 extern long schedule_timeout_killable(long timeout);
 extern long schedule_timeout_uninterruptible(long timeout);
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 0e315a2e77ae..912ae56b96b8 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1851,38 +1851,34 @@ static void process_timeout(struct timer_list *t)
  * jiffies will be returned.  In all cases the return value is guaranteed
  * to be non-negative.
  */
-signed long __sched schedule_timeout(signed long timeout)
+signed long __sched __schedule_timeout(signed long timeout)
 {
 	struct process_timer timer;
 	unsigned long expire;
 
-	switch (timeout)
-	{
-	case MAX_SCHEDULE_TIMEOUT:
-		/*
-		 * These two special cases are useful to be comfortable
-		 * in the caller. Nothing more. We could take
-		 * MAX_SCHEDULE_TIMEOUT from one of the negative value
-		 * but I' d like to return a valid offset (>=0) to allow
-		 * the caller to do everything it want with the retval.
-		 */
+	/*
+	 * We could take MAX_SCHEDULE_TIMEOUT from one of the negative values,
+	 * but I'd like to return a valid offset (>= 0) to allow the caller to
+	 * do everything it wants with the retval.
+	 */
+	if (timeout == MAX_SCHEDULE_TIMEOUT) {
 		schedule();
-		goto out;
-	default:
-		/*
-		 * Another bit of PARANOID. Note that the retval will be
-		 * 0 since no piece of kernel is supposed to do a check
-		 * for a negative retval of schedule_timeout() (since it
-		 * should never happens anyway). You just have the printk()
-		 * that will tell you if something is gone wrong and where.
-		 */
-		if (timeout < 0) {
-			printk(KERN_ERR "schedule_timeout: wrong timeout "
+		return timeout;
+	}
+
+	/*
+	 * Another bit of PARANOID. Note that the retval will be 0 since no
+	 * piece of kernel is supposed to do a check for a negative retval of
+	 * schedule_timeout() (since it should never happens anyway). You just
+	 * have the printk() that will tell you if something is gone wrong and
+	 * where.
+	 */
+	if (timeout < 0) {
+		printk(KERN_ERR "schedule_timeout: wrong timeout "
 				"value %lx\n", timeout);
-			dump_stack();
-			current->state = TASK_RUNNING;
-			goto out;
-		}
+		dump_stack();
+		current->state = TASK_RUNNING;
+		goto out;
 	}
 
 	expire = timeout + jiffies;
@@ -1898,10 +1894,10 @@ signed long __sched schedule_timeout(signed long timeout)
 
 	timeout = expire - jiffies;
 
- out:
+out:
 	return timeout < 0 ? 0 : timeout;
 }
-EXPORT_SYMBOL(schedule_timeout);
+EXPORT_SYMBOL(__schedule_timeout);
 
 /*
  * We can use __set_current_state() here because schedule_timeout() calls

Re: sched,time: Allow better constprop/DCE for schedule_timeout()

[kbuild test robot]

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

Hi Peter,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc6 next-20190826]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Zijlstra/sched-time-Allow-better-constprop-DCE-for-schedule_timeout/20190827-061612
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/um/shared/sysdep/kernel-offsets.h:3:0,
                    from arch/um/kernel/asm-offsets.c:1:
   include/linux/sched.h: In function 'schedule_timeout':
>> include/linux/sched.h:222:3: error: implicit declaration of function 'schedule'; did you mean 'schedule_work'? [-Werror=implicit-function-declaration]
      schedule();
      ^~~~~~~~
      schedule_work
   include/linux/sched.h: At top level:
   include/linux/sched.h:233:17: warning: conflicting types for 'schedule'
    asmlinkage void schedule(void);
                    ^~~~~~~~
   include/linux/sched.h:222:3: note: previous implicit declaration of 'schedule' was here
      schedule();
      ^~~~~~~~
   cc1: some warnings being treated as errors
   make[2]: *** [arch/um/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2
   8 real  6 user  4 sys  130.06% cpu 	make prepare

vim +222 include/linux/sched.h

   218	
   219	static inline long schedule_timeout(long timeout)
   220	{
   221		if (__builtin_constant_p(timeout) && timeout == MAX_SCHEDULE_TIMEOUT) {
 > 222			schedule();
   223			return timeout;
   224		}
   225	
   226		return __schedule_timeout(timeout);
   227	}
   228	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8296 bytes --]

Re: sched,time: Allow better constprop/DCE for schedule_timeout()

[kbuild test robot]

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

Hi Peter,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc6 next-20190826]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Peter-Zijlstra/sched-time-Allow-better-constprop-DCE-for-schedule_timeout/20190827-061612
config: x86_64-lkp (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/uaccess.h:5:0,
                    from include/linux/crypto.h:21,
                    from arch/x86/kernel/asm-offsets.c:9:
   include/linux/sched.h: In function 'schedule_timeout':
>> include/linux/sched.h:222:3: error: implicit declaration of function 'schedule' [-Werror=implicit-function-declaration]
      schedule();
      ^~~~~~~~
   include/linux/sched.h: At top level:
   include/linux/sched.h:233:17: warning: conflicting types for 'schedule'
    asmlinkage void schedule(void);
                    ^~~~~~~~
   include/linux/sched.h:222:3: note: previous implicit declaration of 'schedule' was here
      schedule();
      ^~~~~~~~
   cc1: some warnings being treated as errors
   make[2]: *** [arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2
   18 real  10 user  3 sys  78.71% cpu 	make prepare

vim +/schedule +222 include/linux/sched.h

   218	
   219	static inline long schedule_timeout(long timeout)
   220	{
   221		if (__builtin_constant_p(timeout) && timeout == MAX_SCHEDULE_TIMEOUT) {
 > 222			schedule();
   223			return timeout;
   224		}
   225	
   226		return __schedule_timeout(timeout);
   227	}
   228	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27702 bytes --]

Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Satendra Singh Thakur]

On Mon, 26 Aug 2019 16:25:57 +0200, Peter Zijlstra wrote:
>On Mon, Aug 26, 2019 at 04:14:36PM +0200, Peter Zijlstra wrote:
> > (XXX, we should probably move the schedule_timeout() thing into its own
> > patch)
>
> A better version here...
>
> ---
> Subject: sched,time: Allow better constprop/DCE for schedule_timeout()
> 
> If timeout is constant and MAX_SCHEDULE_TIMEOUT, it would be nice to
> allow to optimize away everything timeout.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> include/linux/sched.h | 13 ++++++++++++-
> kernel/time/timer.c   | 52 ++++++++++++++++++++++++---------------------------
> 2 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f0edee94834a..6003e96bce52 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -214,7 +214,18 @@ extern void scheduler_tick(void);
> 
> #define	MAX_SCHEDULE_TIMEOUT		LONG_MAX
> 
> -extern long schedule_timeout(long timeout);
> +extern long __schedule_timeout(long timeout);
> +
> +static inline long schedule_timeout(long timeout)
> +{
> +	if (__builtin_constant_p(timeout) && timeout == MAX_SCHEDULE_TIMEOUT) {
> +		schedule();
> +		return timeout;
> +	}
> +
> +	return __schedule_timeout(timeout);
> +}
> +
> extern long schedule_timeout_interruptible(long timeout);
> extern long schedule_timeout_killable(long timeout);
> extern long schedule_timeout_uninterruptible(long timeout);
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 0e315a2e77ae..912ae56b96b8 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1851,38 +1851,34 @@ static void process_timeout(struct timer_list *t)
>  * jiffies will be returned.  In all cases the return value is guaranteed
>  * to be non-negative.
>  */
> -signed long __sched schedule_timeout(signed long timeout)
> +signed long __sched __schedule_timeout(signed long timeout)
> {
> 	struct process_timer timer;
> 	unsigned long expire;
> 
> -	switch (timeout)
> -	{
> -	case MAX_SCHEDULE_TIMEOUT:
> -		/*
> -		 * These two special cases are useful to be comfortable
> -		 * in the caller. Nothing more. We could take
> -		 * MAX_SCHEDULE_TIMEOUT from one of the negative value
> -		 * but I' d like to return a valid offset (>=0) to allow
> -		 * the caller to do everything it want with the retval.
> -		 */
> +	/*
> +	 * We could take MAX_SCHEDULE_TIMEOUT from one of the negative values,
> +	 * but I'd like to return a valid offset (>= 0) to allow the caller to
> +	 * do everything it wants with the retval.
> +	 */
> +	if (timeout == MAX_SCHEDULE_TIMEOUT) {
> 		schedule();
> -		goto out;
Hi Mr Peter,
I have a suggestion here:
The condition timeout == MAX_SCHEDULE_TIMEOUT is already handled in
schedule_timeout function and  same conditon is handled again in __schedule_timeout.
Currently, no other function calls __schedule_timeout except schedule_timeout.
Therefore, it seems this condition will never become true.

This conditon will only be true when __schedule_timeout is called directly from kernel
with timeout = MAX_SCHEDULE_TIMEOUT. This case may be rare. Therefore, we can modify it as

if (unlikely(timeout == MAX_SCHEDULE_TIMEOUT))

Please let me know your comments.
Thanks
Satendra
> -	default:
> -		/*
> -		 * Another bit of PARANOID. Note that the retval will be
> -		 * 0 since no piece of kernel is supposed to do a check
> -		 * for a negative retval of schedule_timeout() (since it
> -		 * should never happens anyway). You just have the printk()
> -		 * that will tell you if something is gone wrong and where.
> -		 */
> -		if (timeout < 0) {
> -			printk(KERN_ERR "schedule_timeout: wrong timeout "
> +		return timeout;
> +	}
> +
> +	/*
> +	 * Another bit of PARANOID. Note that the retval will be 0 since no
> +	 * piece of kernel is supposed to do a check for a negative retval of
> +	 * schedule_timeout() (since it should never happens anyway). You just
> +	 * have the printk() that will tell you if something is gone wrong and
> +	 * where.
> +	 */
> +	if (timeout < 0) {
> +		printk(KERN_ERR "schedule_timeout: wrong timeout "
> 				"value %lx\n", timeout);
> -			dump_stack();
> -			current->state = TASK_RUNNING;
> -			goto out;
> -		}
> +		dump_stack();
> +		current->state = TASK_RUNNING;
> +		goto out;
> 	}
> 
> 	expire = timeout + jiffies;
> @@ -1898,10 +1894,10 @@ signed long __sched schedule_timeout(signed long timeout)
> 
> 	timeout = expire - jiffies;
> 
> - out:
> +out:
> 	return timeout < 0 ? 0 : timeout;
> }
> -EXPORT_SYMBOL(schedule_timeout);
> +EXPORT_SYMBOL(__schedule_timeout);
>
> /*
>  * We can use __set_current_state() here because schedule_timeout() calls

Re: [PATCH v1] [semaphore] Removed redundant code from semaphore's down family of function

[Peter Zijlstra]

On Fri, Aug 30, 2019 at 04:10:10PM +0530, Satendra Singh Thakur wrote:

> > +static inline long schedule_timeout(long timeout)
> > +{
> > +	if (__builtin_constant_p(timeout) && timeout == MAX_SCHEDULE_TIMEOUT) {
> > +		schedule();
> > +		return timeout;
> > +	}
> > +
> > +	return __schedule_timeout(timeout);
> > +}

> > +	if (timeout == MAX_SCHEDULE_TIMEOUT) {
> > 		schedule();
> > -		goto out;
> Hi Mr Peter,
> I have a suggestion here:
> The condition timeout == MAX_SCHEDULE_TIMEOUT is already handled in
> schedule_timeout function

Only if it is a compile time constant; otherwise it will end up here.

> and  same conditon is handled again in __schedule_timeout.
> Currently, no other function calls __schedule_timeout except schedule_timeout.
> Therefore, it seems this condition will never become true.

Please read more careful.