linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Qais Yousef <qyousef@layalina.io>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	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>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	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>,
	youssefesmat@google.com
Subject: Re: [RFC PATCH 07/11] sched: Add proxy execution
Date: Tue, 25 Oct 2022 23:10:59 +0100	[thread overview]
Message-ID: <20221025221059.2drigl2gcx5sm4ji@airbuntu> (raw)
In-Reply-To: <7BBED338-D158-401B-8A6B-FDBD9FC03973@joelfernandes.org>

On 10/25/22 07:19, Joel Fernandes wrote:
> 
> 
> > On Oct 24, 2022, at 6:33 PM, Qais Yousef <qyousef@layalina.io> wrote:
> > 
> > On 10/17/22 09:26, Peter Zijlstra wrote:
> > 
> >> Additionally, the highest priotiy waiter will get the lock next.
> > 
> > True for RT. But for CFS, priority is share and there will be no guarantee
> > the 'highest priority' task will run as soon as the lock is released to
> > grab it, no?
> 
> But the mutex lock owner should have done a wake_up in the mutex unlock path,

I admit I'm a bit speculating in terms of how mutexes/futexes will behave.

I thought we could still end up with a situation where all (or some) waiters
could wake up and race to hold the lock. Which now thinking about it is bad
news for RT on SMP systems since lower priority tasks on other CPUs could get
the lock before higher priority ones on another CPU. So maybe this situation
can never happen..

> which is arranged in FIFO order, if I am not mistaken. Subsequently the
> scheduler will at least get a chance to see if the thing that is waiting for
> the lock is of higher priority, at the next preemption point.

Right. If it's always ticketed, then we guarantee the order of who holds the
lock first. But the order of which the lock is held/requested/locked is not the
same as priority. And I am not sure if all variations of locks/configs will
exhibit the same behavior TBH.

Again I haven't looked closely at the patches/code, but I only seen the notion
of priority in rt_mutex; probably something similar was done in the PE patches.

__waiter_prio() in rt_mutex.c returns DEFAULT_PRIO for !rt_prio(). So it seems
for me for CFS the prio will not influence the top_waiter and they will be just
queued at the bottom in-order. For rt_mutex that is; which apparently is
obsolete from PE perspective.

I probably have speculated based on that rt_mutex behavior. Apologies if
I jumped guns.

> If it did not get to run, I don’t think that’s an issue — it was not highest
> priority as far as the scheduler is concerned. No?

For CFS priority === share; but maybe you refer to some implementation detail
I missed here. IOW, for CFS highest priority is the most starved task, which is
influenced by nice, but doesn't guarantee who gets to run next?

> Steve was teaching me some of this code recently, he could chime in :)

Would be good for the rest of us to get enlightened too :-)

> 
> > For example I can envisage:
> > 
> >    +--------+----------------+--------+-------- |  p0    |       p1
> >    |   p0   |   p1 +--------+----------------+--------+--------
> >    ^  ^                 ^      ^ ^ |  |                 |      | | |  |                 |      | Fails
> >    to hold the lock holds lock        releases lock | and proxy execs for
> >    p0 again |                        | |                        | tries to
> >    hold lock         holds lock again proxy execs for p0
> > 
> > The notion of priority in CFS as it stands doesn't help in providing any
> > guarantees in who will be able to hold the lock next. I haven't looked at
> > the patches closely, so this might be handled already. I think the
> > situation will be worse if there're more tasks contending for the lock.
> > Priority will influences the chances, but the end result who holds the lock
> > next is effectively random, AFAICT.
> 
> The wake up during unlock is FIFO order of waiters though. So that’s
> deterministic.

Deterministic in respecting the order the lock was held. But not deterministic
in terms of the priority of the tasks in that order. IOW, that order is based
on time rather than priority.

Say we have 4 CFS tasks, 3 are priority 0 and one is priority -19. If the
3 tasks @prio_0 try to hold the lock before the one @prio_-19; then the FIFO
order means the higher priority one will be the last to get the lock. And the
other tasks - depending how long they hold the lock for - could cause the
@prio_-19 task to run as a proxy for multiple slices first. Which is the
potential problem I was trying to highlight. The @prio_-19 task will not
appear to have been starved for multiple slices since it runs; but as
a proxy.

Again, maybe there's a devilish detail in the patches that addresses that and
it's not really a problem.

> 
> > I had a conversation once with an app developer who came from iOS world and
> > they were confused why their higher priority task is not preempting the
> > lower priority one when they ported it to Android.
> > 
> > I wonder sometimes if we need to introduce a true notion of priority for
> > CFS.  I don't see why an app developer who would like to create 3 tasks and
> > give them strict priority order relative to each others can't do that. At
> > the moment they have little option in controlling execution order.
> 
> I want to talk more about this with you, I am actually working on something
> similar. Let’s talk ;)

Oh. Yes, let us talk! :-)


Cheers

