Re: [PATCH V4 1/3] cpufreq: Make sure frequency transitions are serialized

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 03/21/2014 04:35 PM, Catalin Marinas wrote:
> On Fri, Mar 21, 2014 at 09:21:02AM +0000, Viresh Kumar wrote:
>> @Catalin: We have a problem here and need your expert advice. After changing
>> CPU frequency we need to call this code:
>>
>> cpufreq_notify_post_transition();
>> policy->transition_ongoing = false;
>>
>> And the sequence must be like this only. Is this guaranteed without any
>> memory barriers? cpufreq_notify_post_transition() isn't touching
>> transition_ongoing at all..
> 
> The above sequence doesn't say much. As rmk said, the compiler wouldn't
> reorder the transition_ongoing write before the function call. I think
> most architectures (not sure about Alpha) don't do speculative stores,
> so hardware wouldn't reorder them either. However, other stores inside
> the cpufreq_notify_post_transition() could be reordered after
> transition_ongoing store. The same for memory accesses after the
> transition_ongoing update, they could be reordered before.
> 
> So what we actually need to know is what are the other relevant memory
> accesses that require strict ordering with transition_ongoing.
>

Hmm.. The thing is, _everything_ inside the post_transition() function
should complete before writing to transition_ongoing. Because, setting the
flag to 'false' indicates the end of the critical section, and the next
contending task can enter the critical section.

So, I think we should use smp_mb() before setting transition_ongoing = false.
That way we'll be safe.

> What I find strange in your patch is that
> cpufreq_freq_transition_begin() uses spinlocks around transition_ongoing
> update but cpufreq_freq_transition_end() doesn't.
> 

The reason is that, by the time we drop the spinlock, we would have set
the transition_ongoing flag to true, which prevents any other task from
entering the critical section. Hence, when we call the _end() function,
we are 100% sure that only one task is executing it. Hence locks are not
necessary around that second update. In fact, that very update marks the
end of the critical section (which acts much like a spin_unlock(&lock)
in a "regular" critical section).

I know the "critical section" and the synchronization used in this patch
is a bit unconventional, but that's because the scenario itself is
unconventional : we need to able to start the critical section in one
task, and end it in another task! That's where all the complication
arises :-)  It sounds weird, but in this cpufreq case, its actually valid
and surprisingly, makes sense too! :-)

Regards,
Srivatsa S. Bhat