Re: [PATCH v2] timers: Clear must_forward_clk inside base lock

["Kohli, Gaurav" <gkohli@codeaurora.org>]

On 8/2/2018 12:04 PM, Thomas Gleixner wrote:
> On Thu, 2 Aug 2018, Gaurav Kohli wrote:
>> Timer wheel base->must_forward_clock is indicating that
>> the base clock might be stale due to a long idle sleep.
>> The forwarding of base clock takes place in softirq of timer
> 
> of the base clock takes place in the timer softirq ...
> 
>> or when a timer is enqueued to base which is idle. While migrate
> 
> to a base ..
> 
>> timer from remote CPU to the new base which is idle, then
> 
> See below.
> 
>> following race can happen:
>>
>>    CPU0                                  CPU1
>>    run_timer_softirq                     timers_dead_cpu
>>
>> 					base = lock_timer_base(timer);
>>    base->must_forward_clk = false
>> 					if (base->must_forward_clk)
>> 				         forward(base); >>skip
>>
>> 					migrate_timer_list
> 
> I don't know why you insist on migrate_timer_list() being part of the
> picture here.

Hi Thomas,

Thanks for comment.
I agree this can come with normal enqueue of timer as well, will make it 
more generic to avoid confuzion and upload new patch for same.

Regards
Gaurav


> 
> It's only _ONE_ particular way to observe that issue. But it's not the only
> way. ANY remote enqueue which hits the situation on the other CPU (CPU0 in
> the example) has this problem. Tying it to migrate_timer_list() just
> because you observed it that way is actively misleading. Surely you can add
> a sentence that you observed it in that case, but that's supplemental
> information.
> 
> Thanks,
> 
> 	tglx
> 

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, 
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.