Re: [RFC 2/3] lockdep: be nice about compiling from userspace

[Ingo Molnar <mingo@kernel.org>]

* Sasha Levin <sasha.levin@oracle.com> wrote:

> On 10/25/2012 04:05 AM, Ingo Molnar wrote:
> > 
> > * Sasha Levin <sasha.levin@oracle.com> wrote:
> > 
> >> We can rather easily make lockdep work from userspace, although 3 issues
> >> remain which I'm not sure about:
> >>
> >>  - Kernel naming - we can just wrap init_utsname() to return kvmtool related
> >> utsname, is that what we want though?
> >>
> >>  - static_obj() - I don't have a better idea than calling mprobe(), which sounds
> >> wrong as well.
> >>
> >>  - debug_show_all_locks() - we don't actually call it from userspace yet, but I think
> >> we might want to, so I'm not sure how to make it pretty using existing kernel code.
> >>
> >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >> ---
> >>  kernel/lockdep.c | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> >> index 7981e5b..fdd3670 100644
> >> --- a/kernel/lockdep.c
> >> +++ b/kernel/lockdep.c
> >> @@ -567,10 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr)
> >>  
> >>  static void print_kernel_ident(void)
> >>  {
> >> +#ifdef __KERNEL__
> >>  	printk("%s %.*s %s\n", init_utsname()->release,
> >>  		(int)strcspn(init_utsname()->version, " "),
> >>  		init_utsname()->version,
> >>  		print_tainted());
> >> +#endif
> > 
> > I guess wrapping init_utsname() is not worth it. Although 
> > kvmtool could provide the host system's utsname - kernel 
> > identity is useful for debugging info.
> > 
> > You could generate a Git hash version string like tools/perf/ 
> > does (see PERF_VERSION and tools/perf/util/PERF-VERSION-GEN), 
> > and put that into the ->version field.
> > 
> > ->release could be the kvmtool version, and print_tainted() 
> > could return an empty string.
> > 
> > That way you could provide init_utsname() and could remove this 
> > #ifdef.
> 
> Yeah, we already generate the version string for
> 'lkvm version' anyways, so I guess I'll just add init_utsname().
> 
> 
> >>  }
> >>  
> >>  static int very_verbose(struct lock_class *class)
> >> @@ -586,6 +588,7 @@ static int very_verbose(struct lock_class *class)
> >>   */
> >>  static int static_obj(void *obj)
> >>  {
> >> +#ifdef __KERNEL__
> >>  	unsigned long start = (unsigned long) &_stext,
> >>  		      end   = (unsigned long) &_end,
> >>  		      addr  = (unsigned long) obj;
> >> @@ -609,6 +612,8 @@ static int static_obj(void *obj)
> >>  	 * module static or percpu var?
> >>  	 */
> >>  	return is_module_address(addr) || is_module_percpu_address(addr);
> >> +#endif
> >> +	return 1;
> > 
> > Could you put an:
> > 
> > #ifndef static_obj
> > 
> > around it? Then kvmtool could define its own trivial version of 
> > static_obj():
> > 
> >   #define static_obj(x) 1U
> > 
> > or so.
> > 
> >> @@ -4108,7 +4113,7 @@ void debug_check_no_locks_held(struct task_struct *task)
> >>  	if (unlikely(task->lockdep_depth > 0))
> >>  		print_held_locks_bug(task);
> >>  }
> >> -
> >> +#ifdef __KERNEL__
> >>  void debug_show_all_locks(void)
> >>  {
> >>  	struct task_struct *g, *p;
> > 
> > I guess a show-all-locks functionality would be useful to 
> > kvmtool as well?
> 
> Regarding the above two,
> 
> Yes, we can wrap both static_obj() and debug_show_all_locks() 
> with #ifndefs and let kvmtool provide it's own version of 
> those two.

Only static_obj() - I see no immediate reason why you shouldn't 
be able to utilize debug_show_all_locks(). 'vm debug -a' already 
lists all backtraces on all vcpus - so 'vm debug lockdep -a' 
could list all current locks and indicate which one is held and 
by whom.

> The question is here more of a "would lockdep maintainers be 
> ok with adding that considering there's no in-kernel 
> justification for those?"

Yeah, such a simple patch would be acceptable to me, being nice 
isn't against the law.

Thanks,

	Ingo