[PATCH] Some love to default profiler

[PATCH] Some love to default profiler

[Alexey Dobriyan]

1) Drop __KERNEL__ out of profile.h. It contains only internal kernel stuff and
   not in exported headers list
2) Put profile.c under CONFIG_PROFILING. You enabled profiling in config, you
   will get it. Removes conditional branch from schedule(). Code savings on my
   usual config:

	   text	   data	    bss	    dec	    hex	filename
	2921871	 179895	 180224	3281990	 321446	vmlinux		before
	2920141	 179847	 180224	3280212	 320d54	vmlinux		after
	--------------------------------------------------------------
	  -1730     -48           -1778

3) Make timer_hook static (hi, Adrian!)
4) Convert do {} while (0) into static inline functions
5) minor misc stuff

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/linux/profile.h |   78 +++++++++++++++++++++++++-----------------------
 kernel/Makefile         |    3 +
 kernel/profile.c        |    2 -
 3 files changed, 44 insertions(+), 39 deletions(-)

--- a/include/linux/profile.h
+++ b/include/linux/profile.h
@@ -1,8 +1,6 @@
 #ifndef _LINUX_PROFILE_H
 #define _LINUX_PROFILE_H
 
-#ifdef __KERNEL__
-
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/cpumask.h>
@@ -10,16 +8,22 @@
 
 #include <asm/errno.h>
 
-extern int prof_on __read_mostly;
+struct proc_dir_entry;
+struct pt_regs;
+struct notifier_block;
 
 #define CPU_PROFILING	1
 #define SCHED_PROFILING	2
 #define SLEEP_PROFILING	3
 #define KVM_PROFILING	4
 
-struct proc_dir_entry;
-struct pt_regs;
-struct notifier_block;
+enum profile_type {
+	PROFILE_TASK_EXIT,
+	PROFILE_MUNMAP
+};
+
+#ifdef CONFIG_PROFILING
+extern int prof_on __read_mostly;
 
 /* init basic kernel profiler */
 void __init profile_init(void);
@@ -42,22 +46,6 @@ static inline void profile_hit(int type, void *ip)
 		profile_hits(type, ip, 1);
 }
 
-#ifdef CONFIG_PROC_FS
-void create_prof_cpu_mask(struct proc_dir_entry *);
-#else
-#define create_prof_cpu_mask(x)			do { (void)(x); } while (0)
-#endif
-
-enum profile_type {
-	PROFILE_TASK_EXIT,
-	PROFILE_MUNMAP
-};
-
-#ifdef CONFIG_PROFILING
-
-struct task_struct;
-struct mm_struct;
-
 /* task is in do_exit() */
 void profile_task_exit(struct task_struct * task);
 
@@ -77,14 +65,30 @@ int profile_event_unregister(enum profile_type, struct notifier_block * n);
 
 int register_timer_hook(int (*hook)(struct pt_regs *));
 void unregister_timer_hook(int (*hook)(struct pt_regs *));
-
-/* Timer based profiling hook */
-extern int (*timer_hook)(struct pt_regs *);
-
-struct pt_regs;
-
 #else
