From: Ingo Molnar <email@example.com> To: Waiman Long <firstname.lastname@example.org>, Josh Poimboeuf <email@example.com> Cc: Peter Zijlstra <firstname.lastname@example.org>, Ingo Molnar <email@example.com>, Will Deacon <firstname.lastname@example.org>, Thomas Gleixner <email@example.com>, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, Petr Mladek <email@example.com>, Sergey Senozhatsky <firstname.lastname@example.org>, Andrey Ryabinin <email@example.com>, Tejun Heo <firstname.lastname@example.org>, Andrew Morton <email@example.com>, Linus Torvalds <firstname.lastname@example.org> Subject: Re: [RFC PATCH 00/12] locking/lockdep: Add a new class of terminal locks Date: Fri, 9 Nov 2018 09:04:12 +0100 [thread overview] Message-ID: <20181109080412.GC86700@gmail.com> (raw) In-Reply-To: <email@example.com> * Waiman Long <firstname.lastname@example.org> wrote: > The purpose of this patchset is to add a new class of locks called > terminal locks and converts some of the low level raw or regular > spinlocks to terminal locks. A terminal lock does not have forward > dependency and it won't allow a lock or unlock operation on another > lock. Two level nesting of terminal locks is allowed, though. > > Only spinlocks that are acquired with the _irq/_irqsave variants or > acquired in an IRQ disabled context should be classified as terminal > locks. > > Because of the restrictions on terminal locks, we can do simple checks on > them without using the lockdep lock validation machinery. The advantages > of making these changes are as follows: > > 1) The lockdep check will be faster for terminal locks without using > the lock validation code. > 2) It saves table entries used by the validation code and hence make > it harder to overflow those tables. > > In fact, it is possible to overflow some of the tables by running > a variety of different workloads on a debug kernel. I have seen bug > reports about exhausting MAX_LOCKDEP_KEYS, MAX_LOCKDEP_ENTRIES and > MAX_STACK_TRACE_ENTRIES. This patch will help to reduce the chance > of overflowing some of the tables. > > Performance wise, there was no statistically significant difference in > performanace when doing a parallel kernel build on a debug kernel. Could you please measure a locking intense workload instead, such as: $ perf stat --null --sync --repeat 10 perf bench sched messaging and profile which locks used there could be marked terminal, and measure the before/after performance impact? > Below were selected output lines from the lockdep_stats files of the > patched and unpatched kernels after bootup and running parallel kernel > builds. > > Item Unpatched kernel Patched kernel % Change > ---- ---------------- -------------- -------- > direct dependencies 9732 8994 -7.6% > dependency chains 18776 17033 -9.3% > dependency chain hlocks 76044 68419 -10.0% > stack-trace entries 110403 104341 -5.5% That's pretty impressive! > There were some reductions in the size of the lockdep tables. They were > not significant, but it is still a good start to rein in the number of > entries in those tables to make it harder to overflow them. Agreed. BTW., if you are interested in more radical approaches to optimize lockdep, we could also add a static checker via objtool driven call graph analysis, and mark those locks terminal that we can prove are terminal. This would require the unified call graph of the kernel image and of all modules to be examined in a final pass, but that's within the principal scope of objtool. (This 'final pass' could also be done during bootup, at least in initial versions.) Note that beyond marking it 'terminal' such a static analysis pass would also allow the detection of obvious locking bugs at the build (or boot) stage already - plus it would allow the disabling of lockdep for self-contained locks that don't interact with anything else. I.e. the static analysis pass would 'augment' lockdep and leave only those locks active for runtime lockdep tracking whose dependencies it cannot prove to be correct yet. Thanks, Ingo
next prev parent reply other threads:[~2018-11-09 8:04 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-08 20:34 Waiman Long 2018-11-08 20:34 ` [RFC PATCH 01/12] locking/lockdep: Rework lockdep_set_novalidate_class() Waiman Long 2018-11-10 14:14 ` Peter Zijlstra 2018-11-11 0:26 ` Waiman Long 2018-11-11 1:28 ` Peter Zijlstra 2018-11-08 20:34 ` [RFC PATCH 02/12] locking/lockdep: Add a new terminal lock type Waiman Long 2018-11-10 14:17 ` Peter Zijlstra 2018-11-11 0:28 ` Waiman Long 2018-11-08 20:34 ` [RFC PATCH 03/12] locking/lockdep: Add DEFINE_TERMINAL_SPINLOCK() and related macros Waiman Long 2018-11-08 20:34 ` [RFC PATCH 04/12] printk: Make logbuf_lock a terminal lock Waiman Long 2018-11-08 20:34 ` [RFC PATCH 05/12] debugobjects: Mark pool_lock as " Waiman Long 2018-11-08 20:34 ` [RFC PATCH 06/12] debugobjects: Move printk out of db lock critical sections Waiman Long 2018-11-08 20:34 ` [RFC PATCH 07/12] locking/lockdep: Add support for nested terminal locks Waiman Long 2018-11-10 14:20 ` Peter Zijlstra 2018-11-11 0:30 ` Waiman Long 2018-11-11 1:30 ` Peter Zijlstra 2018-11-08 20:34 ` [RFC PATCH 08/12] debugobjects: Make object hash locks " Waiman Long 2018-11-08 20:34 ` [RFC PATCH 09/12] lib/stackdepot: Make depot_lock a terminal spinlock Waiman Long 2018-11-08 20:34 ` [RFC PATCH 10/12] locking/rwsem: Mark rwsem.wait_lock as a terminal lock Waiman Long 2018-11-08 20:34 ` [RFC PATCH 11/12] cgroup: Mark the rstat percpu lock as terminal Waiman Long 2018-11-08 20:34 ` [RFC PATCH 12/12] mm/kasan: Make quarantine_lock a terminal lock Waiman Long 2018-11-09 8:04 ` Ingo Molnar [this message] 2018-11-09 15:48 ` [RFC PATCH 00/12] locking/lockdep: Add a new class of terminal locks Waiman Long 2018-11-12 5:15 ` Ingo Molnar 2018-11-10 14:10 ` Peter Zijlstra 2018-11-10 23:35 ` Waiman Long 2018-11-12 5:10 ` Ingo Molnar 2018-11-12 5:53 ` Josh Poimboeuf 2018-11-12 6:30 ` Ingo Molnar 2018-11-12 22:22 ` Josh Poimboeuf 2018-11-12 22:56 ` Waiman Long
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=20181109080412.GC86700@gmail.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [RFC PATCH 00/12] locking/lockdep: Add a new class of terminal locks' \ /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
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).