Messages in this thread | | | From | Andi Kleen <> | Subject | Re: [PATCH] profile: add support for modules and make output human readable | Date | Mon, 14 Mar 2011 12:45:25 -0700 |
| |
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
| |