Re: [PATCH v2 (repost)] locking/lockdep: add debug_show_all_lock_holders()

[Ingo Molnar]

* Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:

> --- a/include/linux/debug_locks.h
> +++ b/include/linux/debug_locks.h
> @@ -48,7 +48,18 @@ extern int debug_locks_off(void);
>  #endif
>  
>  #ifdef CONFIG_LOCKDEP
> -extern void debug_show_all_locks(void);
> +extern void __debug_show_all_locks(bool show_stack);
> +
> +static inline void debug_show_all_locks(void)
> +{
> +	__debug_show_all_locks(false);
> +}
> +
> +static inline void debug_show_all_lock_holders(void)
> +{
> +	__debug_show_all_locks(true);
> +}
> +
>  extern void debug_show_held_locks(struct task_struct *task);
>  extern void debug_check_no_locks_freed(const void *from, unsigned long len);
>  extern void debug_check_no_locks_held(void);
> @@ -61,6 +72,10 @@ static inline void debug_show_held_locks(struct task_struct *task)
>  {
>  }
>  
> +static inline void debug_show_all_lock_holders(void)
> +{
> +}
> +

> -		debug_show_all_locks();
> +		debug_show_all_lock_holders();

> -void debug_show_all_locks(void)
> +void __debug_show_all_locks(bool show_stack)
>  {
>  	struct task_struct *g, *p;
>  
> @@ -6495,12 +6496,19 @@ void debug_show_all_locks(void)
>  		pr_warn("INFO: lockdep is turned off.\n");
>  		return;
>  	}
> -	pr_warn("\nShowing all locks held in the system:\n");
> +	if (show_stack)
> +		pr_warn("\nShowing all threads with locks held in the system:\n");
> +	else
> +		pr_warn("\nShowing all locks held in the system:\n");
>  
>  	rcu_read_lock();
>  	for_each_process_thread(g, p) {
>  		if (!p->lockdep_depth)
>  			continue;
> +		if (p == current && p->lockdep_depth == 1)
> +			continue;
> +		if (show_stack)
> +			sched_show_task(p);
>  		lockdep_print_held_locks(p);
>  		touch_nmi_watchdog();
>  		touch_all_softlockup_watchdogs();

Yeah, so note how you introduce a function with a parameter:

	void __debug_show_all_locks(bool show_stack)

... only to then *hide* the new parameter via helper functions:

	static inline void debug_show_all_locks(void)
	{
		__debug_show_all_locks(false);
	}

	static inline void debug_show_all_lock_holders(void)
	{
		__debug_show_all_locks(true);
	}

... which is a *strong* hint by our universe that the new parameter was 
probably a bad idea to begin with.

Given how small debug_show_all_locks() is to begin with, I'd suggest simply 
duplicating the loop into debug_show_all_lock_holders() or so.

Thanks,

	Ingo