-
+#define prof_on 0
+static inline void profile_init(void)
+{
+}
+static inline void profile_tick(int type)
+{
+}
+static inline void profile_hits(int type, void *ip, unsigned int nr_hits)
+{
+}
+static inline void profile_hit(int type, void *ip)
+{
+}
+static inline void profile_munmap(unsigned long addr)
+{
+}
+static inline void profile_task_exit(struct task_struct *tsk)
+{
+}
+static inline int profile_handoff_task(struct task_struct *tsk)
+{
+	return 0;
+}
 static inline int task_handoff_register(struct notifier_block * n)
 {
 	return -ENOSYS;
@@ -105,10 +109,6 @@ static inline int profile_event_unregister(enum profile_type t, struct notifier_
 	return -ENOSYS;
 }
 
-#define profile_task_exit(a) do { } while (0)
-#define profile_handoff_task(a) (0)
-#define profile_munmap(a) do { } while (0)
-
 static inline int register_timer_hook(int (*hook)(struct pt_regs *))
 {
 	return -ENOSYS;
@@ -116,11 +116,15 @@ static inline int register_timer_hook(int (*hook)(struct pt_regs *))
 
 static inline void unregister_timer_hook(int (*hook)(struct pt_regs *))
 {
-	return;
 }
+#endif
 
-#endif /* CONFIG_PROFILING */
-
-#endif /* __KERNEL__ */
+#ifdef CONFIG_PROC_FS
+void create_prof_cpu_mask(struct proc_dir_entry *);
+#else
+static inline void create_prof_cpu_mask(struct proc_dir_entry *pde)
+{
+}
+#endif
 
 #endif /* _LINUX_PROFILE_H */
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -2,7 +2,7 @@
 # Makefile for the linux kernel.
 #
 
-obj-y     = sched.o fork.o exec_domain.o panic.o printk.o profile.o \
+obj-y     = sched.o fork.o exec_domain.o panic.o printk.o \
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
 	    signal.o sys.o kmod.o workqueue.o pid.o \
@@ -10,6 +10,7 @@ obj-y     = sched.o fork.o exec_domain.o panic.o printk.o profile.o \
 	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
 	    hrtimer.o rwsem.o latency.o nsproxy.o srcu.o die_notifier.o
 
+obj-$(CONFIG_PROFILING) += profile.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
 obj-y += time/
 obj-$(CONFIG_DEBUG_MUTEXES) += mutex-debug.o
--- a/kernel/profile.c
+++ b/kernel/profile.c
@@ -37,7 +37,7 @@ struct profile_hit {
 #define NR_PROFILE_GRP		(NR_PROFILE_HIT/PROFILE_GRPSZ)
 
 /* Oprofile timer tick hook */
-int (*timer_hook)(struct pt_regs *) __read_mostly;
+static int (*timer_hook)(struct pt_regs *) __read_mostly;
 
 static atomic_t *prof_buffer;
 static unsigned long prof_len, prof_shift;

Re: [PATCH] Some love to default profiler

[Andi Kleen]

Alexey Dobriyan <adobriyan@gmail.com> writes:

> 2) Put profile.c under CONFIG_PROFILING. You enabled profiling in config, you
>    will get it. Removes conditional branch from schedule(). Code savings on my
>    usual config:
> 
> 	   text	   data	    bss	    dec	    hex	filename
> 	2921871	 179895	 180224	3281990	 321446	vmlinux		before
> 	2920141	 179847	 180224	3280212	 320d54	vmlinux		after
> 	--------------------------------------------------------------
> 	  -1730     -48           -1778

I think it's better to keep it by default. 1.7k is not really all that much.

-Andi

Re: [PATCH] Some love to default profiler

[Jesper Juhl]

On 04/07/07, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 1) Drop __KERNEL__ out of profile.h. It contains only internal kernel stuff and
>    not in exported headers list

Even if it's not in the list of exported headers, does it really hurt
to retain that extra safeguard?

> 2) Put profile.c under CONFIG_PROFILING. You enabled profiling in config, you
>    will get it.

Makes logical sense. If the user didn't enable CONFIG_PROFILING why
should she get any profiling related overhead of any kind if we can
avoid it?

> Removes conditional branch from schedule(). Code savings on my
>    usual config:
>
>            text    data     bss     dec     hex filename
>         2921871  179895  180224 3281990  321446 vmlinux         before
>         2920141  179847  180224 3280212  320d54 vmlinux         after
>         --------------------------------------------------------------
>           -1730     -48           -1778
>
Nice savings there. Not that 1.7K is huge, but it's kernel memory is
precious :-)

> 3) Make timer_hook static (hi, Adrian!)

Makes perfect sense to me.

> 4) Convert do {} while (0) into static inline functions

Seems reasonable.

> 5) minor misc stuff
>

One tiny comment below.


> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
>  include/linux/profile.h |   78 +++++++++++++++++++++++++-----------------------
>  kernel/Makefile         |    3 +
>  kernel/profile.c        |    2 -
>  3 files changed, 44 insertions(+), 39 deletions(-)
>
[snip]
> +#define prof_on 0
> +static inline void profile_init(void)
> +{
> +}

Just to be pedantic; don't we want a blank line between functions
here, even if they are empty?

> +static inline void profile_tick(int type)
> +{
> +}
> +static inline void profile_hits(int type, void *ip, unsigned int nr_hits)
> +{
> +}
> +static inline void profile_hit(int type, void *ip)
> +{
> +}
> +static inline void profile_munmap(unsigned long addr)
> +{
> +}
> +static inline void profile_task_exit(struct task_struct *tsk)
> +{
> +}
> +static inline int profile_handoff_task(struct task_struct *tsk)
> +{
> +       return 0;
> +}
[snip]

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Some love to default profiler

[Denis Vlasenko]

On Thursday 05 July 2007 01:50, Jesper Juhl wrote:
> > Removes conditional branch from schedule(). Code savings on my
> >    usual config:
> >
> >            text    data     bss     dec     hex filename
> >         2921871  179895  180224 3281990  321446 vmlinux         before
> >         2920141  179847  180224 3280212  320d54 vmlinux         after
> >         --------------------------------------------------------------
> >           -1730     -48           -1778
>
> Nice savings there. Not that 1.7K is huge, but it's kernel memory is
> precious :-)

