Message-ID: <20090624142828.7c6a2337@skybase>
Date:	Wed, 24 Jun 2009 14:28:28 +0200
From:	Martin Schwidefsky <schwidefsky@...ibm.com>
To:	Paul Mundt <lethal@...ux-sh.org>
Cc:	Ingo Molnar <mingo@...e.hu>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: register_timer_hook use in arch/sh/oprofile

On Wed, 24 Jun 2009 20:29:29 +0900
Paul Mundt <lethal@...ux-sh.org> wrote:

> On Wed, Jun 24, 2009 at 01:11:04PM +0200, Martin Schwidefsky wrote:
> > Hi Paul,
> > following Ingo's suggestion I'm working on a patch to use a per-cpu
> > hrtimer instead of the timer_hook for the oprofile ticks. Given that
> > oprofile is the only user of the timer_hook I want to remove it
> > completely. That way I came across the register_timer_hook call in
> > arch/sh/oprofile/op_model_sh7750.c. Did this ever work? The startup
> > of oprofile is done in two steps, on module load oprofile_init is
> > called, on profiling start oprofile_start is called.
> > Now for SH7750 this happens:
> > 
> > oprofile_init()
> > 	...
> > 	oprofile_arch_init(&oprofile_ops);
> > 		...
> > 		lmodel->init(); 
> > 		/* init() is sh7750_ppc_init for CPU_SH7750/CPU_7750S */
> > 			sh7750_ppc_reset();
> > 			register_timer_hook(sh7750_timer_notify);
> > 			...
> > 		...
> > 	oprofile_timer_init(&oprofile_ops);
> > 		...
> > 		ops->start = timer_start;
> > 		...
> > 	...
> > 
> > oprofile_start()
> > 	...
> > 	oprofile_ops.start();
> > 	/* start() is timer_start set by oprofile_timer_init */
> > 		register_timer_hook(timer_notify);
> > 	...
> > 
> > As far as I can see the second register_timer_hook will fail
> > because sh7750_timer_notify is already registered. That will cause
> > oprofile_start to fail with -EBUSY, no?
> > 
> No. oprofile_timer_init() is only entered if the performance counters
> fail to register in the SH7750 case, so there is only one timer hook user
> at a time:
> 
> static int __init oprofile_init(void)
> {
>         int err;
> 
>         err = oprofile_arch_init(&oprofile_ops);
> 
>         if (err < 0 || timer) {
>                 printk(KERN_INFO "oprofile: using timer interrupt.\n");
>                 oprofile_timer_init(&oprofile_ops);
>         }
> ...

Oh, I see. That is the reason why the s390 version of
oprofile_arch_init returns -ENODEV. It does so to trigger the fallback
to the timer_hook. That should work for sh as well, no?

> The sh7750 performance counter code has always purposely abused the timer
> interrupt as its profiling interrupt given that the counters themselves do not
> generate IRQs on their own. There are a couple of 64-bit counters that can
> count various events, but have no real event associated with them. As long as
> they are periodically read, they return accurate statistics, but the granularity
> is never better than the timer IRQ.
> 
> In practice oprofile has never been a good fit for these sorts of counters, so
> this has fairly limited use. If there's a way to wiggle these types of counters
> in to the new perf_counter API, then I'll convert that over and just kill the
> old oprofile driver off completely. Barring that, I'll just end up converting it
> over to hrtimers as well, so don't let that stop you from ripping out the timer
> hook bits.
> 
> Most of this code predates hrtimers anyways, and it also predates the timer
> hook, which is only something that we converted to some years back.

Consider the timer_hook as history :-)

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
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