Re: [PATCH v2 RESEND/RFC] timer: make deferrable cpu unbound timers really not bound to a cpu

[Saravana Kannan <skannan@codeaurora.org>]

On 03/19/2015 05:10 PM, Saravana Kannan wrote:
> On 09/23/2014 11:33 AM, Thomas Gleixner wrote:
>> On Mon, 15 Sep 2014, Joonwoo Park wrote:
>>> +#ifdef CONFIG_SMP
>>> +static struct tvec_base *tvec_base_deferral = &boot_tvec_bases;
>>> +#endif
>>
>> In principle I like the idea of a deferrable wheel, but this
>> implementation is going to go nowhere.
>
> Hi Thomas,
>
> To give some more context:
>
> This bug is a serious pain in the a** for anyone using deferrable timers
> or deferrable workqueues today for some periodic work and don't care for
> which CPU the code runs in.
>
> Couple of examples of such issues in existing code:
>
> 1) In a SMP system, CPUfreq governors (ondemand and conservative) end up
> queueing a deferrable work on every single CPU and the first one to run
> the deferrable workqueue cancels the work on all other CPUS, runs the
> work and then sets up a workqueue on all the CPUs again for the next
> sampling point.
>
> 2) Devfreq actually doesn't work well today because it doesn't do this
> nasty hack like CPUfreq. So, if the devfreq work happens to run on a CPU
> that's typically idle, then it doesn't run for a long time. I've
> actually seen logs where the devfreq polling interval is set to 20ms,
> but ends up running 800ms later.
>
> Don't know how many other drivers are suffering from this bug. Yes,
> IMHO, this is a bug. When the timer is CPU unbound, anyone who hasn't
> looked at the actual timer code would assume that it'll run as long as
> any CPU is busy.
>
>> First of all making it SMP only is silly. The deferrable stuff is a
>> pain in other places as well.
>>
>> But whats way worse is:
>>
>>> +static inline void __run_timers(struct tvec_base *base, bool try)
>>>   {
>>>       struct timer_list *timer;
>>>
>>> -    spin_lock_irq(&base->lock);
>>> +    if (!try)
>>> +        spin_lock_irq(&base->lock);
>>> +    else if (!spin_trylock_irq(&base->lock))
>>> +        return;
>>
>> Yuck. All cpus fighting about a single spinlock roughly at the same
>> time? You just created a proper thundering herd cacheline bouncing
>> issue.
>
> I agree, This is not good. I think a simple way to fix this is to have
> only the CPU that's the jiffy owner to run through this deferrable timer
> list.
>
> That should address any unnecessary cache bouncing issues. Would that be
> an acceptable solution to this?
>
>>
>> No way. We have already mechanisms in place to deal with such
>> problems, you just have to use them.
>
> I'm not sure which problem you are referring to here. Or what the
> already existing solutions are.
>
> I don't think you were referring to the "deferrable timer getting
> delayed for long periods despite CPUs being active" problem, because I
> don't think we have a better solution than this patch (with the jiffy
> owner CPU fix). Asking every driver that doesn't care which CPU the work
> runs on to queue a work or set up a timer on every CPU is definitely not
> a good solution -- that's spreading the complexity to every other driver
> instead of fixing it in one place. And that causes unnecessary cache
> pollution too.
>
> Thoughts? I would really like to see a fix for this go in soon so that
> we can simplify cpufreq and have devfreq and other drivers work correctly.
>
> Thanks,
> Saravana
>

Thomas,

Bump.

-Saravana

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