From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [RFC 2/3] lockdep: be nice about compiling from userspace Date: Thu, 25 Oct 2012 12:58:42 -0400 Message-ID: <50896FC2.1070001@oracle.com> References: <1351098010-20849-1-git-send-email-sasha.levin@oracle.com> <1351098010-20849-2-git-send-email-sasha.levin@oracle.com> <20121025080502.GF3712@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: penberg@kernel.org, mingo@redhat.com, peterz@infradead.org, asias.hejun@gmail.com, tglx@linutronix.de, gorcunov@openvz.org, kvm@vger.kernel.org To: Ingo Molnar Return-path: Received: from acsinet15.oracle.com ([141.146.126.227]:41639 "EHLO acsinet15.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757202Ab2JYQ7V (ORCPT ); Thu, 25 Oct 2012 12:59:21 -0400 In-Reply-To: <20121025080502.GF3712@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: On 10/25/2012 04:05 AM, Ingo Molnar wrote: > > * Sasha Levin 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 >> --- >> 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. The question is here more of a "would lockdep maintainers be ok with adding that considering there's no in-kernel justification for those?" Thanks, Sasha