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

[Stephen Boyd]

On 07/22/13 13:48, John Stultz wrote:
> 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?
>

Yes that's fine to fold it in.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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