[Skiboot] [RFC PATCH 2/2] lock: Add additional lock auditing code
Nicholas Piggin
npiggin at gmail.com
Wed Dec 20 12:59:37 AEDT 2017
On Tue, 12 Dec 2017 17:32:34 +1100
Benjamin Herrenschmidt <benh at kernel.crashing.org> wrote:
> Keep track of lock owner name and replace lock_depth counter
> with a per-cpu list of locks held by the cpu.
>
> This allows us to print the actual locks held in case we hit
> the (in)famous message about opal_pollers being run with a
> lock held.
>
> It also allows us to warn (and drop them) if locks are still
> held when returning to the OS or completing a scheduled job.
I like these patches. There is some question about how to handle
detected internal errors like this consistently (like we should
always disable fast reboot and perhaps there are other limp-home
features you might set), but that doesn't have to be solved in
this patch.
This is working closer toward a platform we can implement a
lock ordering checker with too (together with Matt's stuff, his
was a post-deadlock checker I think, but we could do a lock
ordering observer like lockdep which is more useful to catch
rare deadlocks).
> @@ -57,7 +57,7 @@ static void unlock_check(struct lock *l)
> if (l->in_con_path && this_cpu()->con_suspend == 0)
> lock_error(l, "Unlock con lock with console not suspended", 3);
>
> - if (this_cpu()->lock_depth == 0)
> + if (list_empty(&this_cpu()->locks_held))
> lock_error(l, "Releasing lock with 0 depth", 4);
> }
>
Now it's more that it was not marked as owned by us.
> @@ -130,9 +131,10 @@ void unlock(struct lock *l)
>
> unlock_check(l);
>
> + l->owner = NULL;
> lwsync();
> - this_cpu()->lock_depth--;
> l->lock_val = 0;
> + list_del(&l->list);
This needs to be done above the lwsync AFAIKS.
> +void drop_my_locks(bool warn)
> +{
> + struct lock *l;
> +
> + while((l = list_pop(&this_cpu()->locks_held, struct lock, list)) != NULL) {
> + if (warn)
> + prlog(PR_ERR, " %s\n", l->owner);
> + unlock(l);
> + }
> +}
Could disable fast reboot here?
>
> -/* Initializer */
> -#define LOCK_UNLOCKED { .lock_val = 0, .in_con_path = 0 }
> +/* Initializer... not ideal but works for now. If we need different
> + * values for the fields and/or start getting warnings we'll have to
> + * play macro tricks
> + */
> +#define LOCK_UNLOCKED { 0 }
Does this work? Why can't we do named initializers for all?
Otherwise looks good. For both (if you double check the unlock code),
Reviewed-by: Nicholas Piggin <npiggin at gmail.com>
More information about the Skiboot
mailing list