Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Oct 2022 18:00:30 +0000
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc:     Connor O'Brien <connoro@google.com>, linux-kernel@vger.kernel.org,
        kernel-team@android.com, John Stultz <jstultz@google.com>,
        Qais Yousef <qais.yousef@arm.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        Will Deacon <will@kernel.org>,
        Waiman Long <longman@redhat.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        "Paul E . McKenney" <paulmck@kernel.org>
Subject: Re: [RFC PATCH 07/11] sched: Add proxy execution
Message-ID: <Y2ANPi7y5HhHvelr@google.com>
References: <dab347c1-3724-8ac6-c051-9d2caea20101@arm.com>
 <34B2D8B9-A0C1-4280-944D-17224FB24339@joelfernandes.org>
 <4e396924-c3be-1932-91a3-5f458cc843fe@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4e396924-c3be-1932-91a3-5f458cc843fe@arm.com>
Precedence: bulk

On Mon, Oct 31, 2022 at 05:39:45PM +0100, Dietmar Eggemann wrote:
> On 29/10/2022 05:31, Joel Fernandes wrote:
> > Hello Dietmar,
> > 
> >> On Oct 24, 2022, at 6:13 AM, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> >>
> >> ﻿On 03/10/2022 23:44, Connor O'Brien wrote:
> >>> From: Peter Zijlstra <peterz@infradead.org>
> 
> [...]
> 
> >>> +    put_prev_task(rq, next);
> >>> +    if (curr_in_chain) {
> >>> +        rq->proxy = rq->idle;
> >>> +        set_tsk_need_resched(rq->idle);
> >>> +        /*
> >>> +         * XXX [juril] don't we still need to migrate @next to
> >>> +         * @owner's CPU?
> >>> +         */
> >>> +        return rq->idle;
> >>> +    }
> >>
> >> --> (1)
> > 
> > Sorry but what has this got to do with your comment below?
> 
> This was the place where fake_task was used in:
> 
> https://lkml.kernel.org/r/20181009092434.26221-6-juri.lelli@redhat.com
> 
> +migrate_task:
>    ...
> +	 }
> +	 rq->proxy = &fake_task;           <-- !!!
> +
> +	 for (; p; p = p->blocked_task) {
> 
> >>> +    rq->proxy = rq->idle;
> 
> We use `rq->idle` now,

I see. I need to research that, but a comment below:

> [...]
> 
> >>> +    rq_unpin_lock(rq, rf);
> >>> +    raw_spin_rq_unlock(rq);
> >>
> >> Don't we run into rq_pin_lock()'s:
> >>
> >> SCHED_WARN_ON(rq->balance_callback && rq->balance_callback !=
> >> &balance_push_callback)
> >>
> >> by releasing rq lock between queue_balance_callback(, push_rt/dl_tasks)
> >> and __balance_callbacks()?
> > 
> > Apologies, I’m a bit lost here. The code you are responding to inline does not call rq_pin_lock, it calls rq_unpin_lock.  So what scenario does the warning trigger according to you?
> 
> True, but the code which sneaks in between proxy()'s
> raw_spin_rq_unlock(rq) and raw_spin_rq_lock(rq) does.
> 

Got it now, thanks a lot for clarifying. Can this be fixed by do a
__balance_callbacks() at:

> __schedule()
> 
>   rq->proxy = next = pick_next_task()
> 
>     __pick_next_task()
> 
>       pick_next_task_rt()
> 
>         set_next_task_rt()
> 
>           rt_queue_push_tasks()
> 
>             queue_balance_callback(..., push_rt_tasks); <-- queue rt cb
> 
>   proxy()
> 

... here, before doing the following unlock?

>     raw_spin_rq_unlock(rq)
> 
>                  ... <-- other thread does rq_lock_XXX(rq)
>                                               raw_spin_rq_lock_XXX(rq)
>                                                 rq_pin_lock(rq)
> 
>     raw_spin_rq_lock(rq)
> 
>   context_switch()
> 
>      finish_task_switch()
> 
>        finish_lock_switch()
> 
>          __balance_callbacks(rq) <-- run rt cb here
> 
>   __balance_callbacks(rq)() <-- or run rt cb here


Hmm also Connor, does locktorture do hotplug? Maybe it should to reproduce
the balance issues.

thanks,

 - Joel