Messages in this thread | | | Date | Mon, 16 Jan 2023 22:28:01 -0500 | Subject | Re: [PATCH v2 (repost)] locking/lockdep: add debug_show_all_lock_holders() | From | Waiman Long <> |
| |
On 1/14/23 04:53, Tetsuo Handa wrote: > On 2023/01/14 18:36, Ingo Molnar wrote: >> 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. > Initial version at https://lkml.kernel.org/r/82af40cc-bf85-2b53-b8f9-dfc12e66a781@I-love.SAKURA.ne.jp > was duplicating the loop. > > Waiman Long suggested me not to duplicate the loop at > https://lkml.kernel.org/r/3e027453-fda4-3891-3ec3-5623f1525e56@redhat.com . > > Please talk with Waiman. I'm fine with either approach.
My original concern was that two functions are very similar with some minor difference. My suggestion was to use a common helper to reduce the code redundancy and future maintenance.
I do have some nits about the patch. The show_stack parameter isn't informative. Maybe you can use show_tasks as the parameter name since the major difference is the calling of sched_show_task().
Define a new helper like debug_show_all_locks_tasks(bool show_tasks), use it directly in check_hung_uninterruptible_tasks() and make debug_show_all_lock() call debug_show_all_locks_tasks().
Ingo, will that OK with you?
Cheers, Longman
| |