--
Qais Yousef

  reply	other threads:[~2022-10-25 22:11 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-03 21:44 [RFC PATCH 00/11] Reviving the Proxy Execution Series Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 01/11] locking/ww_mutex: Remove wakeups from under mutex::wait_lock Connor O'Brien
2022-10-04 16:01   ` Waiman Long
2022-10-12 23:54     ` Joel Fernandes
2022-10-20 18:43     ` Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 02/11] locking/mutex: Rework task_struct::blocked_on Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 03/11] kernel/locking: Add p->blocked_on wrapper Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 04/11] locking/mutex: make mutex::wait_lock irq safe Connor O'Brien
2022-10-13  4:30   ` Joel Fernandes
2022-10-03 21:44 ` [RFC PATCH 05/11] sched: Split scheduler execution context Connor O'Brien
2022-10-14 17:01   ` Joel Fernandes
2022-10-19 17:17   ` Valentin Schneider
2022-10-20 18:43     ` Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 06/11] kernel/locking: Expose mutex_owner() Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 07/11] sched: Add proxy execution Connor O'Brien
2022-10-12  1:54   ` Joel Fernandes
2022-10-12  9:46     ` Juri Lelli
2022-10-14 17:07     ` Joel Fernandes
2022-10-15 13:53     ` Peter Zijlstra
2022-10-16 20:48       ` Steven Rostedt
2022-10-17  4:03         ` Joel Fernandes
2022-10-17  7:26         ` Peter Zijlstra
2022-10-24 22:33           ` Qais Yousef
2022-10-25 11:19             ` Joel Fernandes
2022-10-25 22:10               ` Qais Yousef [this message]
2022-10-15 15:28     ` Peter Zijlstra
2022-10-15 15:08   ` Peter Zijlstra
2022-10-15 15:10   ` Peter Zijlstra
2022-10-15 15:47   ` Peter Zijlstra
2022-10-24 10:13   ` Dietmar Eggemann
2022-10-29  3:31     ` Joel Fernandes
2022-10-31 16:39       ` Dietmar Eggemann
2022-10-31 18:00         ` Joel Fernandes
2022-11-04 17:09           ` Dietmar Eggemann
2022-11-21  0:22             ` Joel Fernandes
2022-11-21  1:49               ` Joel Fernandes
2022-11-21  3:59                 ` Joel Fernandes
2022-11-22 18:45                   ` Joel Fernandes
2023-01-09  8:51   ` Chen Yu
2022-10-03 21:44 ` [RFC PATCH 08/11] sched: Fixup task CPUs for potential proxies Connor O'Brien
2022-10-03 21:44 ` [RFC PATCH 09/11] sched/rt: Fix proxy/current (push,pull)ability Connor O'Brien
2022-10-10 11:40   ` Valentin Schneider
2022-10-14 22:32     ` Connor O'Brien
2022-10-19 17:05       ` Valentin Schneider
2022-10-20 13:30         ` Juri Lelli
2022-10-20 16:14           ` Valentin Schneider
2022-10-21  2:22         ` Connor O'Brien
2022-10-03 21:45 ` [RFC PATCH 10/11] torture: support randomized shuffling for proxy exec testing Connor O'Brien
2022-11-12 16:54   ` Joel Fernandes
2022-11-14 20:44     ` Connor O'Brien
2022-11-15 16:02       ` Joel Fernandes
2022-10-03 21:45 ` [RFC PATCH 11/11] locktorture: support nested mutexes Connor O'Brien
2022-10-06  9:59 ` [RFC PATCH 00/11] Reviving the Proxy Execution Series Juri Lelli
2022-10-06 10:07   ` Peter Zijlstra
2022-10-06 12:14     ` Juri Lelli
2022-10-15 15:44 ` Peter Zijlstra
2022-10-17  2:23 ` Joel Fernandes
2022-10-19 11:43   ` Qais Yousef
2022-10-19 12:23     ` Joel Fernandes
2022-10-19 13:41       ` Juri Lelli
2022-10-19 13:51         ` Joel Fernandes
2022-10-19 19:30         ` Qais Yousef
2022-10-20  8:51           ` Joel Fernandes
2022-10-17  3:25 ` Chengming Zhou
2022-10-17  3:56   ` Joel Fernandes
2022-10-17  4:26     ` Chengming Zhou
2022-10-17 12:27       ` Joel Fernandes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221025221059.2drigl2gcx5sm4ji@airbuntu \
    --to=qyousef@layalina.io \
    --cc=boqun.feng@gmail.com \
    --cc=bristot@redhat.com \
    --cc=bsegall@google.com \
    --cc=connoro@google.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=joel@joelfernandes.org \
    --cc=jstultz@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=qais.yousef@arm.com \
    --cc=rostedt@goodmis.org \
    --cc=vincent.guittot@linaro.org \
    --cc=vschneid@redhat.com \
    --cc=will@kernel.org \
    --cc=youssefesmat@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).