Messages in this thread Patch in this message | | | Date | Mon, 27 Jun 2005 15:13:13 +0300 | From | Levent Serinol <> | Subject | Re: [PATCH] enable/disable profiling via proc/sysctl |
| |
Hi Andrew,
The fixed patch is attached. I hope, it's ok this time.
Thanks,
On 6/27/05, Andrew Morton <akpm@osdl.org> wrote: > Levent Serinol <lserinol@gmail.com> wrote: > > > > This patch enables controlling kernel profiling through proc/sysctl inferface. > > > > With this patch profiling will be available without rebooting the > > machine (especially for > > production servers) with some drawbacks of vmalloc(tlb). So, bootime > > algorithm part is left unchanged for anyone who wishes to use > > profiling as usual without tlb drawback by rebooting the machine. > > > > --- include/linux/sysctl.h.org 2005-06-13 16:05:17.000000000 +0300 > > +++ include/linux/sysctl.h 2005-06-25 15:05:06.000000000 +0300 > > Patches should be in `patch -p1' form, please. See > http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt > > > +static int prof_on = 0; > > +static int prof_booton = 0; > > There's no need to explicitly initialise these. > > > +#ifdef CONFIG_SMP > > +static int remove_hash_tables(void) > > +{ > > + int cpu; > > + > > + smp_mb(); > > + on_each_cpu(profile_nop, NULL, 0, 1); > > Why? > > > + for_each_online_cpu(cpu) { > > + struct page *page; > > + > > + if (per_cpu(cpu_profile_hits, cpu)[0]) { > > + page = virt_to_page(per_cpu(cpu_profile_hits, cpu)[0]); > > + per_cpu(cpu_profile_hits, cpu)[0] = NULL; > > + __free_page(page); > > + } > > + if (per_cpu(cpu_profile_hits, cpu)[1]) { > > + page = virt_to_page(per_cpu(cpu_profile_hits, cpu)[1]); > > + per_cpu(cpu_profile_hits, cpu)[1] = NULL; > > + __free_page(page); > > + } > > + } > > Can this race against itself? If two cpus run the sysctl at the same time? > We seem to have lock_kernel() coverage. It's be nice to do something > firmer. > > > +int profile_sysctl_handler(ctl_table *table, int write, > > + struct file *file, void __user *buffer, size_t *length, loff_t *ppos) > > +{ > > + int err; > > + struct proc_dir_entry *entry; > > + > > + if (prof_booton && write) return 0; > > + err=proc_dointvec(table, write, file, buffer, length, ppos); > > + if ((err >= 0) && write) { > > + prof_shift = profile_params[1]; > > + switch(profile_params[0]) > > + { > > + case 0: > > + if (prof_on) { > > Coding style is all over the place here, as well as in most of the rest of > the patch. > > > if (prof_booton && write) > return 0; > err = proc_dointvec(table, write, file, buffer, length, ppos); > if (err >= 0 && write) { > prof_shift = profile_params[1]; > switch (profile_params[0]) > { > case 0: > if (prof_on) { > > Every line was changed there... > > Also, doing multiple returns per function is unpopular, although the > very-early > > if (foo) > return <something>; > > right at the top of the function is OK. You can use > > if (err < 0 || !write) > goto out; > > to save a tab stop. > > > + } > > + break; > > } > break; > > > + case SCHED_PROFILING || CPU_PROFILING: > > eh? I'm surprised the compiler swallowed that. I guess it's the same as > `case 1:'. Looks like a bug though. > > > + if (prof_on) return -1; > > + prof_len = (_etext - _stext) >> prof_shift; > > + prof_buffer = vmalloc(prof_len*sizeof(atomic_t)); > > + if (!prof_buffer) return(-ENOMEM); > > + if (create_hash_tables()) { > > + vfree(prof_buffer); > > + return -1; > > + } > > + prof_on = profile_params[0]; > > + if (!(entry = create_proc_entry("profile", S_IWUSR | S_IRUGO, NULL))) { > > + remove_hash_tables(); > > + vfree(prof_buffer); > > + return 0; > > + } > > + entry->proc_fops = &proc_profile_operations; > > + entry->size = (1+prof_len) * sizeof(atomic_t); > > +#ifdef CONFIG_HOTPLUG_CPU > > + register_cpu_notifier(&profile_cpu_notifier); > > +#endif > > + profile_discard_flip_buffers(); > > + memset(prof_buffer, 0, prof_len * sizeof(atomic_t)); > > + switch(prof_on) > > + { > > + case SCHED_PROFILING:printk(KERN_INFO > > + "kernel schedule profiling enabled (shift: %ld)\n", > > + prof_shift); > > + break; > > + case CPU_PROFILING:printk(KERN_INFO > > + "kernel profiling enabled (shift: %ld)\n", > > + prof_shift); > > + break; > > + } > > + break; > > + } > > + } > > + return 0; > > +} > > Documentation/CodingStyle is your friend ;) > > > --- kernel/sysctl.c.org 2005-06-13 16:05:23.000000000 +0300 > > +++ kernel/sysctl.c 2005-06-26 02:06:23.000000000 +0300 > > ... > > +extern int profile_params[]; > > Try to place this declaration in a header. > > What locking protects prof_boot_on()? lock_kernel() won't be sufficient > because we're doing sleeping allocations in here. > > I suspect it would be best to whap a semaphore around the whole thing. >
--
Stay out of the road, if you want to grow old. ~ Pink Floyd ~. diff -uprN -X dontdiff linux-2.6.12-rc6.orig/include/linux/profile.h linux-2.6.12-rc6/include/linux/profile.h --- linux-2.6.12-rc6.orig/include/linux/profile.h 2005-03-02 09:38:08.000000000 +0200 +++ linux-2.6.12-rc6/include/linux/profile.h 2005-06-27 10:19:43.000000000 +0300 @@ -6,14 +6,21 @@ #include <linux/kernel.h> #include <linux/config.h> #include <linux/init.h> +#include <linux/sysctl.h> #include <linux/cpumask.h> #include <asm/errno.h> #define CPU_PROFILING 1 #define SCHED_PROFILING 2 +struct ctl_table; +struct file; struct proc_dir_entry; struct pt_regs; +extern int profile_params[]; +int profile_sysctl_handler(ctl_table *table, int write, + struct file *file, void __user *buffer, size_t +*length, loff_t *ppos); /* init basic kernel profiler */ void __init profile_init(void); diff -uprN -X dontdiff linux-2.6.12-rc6.orig/include/linux/sysctl.h linux-2.6.12-rc6/include/linux/sysctl.h --- linux-2.6.12-rc6.orig/include/linux/sysctl.h 2005-06-27 15:06:23.000000000 +0300 +++ linux-2.6.12-rc6/include/linux/sysctl.h 2005-06-27 10:15:54.000000000 +0300 @@ -136,6 +136,7 @@ enum KERN_UNKNOWN_NMI_PANIC=66, /* int: unknown nmi panic flag */ KERN_BOOTLOADER_TYPE=67, /* int: boot loader type */ KERN_RANDOMIZE=68, /* int: randomize virtual address space */ + KERN_PROFILE=69, /* int: profile on/off */ }; diff -uprN -X dontdiff linux-2.6.12-rc6.orig/kernel/profile.c linux-2.6.12-rc6/kernel/profile.c --- linux-2.6.12-rc6.orig/kernel/profile.c 2005-06-27 15:06:23.000000000 +0300 +++ linux-2.6.12-rc6/kernel/profile.c 2005-06-27 15:05:07.000000000 +0300 @@ -21,7 +21,6 @@ #include <linux/mm.h> #include <linux/cpumask.h> #include <linux/cpu.h> -#include <linux/profile.h> #include <linux/highmem.h> #include <asm/sections.h> #include <asm/semaphore.h> @@ -37,9 +36,12 @@ struct profile_hit { /* Oprofile timer tick hook */ int (*timer_hook)(struct pt_regs *); +struct semaphore prof_sem; +int profile_params[2] = {0, 0}; static atomic_t *prof_buffer; static unsigned long prof_len, prof_shift; static int prof_on; +static int prof_booton; static cpumask_t prof_cpu_mask = CPU_MASK_ALL; #ifdef CONFIG_SMP static DEFINE_PER_CPU(struct profile_hit *[2], cpu_profile_hits); @@ -74,12 +76,14 @@ __setup("profile=", profile_setup); void __init profile_init(void) { + init_MUTEX(&prof_sem); if (!prof_on) return; /* only text is profiled */ prof_len = (_etext - _stext) >> prof_shift; prof_buffer = alloc_bootmem(prof_len*sizeof(atomic_t)); + prof_booton = 1; } /* Profile event notifications */ @@ -367,6 +371,12 @@ static int __devinit profile_cpu_callbac } return NOTIFY_OK; } +static struct notifier_block profile_cpu_notifier = +{ + .notifier_call = profile_cpu_callback, + .priority = 0, +}; + #endif /* CONFIG_HOTPLUG_CPU */ #else /* !CONFIG_SMP */ #define profile_flip_buffers() do { } while (0) @@ -500,11 +510,11 @@ static struct file_operations proc_profi }; #ifdef CONFIG_SMP -static void __init profile_nop(void *unused) +static void profile_nop(void *unused) { } -static int __init create_hash_tables(void) +int create_hash_tables(void) { int cpu; @@ -548,6 +558,104 @@ out_cleanup: #define create_hash_tables() ({ 0; }) #endif +#ifdef CONFIG_SMP +int remove_hash_tables(void) +{ + int cpu; + + + for_each_online_cpu(cpu) { + struct page *page; + + if (per_cpu(cpu_profile_hits, cpu)[0]) { + page = virt_to_page(per_cpu(cpu_profile_hits, cpu)[0]); + per_cpu(cpu_profile_hits, cpu)[0] = NULL; + __free_page(page); + } + if (per_cpu(cpu_profile_hits, cpu)[1]) { + page = virt_to_page(per_cpu(cpu_profile_hits, cpu)[1]); + per_cpu(cpu_profile_hits, cpu)[1] = NULL; + __free_page(page); + } + } + return -1; +} +#else +#define remove_hash_tables() ({ 0; }) +#endif + +int profile_sysctl_handler(ctl_table *table, int write, struct file *file, + void __user *buffer, size_t *length, loff_t *ppos) +{ + int err; + struct proc_dir_entry *entry; + + + down(&prof_sem); + + if (prof_booton && write) + goto out; + + err=proc_dointvec(table, write, file, buffer, length, ppos); + if (err < 0 || !write) + goto out; + + prof_shift = profile_params[1]; + + if (!profile_params[0]) { + if (prof_on) { + prof_on = 0; + remove_proc_entry("profile",NULL); +#ifdef CONFIG_HOTPLUG_CPU + unregister_cpu_notifier(&profile_cpu_notifier); +#endif + remove_hash_tables(); + vfree(prof_buffer); + printk(KERN_INFO "kernel profiling disabled\n"); + } + } + + if ((profile_params[0]==SCHED_PROFILING) || (profile_params[0]==CPU_PROFILING)) { + if (prof_on) + goto out; + prof_len = (_etext - _stext) >> prof_shift; + prof_buffer = vmalloc(prof_len*sizeof(atomic_t)); + if (!prof_buffer) + goto out; + if (create_hash_tables()) { + vfree(prof_buffer); + goto out; + } + prof_on = profile_params[0]; + if (!(entry = create_proc_entry("profile", S_IWUSR | S_IRUGO, NULL))) { + remove_hash_tables(); + vfree(prof_buffer); + goto out; + } + entry->proc_fops = &proc_profile_operations; + entry->size = (1+prof_len) * sizeof(atomic_t); +#ifdef CONFIG_HOTPLUG_CPU + register_cpu_notifier(&profile_cpu_notifier); +#endif + profile_discard_flip_buffers(); + memset(prof_buffer, 0, prof_len * sizeof(atomic_t)); + switch(prof_on) { + case SCHED_PROFILING: + printk(KERN_INFO "kernel schedule profiling enabled (shift: %ld)\n", + prof_shift); + break; + case CPU_PROFILING: + printk(KERN_INFO "kernel profiling enabled (shift: %ld)\n", + prof_shift); + break; + } + } + +out: + up(&prof_sem); + return 0; +} + static int __init create_proc_profile(void) { struct proc_dir_entry *entry; @@ -560,7 +668,11 @@ static int __init create_proc_profile(vo return 0; entry->proc_fops = &proc_profile_operations; entry->size = (1+prof_len) * sizeof(atomic_t); - hotcpu_notifier(profile_cpu_callback, 0); +#ifdef CONFIG_HOTPLUG_CPU + register_cpu_notifier(&profile_cpu_notifier); +#endif + profile_params[0] = prof_on; + profile_params[1] = prof_shift; return 0; } module_init(create_proc_profile); diff -uprN -X dontdiff linux-2.6.12-rc6.orig/kernel/sysctl.c linux-2.6.12-rc6/kernel/sysctl.c --- linux-2.6.12-rc6.orig/kernel/sysctl.c 2005-06-27 15:06:23.000000000 +0300 +++ linux-2.6.12-rc6/kernel/sysctl.c 2005-06-27 10:19:07.000000000 +0300 @@ -21,6 +21,7 @@ #include <linux/config.h> #include <linux/module.h> #include <linux/mm.h> +#include <linux/profile.h> #include <linux/swap.h> #include <linux/slab.h> #include <linux/sysctl.h> @@ -642,7 +643,15 @@ static ctl_table kern_table[] = { .mode = 0644, .proc_handler = &proc_dointvec, }, - + { + .ctl_name = KERN_PROFILE, + .procname = "profile", + .data = &profile_params, + .maxlen = 2*sizeof(int), + .mode = 0644, + .proc_handler = &profile_sysctl_handler, + .strategy = &sysctl_intvec, + }, { .ctl_name = 0 } }; | |