lkml.org 
[lkml]   [2024]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v9 0/7] Preparatory changes for Proxy Execution v9
From
On 15/03/2024 4:39 am, John Stultz wrote:
> As mentioned last time[1], after previous submissions of the
> Proxy Execution series, I got feedback that the patch series was
> getting a bit unwieldy to review, and Qais suggested I break out
> just the cleanups/preparatory components of the patch series and
> submit them on their own in the hope we can start to merge the
> less complex bits and discussion can focus on the more
> complicated portions afterwards. This so far has not been very
> successful, with the submission & RESEND of the v8 preparatory
> changes not getting much in the way of review.
>
> Nonetheless, for v9 of this series, I’m again only submitting
> those early cleanup/preparatory changes here (which have not
> changed since the v8 submissions, but to avoid confusion with the
> git branch names, I’m labeling it as v9). In the meantime, I’ve
> continued to put a lot of effort into the full series, mostly
> focused on polishing the series for correctness, and fixing some
> hard to trip races.
>
> If you are interested, the full v9 series, it can be found here:
> https://github.com/johnstultz-work/linux-dev/commits/proxy-exec-v9-6.8
> https://github.com/johnstultz-work/linux-dev.git proxy-exec-v9-6.8
>
>
> New in v9:
> (In the git tree. Again, none of the preparatory patches
> submitted here have changed since v8)
> ---------
> * Change to force mutex lock handoff when we have a blocked donor
> (preserves optimistic spinning elsewhere, but still prioritizes
> donor when present on unlock)
>
> * Do return migration whenever we’re not on the wake_cpu (should
> address placement concerns brought up earlier by Xuewen Yan)
>
> * Closed hole where we might mark a task as BO_RUNNABLE without
> doing return migration
>
> * Much improved handling of balance callbacks when we need to
> pick_again
>
> * Fixes for cases where we put_prev_task() but left a dangling
> pointer to rq_selected() when deactivating a task (as it could
> then be migrated away while we still have a reference to it),
> by selecting idle before deactivating next.
>
> * Fixes for dangling references to rq->curr (which had been
> put_prev_task’ed) when we drop rq lock for proxy_migration
>
> * Fixes for ttwu / find_proxy_task() races if the lock owner was
> being return migrated, and ttwu hadn’t yet set_task_cpu() and
> activated it, which allowed that task to be scheduled on two
> cpus at the same time.
>
> * Fix for live-lock between activate_blocked_tasks() and
> proxy_enqueue_on_owner() if activated owner went right back to
> sleep (which also simplifies the locking in
> activate_blocked_tasks())
>
> * Cleanups to avoid locked BO_WAKING->BO_RUNNABLE transition in
> try_to_wake_up() if proxy execution isn't enabled
>
> * Fix for psi_dequeue, as proxy changes assumptions around
> voluntary sleeps.
>
> * Numerous typos, comment improvements, and other fixups
> suggested by Metin
>
> * And more!
>
>
> Performance:
> ---------
> K Prateek Nayak provided some feedback on the v8 series here[2].
> Given the potential extra overhead of doing rq migrations/return
> migrations/etc for the proxy case, it’s not completely surprising
> a few of K Prateek’s test cases saw ~3-5% regressions, but I’m
> hoping to look into this soon to see if we can reduce those
> further. The donor mutex handoff in this revision may help some.
>
>
> Issues still to address:
> ---------
> * The chain migration functionality needs further iterations and
> better validation to ensure it truly maintains the RT/DL load
> balancing invariants.
>
> * CFS load balancing. There was concern that blocked tasks may
> carry forward load (PELT) to the lock owner's CPU, so the CPU
> may look like it is overloaded. Needs investigation.
>
> * The sleeping owner handling (where we deactivate waiting tasks
> and enqueue them onto a list, then reactivate them when the
> owner wakes up) doesn’t feel great. This is in part because
> when we want to activate tasks, we’re already holding a
> task.pi_lock and a rq_lock, just not the locks for the task
> we’re activating, nor the rq we’re enqueuing it onto. So there
> has to be a bit of lock juggling to drop and acquire the right
> locks (in the right order). It feels like there’s got to be a
> better way. Also needs some rework to get rid of the recursion.
>
>
> Credit/Disclaimer:
> —--------------------
> As mentioned previously, this Proxy Execution series has a long
> history: First described in a paper[3] by Watkins, Straub,
> Niehaus, then from patches from Peter Zijlstra, extended with
> lots of work by Juri Lelli, Valentin Schneider, and Connor
> O'Brien. (and thank you to Steven Rostedt for providing
> additional details here!)
>
> So again, many thanks to those above, as all the credit for this
> series really is due to them - while the mistakes are likely
> mine.
>
> Thanks so much!
> -john
>
> [1] https://lore.kernel.org/lkml/20240224001153.2584030-1-jstultz@google.com/
> [2] https://lore.kernel.org/lkml/c26251d2-e1bf-e5c7-0636-12ad886e1ea8@amd.com/
> [3] https://static.lwn.net/images/conf/rtlws11/papers/proc/p38.pdf
>
> Cc: Joel Fernandes <joelaf@google.com>
> Cc: Qais Yousef <qyousef@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Valentin Schneider <vschneid@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ben Segall <bsegall@google.com>
> Cc: Zimuzo Ezeozue <zezeozue@google.com>
> Cc: Youssef Esmat <youssefesmat@google.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Metin Kaya <Metin.Kaya@arm.com>
> Cc: Xuewen Yan <xuewen.yan94@gmail.com>
> Cc: K Prateek Nayak <kprateek.nayak@amd.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: kernel-team@android.com
>
>
> Connor O'Brien (2):
> sched: Add do_push_task helper
> sched: Consolidate pick_*_task to task_is_pushable helper
>
> John Stultz (1):
> sched: Split out __schedule() deactivate task logic into a helper
>
> Juri Lelli (2):
> locking/mutex: Make mutex::wait_lock irq safe
> locking/mutex: Expose __mutex_owner()
>
> Peter Zijlstra (2):
> locking/mutex: Remove wakeups from under mutex::wait_lock
> sched: Split scheduler and execution contexts
>
> kernel/locking/mutex.c | 60 +++++++----------
> kernel/locking/mutex.h | 25 +++++++
> kernel/locking/rtmutex.c | 26 +++++---
> kernel/locking/rwbase_rt.c | 4 +-
> kernel/locking/rwsem.c | 4 +-
> kernel/locking/spinlock_rt.c | 3 +-
> kernel/locking/ww_mutex.h | 49 ++++++++------
> kernel/sched/core.c | 122 +++++++++++++++++++++--------------
> kernel/sched/deadline.c | 53 ++++++---------
> kernel/sched/fair.c | 18 +++---
> kernel/sched/rt.c | 59 +++++++----------
> kernel/sched/sched.h | 44 ++++++++++++-
> 12 files changed, 268 insertions(+), 199 deletions(-)
>

