[PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[rodrigosiqueira]

The first parameter of prepare_lock_switch (kernel/sched/sched.h) is not
used anymore. Commit c55f5158f removed the code that use the first
parameter and function prepare_lock_switch is only used in
prepare_task_switch (kernel/sched/core.c)

I tested it in a virtual machine running Debian x86_64 with kvm. I
also tested it in my computer which is a x86_64 machine running Arch Linux
and everything is fine.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 kernel/sched/core.c  | 2 +-
 kernel/sched/sched.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c85dfb746f8c..0b1781cfa0ca 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2592,7 +2592,7 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev,
 	sched_info_switch(rq, prev, next);
 	perf_event_task_sched_out(prev, next);
 	fire_sched_out_preempt_notifiers(prev, next);
-	prepare_lock_switch(rq, next);
+	prepare_lock_switch(next);
 	prepare_arch_switch(next);
 }
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b19552a212de..089953e4ec66 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1328,7 +1328,7 @@ static inline int task_on_rq_migrating(struct task_struct *p)
 # define finish_arch_post_lock_switch()	do { } while (0)
 #endif
 
-static inline void prepare_lock_switch(struct rq *rq, struct task_struct *next)
+static inline void prepare_lock_switch(struct task_struct *next)
 {
 #ifdef CONFIG_SMP
 	/*
-- 
2.15.1

Re: [PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[Peter Zijlstra]

On Tue, Dec 05, 2017 at 12:02:00PM -0200, rodrigosiqueira wrote:
> The first parameter of prepare_lock_switch (kernel/sched/sched.h) is not
> used anymore. Commit c55f5158f removed the code that use the first
> parameter and function prepare_lock_switch is only used in
> prepare_task_switch (kernel/sched/core.c)

Yes, this is correct. However it had me looking at that code and pretty
much everything else is completely wrong :-)

That is, its functionally correct (probably), but the function name is
not descriptive of what the function does and the comment is just plain
wrong.

Also, since both functions are only used in core.c we should probably
move them there.

Do you think you can fix all that as well?

Re: [PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[Rodrigo Siqueira]

> Yes, this is correct. However it had me looking at that code and pretty
> much everything else is completely wrong :-)
> 
> That is, its functionally correct (probably), but the function name is
> not descriptive of what the function does and the comment is just plain
> wrong.
> 
> Also, since both functions are only used in core.c we should probably
> move them there.

I'm not sure I understood it completely. What do you mean for wrong? Will 
CONFIG_SMP a meaningless check here?

How about moving 'prepare_lock_switch' code from sched.h to prepare_task_switch
in core.c?

And about the comment in 'prepare_lock_switch', I can replace it to
"Set on_cpu to 1 during the context switch will lock the processes on the cpu"
 
> Do you think you can fix all that as well?

Yeah absolutely, I just might need a few more comprehension on it.

Re: [PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[Peter Zijlstra]

On Wed, Dec 06, 2017 at 09:50:19PM -0200, Rodrigo Siqueira wrote:
> > Yes, this is correct. However it had me looking at that code and pretty
> > much everything else is completely wrong :-)
> > 
> > That is, its functionally correct (probably), but the function name is
> > not descriptive of what the function does and the comment is just plain
> > wrong.
> > 
> > Also, since both functions are only used in core.c we should probably
> > move them there.
> 
> I'm not sure I understood it completely. What do you mean for wrong? Will 
> CONFIG_SMP a meaningless check here?

So the actual effective code is ok; including the #ifdef for SMP. But
the comment is complete nonsense.

Look at the comments:

 - in finish_lock_switch() doing smp_store_release()
 - before try_to_wake_up() describing migration/blocking
 - in try_to_wake_up() doing smp_cond_load_acquire().

To get a feeling for what on_cpu actually does; it doesn't have anything
much to do with SMP rebalancing code from interrupt contexts (although
that too still cares through can_migrate_task() <- task_running()).

> How about moving 'prepare_lock_switch' code from sched.h to prepare_task_switch
> in core.c?

With a rename; yes. Maybe something like 'acquire_task()' would do.

Then split the smp_store_release() out from finish_lock_switch() and
call it release_task(), and place is near the new acquire_task()
function -- don't forget to update all comments referring to
finish_lock_switch().

This then leaves the actual rq->lock fiddling in finish_lock_switch();
and that whole function too can be moved to core.c, somewhere near
finish_task_switch() I think.

Re: [PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[Rodrigo Siqueira]

> On Wed, Dec 06, 2017 at 09:50:19PM -0200, Rodrigo Siqueira wrote:
> > > Yes, this is correct. However it had me looking at that code and pretty
> > > much everything else is completely wrong :-)
> > > 
> > > That is, its functionally correct (probably), but the function name is
> > > not descriptive of what the function does and the comment is just plain
> > > wrong.
> > > 
> > > Also, since both functions are only used in core.c we should probably
> > > move them there.
> > 
> > I'm not sure I understood it completely. What do you mean for wrong? Will 
> > CONFIG_SMP a meaningless check here?
> 
> So the actual effective code is ok; including the #ifdef for SMP. But
> the comment is complete nonsense.
> 
> Look at the comments:
> 
>  - in finish_lock_switch() doing smp_store_release()
>  - before try_to_wake_up() describing migration/blocking
>  - in try_to_wake_up() doing smp_cond_load_acquire().
> 
> To get a feeling for what on_cpu actually does; it doesn't have anything
> much to do with SMP rebalancing code from interrupt contexts (although
> that too still cares through can_migrate_task() <- task_running()).
> 
> > How about moving 'prepare_lock_switch' code from sched.h to prepare_task_switch
> > in core.c?
> 
> With a rename; yes. Maybe something like 'acquire_task()' would do.
> 
> Then split the smp_store_release() out from finish_lock_switch() and
> call it release_task(), and place is near the new acquire_task()
> function -- don't forget to update all comments referring to
> finish_lock_switch().
> 
> This then leaves the actual rq->lock fiddling in finish_lock_switch();
> and that whole function too can be moved to core.c, somewhere near
> finish_task_switch() I think.

Got it! I am working on it.

Re: [PATCH] Scheduler: Removed first parameter from prepare_lock_switch

[Rodrigo Siqueira]

Peter,

I tried to understand and applied all your suggestions, follows the
modification:

Adjustments: lock/unlock task in context_switch

Function prepare_lock_switch have an unused parameter, and also the
function name was not descriptive. To improve the readability and remove
the extra parameter, the following changes were made:

* Moved prepare_lock_switch from kernel/sched/sched.h to
  kernel/sched/core.c, renamed it to acquire_lock_task, and removed the
  unused parameter.

* Split the smp_store_release() out from finish_lock_switch() to a
  function named release_lock_task.

* Comments ajdustments.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 kernel/sched/core.c  | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----
 kernel/sched/sched.h | 41 -----------------------------------------
 2 files changed, 48 insertions(+), 45 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 75554f366fd3..5b36a2d2b1ee 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2045,7 +2045,7 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
 	 * If the owning (remote) CPU is still in the middle of schedule() with
 	 * this task as prev, wait until its done referencing the task.
 	 *
-	 * Pairs with the smp_store_release() in finish_lock_switch().
+	 * Pairs with the smp_store_release() in release_lock_task().
 	 *
 	 * This ensures that tasks getting woken will be fully ordered against
 	 * their previous state and preserve Program Order.
@@ -2571,6 +2571,49 @@ fire_sched_out_preempt_notifiers(struct task_struct *curr,
 
 #endif /* CONFIG_PREEMPT_NOTIFIERS */
 
+static inline void acquire_lock_task(struct task_struct *next)
+{
+#ifdef CONFIG_SMP
+	/*
+	 * Avoid task to be moved to a different CPU
+	 */
+	next->on_cpu = 1;
+#endif
+}
+
+static inline void release_lock_task(struct task_struct *prev)
+{
+#ifdef CONFIG_SMP
+	/*
+	 * After ->on_cpu is cleared, the task can be moved to a different CPU.
+	 * We must ensure this doesn't happen until the switch is completely
+	 * finished.
+	 *
+	 * In particular, the load of prev->state in finish_task_switch() must
+	 * happen before this.
+	 *
+	 * Pairs with the smp_cond_load_acquire() in try_to_wake_up().
+	 */
+	smp_store_release(&prev->on_cpu, 0);
+#endif
+}
+
+static inline void finish_lock_switch(struct rq *rq)
+{
+#ifdef CONFIG_DEBUG_SPINLOCK
+	/* this is a valid case when another task releases the spinlock */
+	rq->lock.owner = current;
+#endif
+	/*
+	 * If we are tracking spinlock dependencies then we have to
+	 * fix up the runqueue lock - which gets 'carried over' from
+	 * prev into current:
+	 */
+	spin_acquire(&rq->lock.dep_map, 0, 0, _THIS_IP_);
+
+	raw_spin_unlock_irq(&rq->lock);
+}
+
 /**
  * prepare_task_switch - prepare to switch tasks
  * @rq: the runqueue preparing to switch
@@ -2591,7 +2634,7 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev,
 	sched_info_switch(rq, prev, next);
 	perf_event_task_sched_out(prev, next);
 	fire_sched_out_preempt_notifiers(prev, next);
-	prepare_lock_switch(rq, next);
+	acquire_lock_task(next);
 	prepare_arch_switch(next);
 }
 
@@ -2646,7 +2689,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	 * the scheduled task must drop that reference.
 	 *
 	 * We must observe prev->state before clearing prev->on_cpu (in
-	 * finish_lock_switch), otherwise a concurrent wakeup can get prev
+	 * release_lock_task), otherwise a concurrent wakeup can get prev
 	 * running on another CPU and we could rave with its RUNNING -> DEAD
 	 * transition, resulting in a double drop.
 	 */
@@ -2663,7 +2706,8 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	 * to use.
 	 */
 	smp_mb__after_unlock_lock();
-	finish_lock_switch(rq, prev);
+	release_lock_task(prev);
+	finish_lock_switch(rq);
 	finish_arch_post_lock_switch();
 
 	fire_sched_in_preempt_notifiers(current);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b19552a212de..43f5d6e936bb 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1328,47 +1328,6 @@ static inline int task_on_rq_migrating(struct task_struct *p)
 # define finish_arch_post_lock_switch()	do { } while (0)
 #endif
 
-static inline void prepare_lock_switch(struct rq *rq, struct task_struct *next)
-{
-#ifdef CONFIG_SMP
-	/*
-	 * We can optimise this out completely for !SMP, because the
-	 * SMP rebalancing from interrupt is the only thing that cares
-	 * here.
-	 */
-	next->on_cpu = 1;
-#endif
-}
-
-static inline void finish_lock_switch(struct rq *rq, struct task_struct *prev)
-{
-#ifdef CONFIG_SMP
-	/*
-	 * After ->on_cpu is cleared, the task can be moved to a different CPU.
-	 * We must ensure this doesn't happen until the switch is completely
-	 * finished.
-	 *
-	 * In particular, the load of prev->state in finish_task_switch() must
-	 * happen before this.
-	 *
-	 * Pairs with the smp_cond_load_acquire() in try_to_wake_up().
-	 */
-	smp_store_release(&prev->on_cpu, 0);
-#endif
-#ifdef CONFIG_DEBUG_SPINLOCK
-	/* this is a valid case when another task releases the spinlock */
-	rq->lock.owner = current;
-#endif
-	/*
-	 * If we are tracking spinlock dependencies then we have to
-	 * fix up the runqueue lock - which gets 'carried over' from
-	 * prev into current:
-	 */
-	spin_acquire(&rq->lock.dep_map, 0, 0, _THIS_IP_);
-
-	raw_spin_unlock_irq(&rq->lock);
-}
-
 /*
  * wake flags
  */
-- 
2.15.1