On 03/21/2014 01:28 PM, Viresh Kumar wrote: > On 21 March 2014 13:16, Srivatsa S. Bhat > <srivatsa.bhat@xxxxxxxxxxxxxxxxxx> wrote: >> We need this assignment to happen exactly at this point, that is, *after* >> the call to post_transition() completes and before calling wake_up(). >> >> If the compiler or the CPU reorders the instructions and moves this >> assignment to some other place, then we will be in trouble! >> >> We might need memory barriers to ensure this doesn't get reordered. >> Alternatively, we can simply hold the spin-lock around this assignment, >> since locks automatically imply memory barriers. As an added advantage, >> the code will then look more intuitive and easier to understand as well. >> >> Thoughts? > > I may be wrong but this is how I understand locks. Yes, spinlocks have > memory barriers implemented but it wouldn't guarantee what you are > asking for in the above explanation. > > It will guarantee that transition_ongoing will be updated after the lock > is taken but the notification() can happen after the lock is taken and > also after the variable is modified. > Actually, yes, that's true. The lock and unlock act as one-way barriers, hence they ensure that the critical section doesn't seep outside of the locks, but don't necessarily ensure that pieces of code outside the critical section don't seep -into- the critical section. IOW, my reasoning was not quite correct, but see below. > You can find some information on this in > Documentation/memory-barriers.txt > Yep, I know, I have read it several times, but I'm no expert ;-) I found this interesting section on "SLEEP AND WAKE-UP FUNCTIONS". It says that doing: policy->transition_ongoing = false; wake_up(&policy->transition_wait); is safe (as long as some tasks are woken up). So we don't have to worry about that part. So only the first part remains to be solved: ensuring that the assignment occurs _after_ completing the invocation of the POSTCHANGE notifiers. For that, we can do: cpufreq_notify_post_transition(); smp_mb(); policy->transition_ongoing = false; That should take care of everything. > I don't think compiler or CPU will reorder calls to a function and > updates of a variable. I'm not sure about that. I think it is free to do so if it finds that there is no dependency that prevents it from reordering. In this case the update to the flag has no "visible" dependency on the call to post_transition(). > And so this code might simply work. And > I hope there would be plenty of such code in kernel. > Sure, there are plenty of examples in the kernel where we call functions and update variables. But in this particular case, our synchronization _depends_ on those operations happening in a particular order. Hence we need to ensure the ordering is right. Otherwise the synchronization might get broken. Here are some examples where memory barriers are inserted to avoid reordering of variable updates and function calls: kernel/rcu/torture.c: rcu_torture_barrier_cbs() kernel/smp.c: kick_all_cpus_sync() Regards, Srivatsa S. Bhat -- To unsubscribe from this list: send the line "unsubscribe cpufreq" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