I could cleanly apply this patch series to `linux-next/next-20240318`
branch.

Here is checkpatch report:
```
$ ./scripts/checkpatch.pl -g 6aa2a9e7aea6d..HEAD



---------------------------------------------------------------------



Commit a9124906b27f ("locking/mutex: Make mutex::wait_lock irq safe")



---------------------------------------------------------------------
total: 0 errors, 0 warnings, 128 lines checked

Commit a9124906b27f ("locking/mutex: Make mutex::wait_lock irq safe")
has no obvious style problems and is ready for submission.
-------------------------------------------------------------
Commit c3b81eb4858a ("locking/mutex: Expose __mutex_owner()")
-------------------------------------------------------------
WARNING: Possible repeated word: 'at'
#91: FILE: kernel/locking/mutex.h:26:
+ * NULL means not owned. Since task_struct pointers are aligned at
+ * at least L1_CACHE_BYTES, we have low bits to store extra state.

total: 0 errors, 1 warnings, 62 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.

Commit c3b81eb4858a ("locking/mutex: Expose __mutex_owner()") has style
problems, please review.
------------------------------------------------------
Commit 6adb713bd389 ("sched: Add do_push_task helper")
------------------------------------------------------
total: 0 errors, 0 warnings, 64 lines checked

Commit 6adb713bd389 ("sched: Add do_push_task helper") has no obvious
style problems and is ready for submission.
---------------------------------------------------------------------------------
Commit 037345d6896a ("sched: Consolidate pick_*_task to task_is_pushable
helper")
---------------------------------------------------------------------------------
total: 0 errors, 0 warnings, 61 lines checked

Commit 037345d6896a ("sched: Consolidate pick_*_task to task_is_pushable
helper") has no obvious style problems and is ready for submission.
-----------------------------------------------------------------------------------------
Commit 6c9aa2fd15de ("sched: Split out __schedule() deactivate task
logic into a helper")
-----------------------------------------------------------------------------------------
total: 0 errors, 0 warnings, 84 lines checked

Commit 6c9aa2fd15de ("sched: Split out __schedule() deactivate task
logic into a helper") has no obvious style problems and is ready for
submission.
---------------------------------------------------------------------
Commit 6af85d44be35 ("sched: Split scheduler and execution contexts")
---------------------------------------------------------------------
WARNING: Duplicate signature
#46:
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

WARNING: Do not crash the kernel unless it is absolutely
unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead
of BUG() or variants
#557: FILE: kernel/sched/rt.c:1979:
+ BUG_ON(task_current_selected(rq, p));

total: 0 errors, 2 warnings, 539 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.

Commit 6af85d44be35 ("sched: Split scheduler and execution contexts")
has style problems, please review.

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
```

