[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <m2oc5dzdt6.fsf@firstfloor.org>
Date: Mon, 14 Mar 2011 12:45:25 -0700
From: Andi Kleen <andi@...stfloor.org>
To: Cliff Frey <cliff@...aki.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] profile: add support for modules and make output human readable
Cliff Frey <cliff@...aki.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@...ux.intel.com -- Speaking for myself only
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists