Received-SPF: pass (google.com: domain of linux-kernel+bounces-9010-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20231220001856.3710363-1-jstultz@google.com> <20231220001856.3710363-15-jstultz@google.com>
 <534c73b2-c28c-4e2a-9e6b-a5002c72afe8@arm.com>
In-Reply-To: <534c73b2-c28c-4e2a-9e6b-a5002c72afe8@arm.com>
From: John Stultz <jstultz@google.com>
Date: Thu, 21 Dec 2023 11:46:10 -0800
Message-ID: <CANDhNCp+FLdu2M+pi_E_LzFZnvqo8PLX2vk50jvp-CnMgS=7SQ@mail.gmail.com>
Subject: Re: [PATCH v7 14/23] sched: Handle blocked-waiter migration (and
 return migration)
To: Metin Kaya <metin.kaya@arm.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Joel Fernandes <joelaf@google.com>, 
	Qais Yousef <qyousef@google.com>, Ingo Molnar <mingo@redhat.com>, 
	Peter Zijlstra <peterz@infradead.org>, Juri Lelli <juri.lelli@redhat.com>, 
	Vincent Guittot <vincent.guittot@linaro.org>, Dietmar Eggemann <dietmar.eggemann@arm.com>, 
	Valentin Schneider <vschneid@redhat.com>, Steven Rostedt <rostedt@goodmis.org>, 
	Ben Segall <bsegall@google.com>, Zimuzo Ezeozue <zezeozue@google.com>, 
	Youssef Esmat <youssefesmat@google.com>, Mel Gorman <mgorman@suse.de>, 
	Daniel Bristot de Oliveira <bristot@redhat.com>, Will Deacon <will@kernel.org>, Waiman Long <longman@redhat.com>, 
	Boqun Feng <boqun.feng@gmail.com>, "Paul E. McKenney" <paulmck@kernel.org>, 
	Xuewen Yan <xuewen.yan94@gmail.com>, K Prateek Nayak <kprateek.nayak@amd.com>, 
	Thomas Gleixner <tglx@linutronix.de>, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Dec 21, 2023 at 8:13=E2=80=AFAM Metin Kaya <metin.kaya@arm.com> wro=
te:
> On 20/12/2023 12:18 am, John Stultz wrote:
> > Because tasks may get migrated to where they cannot run,
> > this patch also modifies the scheduling classes to avoid
> > sched class migrations on mutex blocked tasks, leaving
> > proxy() to do the migrations and return migrations.
>
> s/proxy/find_proxy_task/

Thanks, fixed.

> > +     if (p->blocked_on && p->blocked_on_state =3D=3D BO_WAKING) {
> > +             raw_spin_lock(&p->blocked_lock);
> > +             if (!is_cpu_allowed(p, cpu_of(rq))) {
> > +                     if (task_current_selected(rq, p)) {
> > +                             put_prev_task(rq, p);
> > +                             rq_set_selected(rq, rq->idle);
> > +                     }
> > +                     deactivate_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_NO=
CLOCK);
> > +                     resched_curr(rq);
> > +                     raw_spin_unlock(&p->blocked_lock);
> > +                     return true;
> > +             }
> > +             resched_curr(rq);
> > +             raw_spin_unlock(&p->blocked_lock);
>
> Do we need to hold blocked_lock while checking allowed CPUs? Would
> moving raw_spin_lock(&p->blocked_lock); inside if (!is_cpu_allowed())
> block be silly? i.e.,:

That's an interesting idea. I'll take a shot at reworking it. Thanks!

> Nit: what about this
> -#ifdef CONFIG_SMP
>   static inline bool proxy_needs_return(struct rq *rq, struct
> task_struct *p)
>   {
> +       if (!IS_ENABLED(CONFIG_SMP))
> +               return false;
> +

It would be nice, but the trouble is is_cpu_allowed() isn't defined
for !CONFIG_SMP, so that won't build.


> > + * Note: The owner can disappear, but simply migrate to @target_cpu
> > + * and leave that CPU to sort things out.
> > + */
> > +static struct task_struct *
>
> proxy_migrate_task() always returns NULL. Is return type really needed?

Good point. Reworked to clean that up.

> > +proxy_migrate_task(struct rq *rq, struct rq_flags *rf,
> > +                struct task_struct *p, int target_cpu)
> > +{
> > +     struct rq *target_rq;
> > +     int wake_cpu;
> > +
>
> Having a "if (!IS_ENABLED(CONFIG_SMP))" check here would help in
> dropping #else part. i.e.,

Sadly same problem as before, as wake_cpu isn't defined when !CONFIG_SMP :(

thanks again for the detailed review!
-john