I think only the duplicate signature warning is a real problem and will
reply to the offending mail.

Issued `make defconfig` on a x86_64 machine, enabled lock & RCU
debugging config options and built the kernel.

Then, ran commands below on a QEMU Debian guest (arch=x86_64, cores=32,
threads=2, memory=2GiB) before and after this patch series:
```
$ insmod torture.ko && insmod scftorture.ko verbose=1 onoff_holdoff=5
onoff_interval=1; sleep 60; rmmod scftorture torture
$ insmod torture.ko random_shuffle=1 && insmod locktorture.ko
torture_type=mutex_lock rt_boost=1 rt_boost_factor=50 nested_locks=3
writer_fifo=1; sleep 60; rmmod locktorture torture
$ taskset -c 0-31,32-63 perf bench sched messaging -p -t -l 100000 -g 2
$ perf bench sched pipe -l 100000 -T
```

Logs before this patch series:
```
$ insmod torture.ko && insmod scftorture.ko verbose=1 onoff_holdoff=5
onoff_interval=1; sleep 60; rmmod scftorture torture
[ 35.912508] torture module --- scftorture: disable_onoff_at_boot=0
ftrace_dump_at_shutdown=0 verbose_sleep_frequency=0
verbose_sleep_duration=1 random_shuffle=0
[ 35.914627] scftorture: --- Start of test: verbose=1 holdoff=0
longwait=0 nthreads=-1 onoff_holdoff=5 onoff_interval=1 shutdown_secs=0
stat_interval=60 stutter=5 use_cpus_read_lock=0, weight_resched=-1,
weight_single=-1, weight_single_rpc=-1, weight_single_wait=-1, weight_many1
[ 35.918824] scftorture: !!! built as module, weight_resched ignored
[ 35.919842] scf_sel_dump: 32.820 smp_call_function_single(nowait)
[ 35.921035] scf_sel_dump: 32.820 smp_call_function_single_rpc(wait)
[ 35.922183] scf_sel_dump: 32.820 smp_call_function_single(wait)
[ 35.923334] scf_sel_dump: 0.512 smp_call_function_many(nowait)
[ 35.924372] scf_sel_dump: 0.512 smp_call_function_many(wait)
[ 35.925410] scf_sel_dump: 0.256 smp_call_function(nowait)
[ 35.926398] scf_sel_dump: 0.256 smp_call_function(wait)
[ 35.927373] scftorture-torture: Creating torture_onoff task
[ 35.928665] scftorture-torture: torture_onoff task started
[ 35.928677] scftorture-torture: Creating torture_stutter task
[ 35.929696] scftorture-torture: torture_onoff begin holdoff
[ 35.930902] scftorture: Starting 64 smp_call_function() threads
[ 35.930904] scftorture-torture: torture_stutter task started
[ 35.933755] scftorture-torture: Creating scftorture_invoker task
[ 35.934966] scftorture-torture: Creating scftorture_invoker task
[ 35.934980] scftorture: scftorture_invoker 0: task started
..
[ 99.689799] scftorture: scf_invoked_count VER: 87177502 resched: 0
single: 28726722/28492519 single_ofl: 13877596/13766523 single_rpc:
28492768 single_rpc_ofl: 0 many: 445547/446215 all: 221861/223143 onoff:
1527/1527:1559/1559 14,74:8,642 23749:29438 (HZ=1000) ste: 0 stnmie: 0
[ 99.694023] scftorture: --- End of test: SUCCESS: verbose=1
holdoff=0 longwait=0 nthreads=64 onoff_holdoff=5 onoff_interval=1
shutdown_secs=0 stat_interval=60 stutter=5 use_cpus_read_lock=0,
weight_resched=-1, weight_single=-1, weight_single_rpc=-1,
weight_single_wait=-1, weig1

$ insmod torture.ko random_shuffle=1 && insmod locktorture.ko
torture_type=mutex_lock rt_boost=1 rt_boost_factor=50 nested_locks=3
writer_fifo=1; sleep 60; rmmod locktorture torture
[ 114.543156] torture module --- mutex_lock: disable_onoff_at_boot=0
ftrace_dump_at_shutdown=0 verbose_sleep_frequency=0
verbose_sleep_duration=1 random_shuffle=1
[ 114.545291] mutex_lock-torture:--- Start of test [debug]:
acq_writer_lim=0 bind_readers=0-63 bind_writers=0-63 call_rcu_chains=0
long_hold=100 nested_locks=3 nreaders_stress=0 nwriters_stress=128
onoff_holdoff=0 onoff_interval=0 rt_boost=1 rt_boost_factor=50
shuffle_interval=3 1
[ 114.549640] mutex_lock-torture: Creating torture_shuffle task
[ 114.550880] mutex_lock-torture: Creating torture_stutter task
[ 114.550905] mutex_lock-torture: torture_shuffle task started
[ 114.552006] mutex_lock-torture: Creating lock_torture_writer task
[ 114.552021] mutex_lock-torture: torture_stutter task started
..
[ 177.883162] mutex_lock-torture: Stopping lock_torture_stats task
[ 177.884249] Writes: Total: 72982638 Max/Min: 748221/404806 Fail: 0
[ 177.885483] mutex_lock-torture: lock_torture_stats is stopping
[ 177.886516] Writes: Total: 72982638 Max/Min: 748221/404806 Fail: 0
[ 177.887630] mutex_lock-torture:--- End of test: SUCCESS [debug]:
acq_writer_lim=0 bind_readers=0-63 bind_writers=0-63 call_rcu_chains=0
long_hold=100 nested_locks=3 nreaders_stress=0 nwriters_stress=128
onoff_holdoff=0 onoff_interval=0 rt_boost=1 rt_boost_factor=50 shuffle_inte1

$ taskset -c 0-31,32-63 perf bench sched messaging -p -t -l 100000 -g 2
# Running 'sched/messaging' benchmark:0-31,32-63 perf bench sched
messaging -p -t -l 100000 -g 2
# 20 sender and receiver threads per group
# 2 groups == 80 threads run

Total time: 13.835 [sec]

$ perf bench sched pipe -l 100000 -T
# Running 'sched/pipe' benchmark:
# Executed 100000 pipe operations between two threads

Total time: 2.772 [sec]

27.725990 usecs/op
36067 ops/sec
```