Hehe. In busybox project people can kill for 1.7K :)
--
vda

Re: [PATCH] Some love to default profiler

[Robert P. J. Day]

On Thu, 5 Jul 2007, Denis Vlasenko wrote:

> On Thursday 05 July 2007 01:50, Jesper Juhl wrote:
> > > Removes conditional branch from schedule(). Code savings on my
> > >    usual config:
> > >
> > >            text    data     bss     dec     hex filename
> > >         2921871  179895  180224 3281990  321446 vmlinux         before
> > >         2920141  179847  180224 3280212  320d54 vmlinux         after
> > >         --------------------------------------------------------------
> > >           -1730     -48           -1778
> >
> > Nice savings there. Not that 1.7K is huge, but it's kernel memory is
> > precious :-)
>
> Hehe. In busybox project people can kill for 1.7K :)
> --

true enough.  say ... whatever *did* you do with rob landley's body?
:-)

rday

p.s  that was, i say, that was a *joke*, son.
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

Re: [PATCH] Some love to default profiler

[Alexey Dobriyan]

On Thu, Jul 05, 2007 at 01:50:27AM +0200, Jesper Juhl wrote:
> One tiny comment below.

> >+#define prof_on 0
> >+static inline void profile_init(void)
> >+{
> >+}
>
> Just to be pedantic; don't we want a blank line between functions
> here, even if they are empty?

Dunno, it's boilerplate code, nobody reads it.

> >+static inline void profile_tick(int type)
> >+{
> >+}
> >+static inline void profile_hits(int type, void *ip, unsigned int nr_hits)
> >+{
> >+}
> >+static inline void profile_hit(int type, void *ip)
> >+{
> >+}
> >+static inline void profile_munmap(unsigned long addr)
> >+{
> >+}
> >+static inline void profile_task_exit(struct task_struct *tsk)
> >+{
> >+}
> >+static inline int profile_handoff_task(struct task_struct *tsk)
> >+{
> >+       return 0;
> >+}

Re: [PATCH] Some love to default profiler

[Adrian Bunk]

On Thu, Jul 05, 2007 at 01:50:27AM +0200, Jesper Juhl wrote:
> On 04/07/07, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> 1) Drop __KERNEL__ out of profile.h. It contains only internal kernel 
>> stuff and
>>    not in exported headers list
>
> Even if it's not in the list of exported headers, does it really hurt
> to retain that extra safeguard?
>...

Safeguard for what?

And it has to be consistent, and current policy is to not have
#ifdef __KERNEL__'s in all not to userspace exported headers.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] Some love to default profiler

[Jesper Juhl]

On 08/07/07, Adrian Bunk <bunk@stusta.de> wrote:
> On Thu, Jul 05, 2007 at 01:50:27AM +0200, Jesper Juhl wrote:
> > On 04/07/07, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >> 1) Drop __KERNEL__ out of profile.h. It contains only internal kernel
> >> stuff and
> >>    not in exported headers list
> >
> > Even if it's not in the list of exported headers, does it really hurt
> > to retain that extra safeguard?
> >...
>
> Safeguard for what?
>
Well, for userspace code that tries to include the file.  I know
nobody should, but somebody still might.   At least that was my
thought when I made that comment.

> And it has to be consistent, and current policy is to not have
> #ifdef __KERNEL__'s in all not to userspace exported headers.
>
Ok, in that case it should go away.


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Some love to default profiler

[Matt Mackall]

On Thu, Jul 05, 2007 at 01:34:20AM +0400, Alexey Dobriyan wrote:
> 1) Drop __KERNEL__ out of profile.h. It contains only internal kernel stuff and
>    not in exported headers list
> 2) Put profile.c under CONFIG_PROFILING. You enabled profiling in config, you
>    will get it. Removes conditional branch from schedule(). Code savings on my
>    usual config:
> 
> 	   text	   data	    bss	    dec	    hex	filename
> 	2921871	 179895	 180224	3281990	 321446	vmlinux		before
> 	2920141	 179847	 180224	3280212	 320d54	vmlinux		after
> 	--------------------------------------------------------------
> 	  -1730     -48           -1778
> 
> 3) Make timer_hook static (hi, Adrian!)
> 4) Convert do {} while (0) into static inline functions
> 5) minor misc stuff

Something here suggests this should have been 5 patches.

Otherwise, looks good to me.

-- 
Mathematics is the supreme nostalgia of our time.