| Message ID | 1542653726-5655-10-git-send-email-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locking/lockdep: Add a new class of terminal locks | expand |
On Mon 2018-11-19 13:55:18, Waiman Long wrote: > By making the object hash locks nestable terminal locks, we can avoid > a bunch of unnecessary lockdep validations as well as saving space > in the lockdep tables. Please, explain which terminal lock might be nested. Hmm, it would hide eventual nesting of other terminal locks. It might reduce lockdep reliability. I wonder if the space optimization is worth it. Finally, it might be good to add a short explanation what (nested) terminal locks mean into each commit. It would help people to understand the effect without digging into the lockdep code, ... Best Regards, Petr
On 11/22/2018 10:33 AM, Petr Mladek wrote: > On Mon 2018-11-19 13:55:18, Waiman Long wrote: >> By making the object hash locks nestable terminal locks, we can avoid >> a bunch of unnecessary lockdep validations as well as saving space >> in the lockdep tables. > Please, explain which terminal lock might be nested. > > Hmm, it would hide eventual nesting of other terminal locks. > It might reduce lockdep reliability. I wonder if the space > optimization is worth it. > > Finally, it might be good to add a short explanation what (nested) > terminal locks mean into each commit. It would help people to > understand the effect without digging into the lockdep code, ... > > Best Regards, > Petr Nested terminal lock is currently only used in the debugobjects code. It should only be used on a case-by-case basis. In the case of the debugojects code, the locking patterns are: (1) raw_spin_lock(&db_lock); ... raw_spin_unlock(&db_lock); (2) raw_spin_lock(&db_lock); ... raw_spin_lock(&pool_lock); ... raw_spin_unlock(&pool_lock); ... raw_spin_unlock(&db_lock); (3) raw_spin_lock(&pool_lock); ... raw_spin_unlock(&pool_lock); So the db_lock is made to be nestable that it can allow acquisition of pool_lock (a regular terminal lock) underneath it. Cheers, Longman
On Thu 2018-11-22 15:17:52, Waiman Long wrote: > On 11/22/2018 10:33 AM, Petr Mladek wrote: > > On Mon 2018-11-19 13:55:18, Waiman Long wrote: > >> By making the object hash locks nestable terminal locks, we can avoid > >> a bunch of unnecessary lockdep validations as well as saving space > >> in the lockdep tables. > > Please, explain which terminal lock might be nested. > > So the db_lock is made to be nestable that it can allow acquisition of > pool_lock (a regular terminal lock) underneath it. Please, mention this in the commit message. Best Regards, Petr
On Mon, Nov 19, 2018 at 01:55:18PM -0500, Waiman Long wrote: > By making the object hash locks nestable terminal locks, we can avoid > a bunch of unnecessary lockdep validations as well as saving space > in the lockdep tables. So the 'problem'; which you've again not explained; is that debugobjects has the following lock order: &db->lock &pool_lock And you seem to want to tag '&db->lock' as terminal, which is obviuosly a big fat lie. You've also not explained why it is safe to do this (I think it actually is, but you really should've spelled it out). Furthermore; you've not justified any of this 'insanity' with numbers. What do we gain with this nestable madness that justifies the crazy?
diff --git a/lib/debugobjects.c b/lib/debugobjects.c index 4216d3d..c6f3967 100644 --- a/lib/debugobjects.c +++ b/lib/debugobjects.c @@ -1129,8 +1129,13 @@ void __init debug_objects_early_init(void) { int i; - for (i = 0; i < ODEBUG_HASH_SIZE; i++) + /* + * Make the obj_hash locks nestable terminal locks. + */ + for (i = 0; i < ODEBUG_HASH_SIZE; i++) { raw_spin_lock_init(&obj_hash[i].lock); + lockdep_set_terminal_nestable_class(&obj_hash[i].lock); + } for (i = 0; i < ODEBUG_POOL_SIZE; i++) hlist_add_head(&obj_static_pool[i].node, &obj_pool);
By making the object hash locks nestable terminal locks, we can avoid a bunch of unnecessary lockdep validations as well as saving space in the lockdep tables. Signed-off-by: Waiman Long <longman@redhat.com> --- lib/debugobjects.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)