> The compiler is within its rights to optimize the active_writer local
> variable out of existence, thus re-introducing the possible race with
> the writer that can pass a NULL pointer to wake_up_process(). So you
> really need the ACCESS_ONCE() on the read from cpu_hotplug.active_writer.
> Please see http://lwn.net/Articles/508991/ for more information why
> this is absolutely required.
You're absolutely right, saw your reply on the other patch just after I sent
this version ...
So if you agree with the change below, I'll send an updated version!
>
> > + if (unlikely(active_writer))
> > + wake_up_process(active_writer);
> > cpuhp_lock_release();
> > return;
> > }
> > @@ -161,15 +167,17 @@ void cpu_hotplug_begin(void)
> > cpuhp_lock_acquire();
> > for (;;) {
> > mutex_lock(&cpu_hotplug.lock);
> > + __set_current_state(TASK_UNINTERRUPTIBLE);
>
> You lost me on this one. How does this help?
>
> Thanx, Paul
Imagine e.g. the following (simplified) scenario:
CPU1 CPU2
----------------------------------------------------------------------------
!mutex_trylock(&cpu_hotplug.lock) |
| cpu_hotplug.puts_pending == 0
cpu_hotplug.puts_pending++; |
| cpu_hotplug.refcount != 0
wake_up_process(active_writer)
| __set_current_state(TASK_UNINTERRUPTIBLE);
| schedule();
| /* will never be woken up */
Therefore we have to move the condition check inside the
__set_current_state(TASK_UNINTERRUPTIBLE) -> schedule();
section to not miss any wake ups when the condition is satisfied.
So wake_up_process() will either see TASK_RUNNING and do nothing or see
TASK_UNINTERRUPTIBLE and set it to TASK_RUNNING, so schedule() will in
fact be woken up again.
Thanks a lot!
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
