On 10/25/2012 04:05 AM, Ingo Molnar wrote:
>
> * Sasha Levin <sasha.levin@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
