Re: [PATCH v4 03/17] sched_clock: Use an hrtimer instead of timer

[John Stultz]

On 07/22/2013 11:58 AM, Stephen Boyd wrote:
> On 07/22/13 11:45, Stephen Boyd wrote:
> > On 07/22/13 11:21, John Stultz wrote:
> > > On 07/18/2013 04:21 PM, Stephen Boyd wrote:
> > > > In the next patch we're going to increase the number of bits that
> > > > the generic sched_clock can handle to be greater than 32. With
> > > > more than 32 bits the wraparound time can be larger than what can
> > > > fit into the units that msecs_to_jiffies takes (unsigned int).
> > > > Luckily, the wraparound is initially calculated in nanoseconds
> > > > which we can easily use with hrtimers, so switch to using an
> > > > hrtimer.
> > > >
> > > > Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> > > Hrmm. So in my testing (under qemu), this patch causes bootup to hang.
> > >
> > > qemu-system-arm -kernel zImage-arm -M vexpress-a9 -cpu cortex-a9
> > > -nographic -m 1024  -append 'root=/dev/mmcblk0p2 rw mem=1024M
> > > raid=noautodetect console=ttyAMA0,38400n8 rootwait vmalloc=256MB
> > > devtmpfs.mount=0' -sd test-arm.img -redir tcp:4300::22
> > >
> > > Config file attached.
> > >
> > > I haven't gotten a chance to look very closely, but it seems the
> > > folowing patch resolves the issue. I'm not sure if we're seeing
> > > callers to setup_sched_clock happen after sched_clock_postinit or
> > > what, but it probably needs another look over.
> > >
> > > thanks
> > > -john
> > >
> > >
> > > diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> > > index a269890b..c018ffc 100644
> > > --- a/kernel/time/sched_clock.c
> > > +++ b/kernel/time/sched_clock.c
> > > @@ -138,12 +138,6 @@ void __init setup_sched_clock(u32 (*read)(void),
> > > int bits, unsigned long rate)
> > >       pr_info("sched_clock: %u bits at %lu%cHz, resolution %lluns,
> > > wraps every %lluns\n",
> > >           bits, r, r_unit, res, wrap);
> > >   
> > > -    /*
> > > -     * Start the timer to keep sched_clock() properly updated and
> > > -     * sets the initial epoch.
> > > -     */
> > > -    hrtimer_init(&sched_clock_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > > -    sched_clock_timer.function = sched_clock_poll;
> > >       update_sched_clock();
> > >   
> > >       /*
> > > @@ -175,6 +169,13 @@ void __init sched_clock_postinit(void)
> > >           setup_sched_clock(jiffy_sched_clock_read, 32, HZ);
> > >   
> > >       update_sched_clock();
> > > +
> > > +    /*
> > > +     * Start the timer to keep sched_clock() properly updated and
> > > +     * sets the initial epoch.
> > > +     */
> > > +    hrtimer_init(&sched_clock_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > > +    sched_clock_timer.function = sched_clock_poll;
> > >       hrtimer_start(&sched_clock_timer, cd.wrap_kt, HRTIMER_MODE_REL);
> > >   }
> > >   
> > Hmm. Is it too early to use hrtimers? Moving the hrtimer_start() into
> > sched_clock_register() also causes the same crash.
> Yes that seems to be the problem. The vexpress board is setting up the
> sched_clock in setup_arch() (via v2m_init_early) which runs before
> hrtimers_init(). I've only tested this on boards that setup the timer in
> the time_init() callback which runs after hrtimers_init(). Your patch
> should be fine, although it would be nice if we didn't have callers
> setting up the sched_clock so early.

Although as Russell pointed out, setting up sched_clock later isn't 
really a good option (although I'd like it best if we could handle 
switching sched_clocks dynamically as needed so there were less 
constraints on when it has to be registered).

So I'll probably fold in my change into the patch if you're ok with that?

thanks
-john

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/