[Skiboot] [RFC PATCH 2/2] lock: Add additional lock auditing code

[Nicholas Piggin]

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>