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@xxxxxxxxx>
Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
---
 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