lkml.org 
[lkml]   [2011]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] profile: add support for modules and make output human readable
Date
Cliff Frey <cliff@meraki.com> writes:

> This changes /proc/profile to include kernel modules and also print out data in
> human readable form (it does symbol lookups of all kernel addresses). It also
> adds /proc/profile_m which gives a high level summary of samples in
> userspace/kernel/each module.

I like the idea of extending this profiler to modules.

But the symbol lookup will make existing readprofile much slower than it
used to be. Please make it a new file.

In general it seems to still need quite a lot of work though:

> - Make this a separate kernel config option.

This means it'll be off when you need it. The cool thing about
it was always that it was always there.

> #endif
> -
> +#ifdef CONFIG_PROFILING
> + {
> + extern unsigned prof_shift;

This should be in a header.

> return 1;
> @@ -105,6 +116,11 @@ __setup("profile=", profile_setup);
> int __ref profile_init(void)
> {
> int buffer_bytes;
> +
> + // XXX turn on profiling
> + prof_shift = 4;
> + prof_on = CPU_PROFILING;

Was this supposed to be a submission ready patch?

> +
> if (!prof_on)
> return 0;
>
> @@ -225,8 +241,13 @@ void unregister_timer_hook(int (*hook)(struct pt_regs *))
> }
> EXPORT_SYMBOL_GPL(unregister_timer_hook);
>
> +static inline int within(unsigned long addr, void *start, unsigned long size)
> +{
> + return ((void *)addr >= start && (void *)addr < start + size);
> +}
>

I'm pretty sure we already have that in kernel.h

> #ifdef CONFIG_SMP
> +#error "Meraki has not added SMP profiling support yet"

Ok that explains the missing locking. Obviously that's not usable
for a lot of people.

> /*
> * Each cpu has a pair of open-addressed hashtables for pending
> * profile hits. read_profile() IPI's all cpus to request them
> @@ -420,23 +441,53 @@ out_free:
> void profile_hits(int type, void *__pc, unsigned int nr_hits)
> {
> unsigned long pc;
> + unsigned long pc_kernel_offset;
> + struct module *mod = NULL;
>
> + pc = (unsigned long) __pc;
> if (prof_on != type || !prof_buffer)
> return;
> - pc = ((unsigned long)__pc - (unsigned long)_stext) >> prof_shift;
> - atomic_add(nr_hits, &prof_buffer[min(pc, prof_len - 1)]);
> + pc_kernel_offset = (pc - (unsigned long)_stext) >> prof_shift;
> + if (pc_kernel_offset < prof_len) {
> + atomic_add(nr_hits, &prof_buffer[pc_kernel_offset]);
> + atomic_add(nr_hits, &known_kernel_hits);
> + return;
> + }
> +
> + list_for_each_entry(mod, &modules, list)

You cannot just walk this list without any protection. Modules
can be unloaded any time. Or maybe make it depend
on the !MODULE_UNLOAD

> + if (lastcount > 5)
> + seq_printf(m, "%08lx % 6d %s\n",
> lastpc, lastcount, lastname);

Weird magic number


-Andi
--
ak@linux.intel.com -- Speaking for myself only


\
 
 \ /
  Last update: 2011-03-14 20:49    [W:0.046 / U:0.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site