Logs after applying this patch series:
```
$ insmod torture.ko && insmod scftorture.ko verbose=1 onoff_holdoff=5
onoff_interval=1; sleep 60; rmmod scftorture torture
[ 17.840768] torture module --- scftorture: disable_onoff_at_boot=0
ftrace_dump_at_shutdown=0 verbose_sleep_frequency=0
verbose_sleep_duration=1 random_shuffle=0
[ 17.842941] scftorture: --- Start of test: verbose=1 holdoff=0
longwait=0 nthreads=-1 onoff_holdoff=5 onoff_interval=1 shutdown_secs=0
stat_interval=60 stutter=5 use_cpus_read_lock=0, weight_resched=-1,
weight_single=-1, weight_single_rpc=-1, weight_single_wait=-1, weight_many1
[ 17.847211] scftorture: !!! built as module, weight_resched ignored
[ 17.848135] scf_sel_dump: 32.820 smp_call_function_single(nowait)
[ 17.849230] scf_sel_dump: 32.820 smp_call_function_single_rpc(wait)
[ 17.850475] scf_sel_dump: 32.820 smp_call_function_single(wait)
[ 17.851523] scf_sel_dump: 0.512 smp_call_function_many(nowait)
[ 17.852559] scf_sel_dump: 0.512 smp_call_function_many(wait)
[ 17.853602] scf_sel_dump: 0.256 smp_call_function(nowait)
[ 17.854541] scf_sel_dump: 0.256 smp_call_function(wait)
[ 17.855490] scftorture-torture: Creating torture_onoff task
[ 17.856673] scftorture-torture: torture_onoff task started
[ 17.856694] scftorture-torture: Creating torture_stutter task
[ 17.857701] scftorture-torture: torture_onoff begin holdoff
[ 17.859816] scftorture: Starting 64 smp_call_function() threads
[ 17.859816] scftorture-torture: torture_stutter task started
[ 17.860839] scftorture-torture: Creating scftorture_invoker task
[ 17.860930] scftorture-torture: Creating scftorture_invoker task
[ 17.860940] scftorture: scftorture_invoker 0: task started
..
[ 78.914652] scftorture: scf_invoked_count VER: 86855271 resched: 0
single: 27751906/27523464 single_ofl: 12966034/12854343 single_rpc:
27530433 single_rpc_ofl: 0 many: 430178/430527 all: 215119/214473 onoff:
1441/1441:1474/1474 14,268:8,177 22685:28034 (HZ=1000) ste: 0 stnmie:0
[ 78.919133] scftorture: --- End of test: SUCCESS: verbose=1
holdoff=0 longwait=0 nthreads=64 onoff_holdoff=5 onoff_interval=1
shutdown_secs=0 stat_interval=60 stutter=5 use_cpus_read_lock=0,
weight_resched=-1, weight_single=-1, weight_single_rpc=-1,
weight_single_wait=-1, weig1

$ insmod torture.ko random_shuffle=1 && insmod locktorture.ko
torture_type=mutex_lock rt_boost=1 rt_boost_factor=50 nested_locks=3
writer_fifo=1; sleep 60; rmmod locktorture torture
[ 83.340115] torture module --- mutex_lock: disable_onoff_at_boot=0
ftrace_dump_at_shutdown=0 verbose_sleep_frequency=0
verbose_sleep_duration=1 random_shuffle=1
[ 83.342142] mutex_lock-torture:--- Start of test [debug]:
acq_writer_lim=0 bind_readers=0-63 bind_writers=0-63 call_rcu_chains=0
long_hold=100 nested_locks=3 nreaders_stress=0 nwriters_stress=128
onoff_holdoff=0 onoff_interval=0 rt_boost=1 rt_boost_factor=50
shuffle_interval=3 1
[ 83.346306] mutex_lock-torture: Creating torture_shuffle task
[ 83.347383] mutex_lock-torture: Creating torture_stutter task
[ 83.347395] mutex_lock-torture: torture_shuffle task started
[ 83.348549] mutex_lock-torture: Creating lock_torture_writer task
[ 83.348555] mutex_lock-torture: torture_stutter task started
..
[ 146.611980] mutex_lock-torture: Stopping lock_torture_stats task
[ 146.613012] Writes: Total: 75575274 Max/Min: 1085187/396665 ???
Fail: 0
[ 146.614183] mutex_lock-torture: lock_torture_stats is stopping
[ 146.615160] Writes: Total: 75575274 Max/Min: 1085187/396665 ???
Fail: 0
[ 146.616273] mutex_lock-torture:--- End of test: SUCCESS [debug]:
acq_writer_lim=0 bind_readers=0-63 bind_writers=0-63 call_rcu_chains=0
long_hold=100 nested_locks=3 nreaders_stress=0 nwriters_stress=128
onoff_holdoff=0 onoff_interval=0 rt_boost=1 rt_boost_factor=50 shuffle_inte1

$ taskset -c 0-31,32-63 perf bench sched messaging -p -t -l 100000 -g 2
# Running 'sched/messaging' benchmark:0-31,32-63 perf bench sched
messaging -p -t -l 100000 -g 2
# 20 sender and receiver threads per group
# 2 groups == 80 threads run

Total time: 13.548 [sec]

$ perf bench sched pipe -l 100000 -T
# Running 'sched/pipe' benchmark:
# Executed 100000 pipe operations between two threads

Total time: 2.350 [sec]

23.503460 usecs/op
42546 ops/sec
```

Looks like there are slight differences (both improvements and
regressions) before & after this patch series. I believe they should be
ignored since I ran the test only few times and it's just a virtual machine.

I also ported commits in
https://github.com/johnstultz-work/linux-dev/commits/proxy-exec-v9-6.8
to an Android v6.1 kernel and tested on my Pixel 6.

In conclusion, I'm happy to give my:
Reviewed-by: Metin Kaya <metin.kaya@arm.com>
Tested-by: Metin Kaya <metin.kaya@arm.com>

Thanks,

\
 
 \ /
  Last update: 2024-05-27 15:53    [W:0.103 / U:2.628 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site