Re: [PATCH v2] sched/rt : return accurate release rq lock info

[Steven Rostedt]

On Wed, 3 Oct 2018 04:17:22 +0800
Peng Hao <peng.h...@zte.com.cn> wrote:

> find_lock_lowest_rq may or not releease rq lock, but it is fuzzy.
> If not releasing rq lock, it is unnecessary to re-call
> pick_next_pushable_task.
> When CONFIG_PREEMPT=n, not releasing rq lock frequently happens
> in a simple test case:
> Four different rt priority tasks run on limited two cpus.
> 
> Signed-off-by: Peng Hao <peng.h...@zte.com.cn>
> ---
>  kernel/sched/rt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 2e2955a..7c5382a 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1718,6 +1718,7 @@ static int find_lowest_rq(struct task_struct *task)
>  static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq 
> *rq)
>  {
>       struct rq *lowest_rq = NULL;
> +     bool release_lock = false;
>       int tries;
>       int cpu;
>  
> @@ -1741,6 +1742,7 @@ static struct rq *find_lock_lowest_rq(struct 
> task_struct *task, struct rq *rq)
>  
>               /* if the prio of this runqueue changed, try again */
>               if (double_lock_balance(rq, lowest_rq)) {
> +                     release_lock = true;
>                       /*
>                        * We had to unlock the run queue. In
>                        * the mean time, task could have
> @@ -1768,6 +1770,8 @@ static struct rq *find_lock_lowest_rq(struct 
> task_struct *task, struct rq *rq)
>               lowest_rq = NULL;
>       }
>  
> +     if (!lowest_rq && release_lock)
> +             lowest_rq = RETRY_TASK;
>       return lowest_rq;
>  }

Instead of adding the above boolean variable, wouldn't this work just
as well?

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 2e2955a8cf8f..b363ef70412a 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1754,7 +1754,7 @@ static struct rq *find_lock_lowest_rq(struct task_struct 
*task, struct rq *rq)
                                     !task_on_rq_queued(task))) {
 
                                double_unlock_balance(rq, lowest_rq);
-                               lowest_rq = NULL;
+                               lowest_rq = RETRY_TASK;
                                break;
                        }
                }

>  
> @@ -1830,7 +1834,7 @@ static int push_rt_task(struct rq *rq)
>  
>       /* find_lock_lowest_rq locks the rq if found */
>       lowest_rq = find_lock_lowest_rq(next_task, rq);
> -     if (!lowest_rq) {

Not a biggy, but I would add here:

        if (!lowest_rq)
                goto out;

And then add the below. It just flows better.

-- Steve

> +     if (lowest_rq == RETRY_TASK) {
>               struct task_struct *task;
>               /*
>                * find_lock_lowest_rq releases rq->lock
> @@ -1863,6 +1867,8 @@ static int push_rt_task(struct rq *rq)
>               goto retry;
>       }
>  
> +     if (!lowest_rq)
> +             goto out;
>       deactivate_task(rq, next_task, 0);
>       set_task_cpu(next_task, lowest_rq->cpu);
>       activate_task(lowest_rq, next_task, 0);