| Message ID | 1351098010-20849-2-git-send-email-sasha.levin@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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. > } > > 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? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/25/2012 01:06 PM, Ingo Molnar wrote: > * Sasha Levin <sasha.levin@oracle.com> wrote: >> 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. I'm not sure how we'd make debug_show_all_locks() work in userspace since it would require us to wrap do_each_thread() & friends to iterate over all our task_structs. I was thinking about writing a corresponding debug_show_all_locks() that would simply iterate a list of our dummy task_structs. Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 } 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; } /* @@ -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; @@ -4166,7 +4171,7 @@ retry: read_unlock(&tasklist_lock); } EXPORT_SYMBOL_GPL(debug_show_all_locks); - +#endif /* * Careful: only use this function if you are sure that * the task cannot run in parallel!
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(-)