On Fri, Dec 02, 2016 at 06:18:39PM -0800, Davidlohr Bueso wrote:
> @@ -102,8 +103,13 @@ void __percpu_up_read(struct percpu_rw_semaphore *sem)
> */
> __this_cpu_dec(*sem->read_count);
>
> + rcu_read_lock();
> + writer = rcu_dereference(sem->writer);
Don't think this is correct, I think Oleg suggested using
task_rcu_dereference(), which is a giant pile of magic.
The problem is that task_struct isn't RCU protected as such.
> +
> /* Prod writer to recheck readers_active */
> - wake_up(&sem->writer);
> + if (writer)
> + wake_up_process(writer);
> + rcu_read_unlock();
> }
> EXPORT_SYMBOL_GPL(__percpu_up_read);
>
> @@ -159,8 +165,18 @@ void percpu_down_write(struct percpu_rw_semaphore *sem)
> * will wait for them.
> */
>
> - /* Wait for all now active readers to complete. */
> - wait_event(sem->writer, readers_active_check(sem));
> + WRITE_ONCE(sem->writer, current);
So this one matches rcu_dereference(), which is weird, because you now
have unmatched barriers.
> + for (;;) {
> + set_current_state(TASK_UNINTERRUPTIBLE);
> +
> + if (readers_active_check(sem))
> + break;
> +
> + schedule();
> + }
> +
> + rcu_assign_pointer(sem->writer, NULL);
And this one does not, and the value being NULL this actually reverts to
WRITE_ONCE().
> + __set_current_state(TASK_RUNNING);
> }
> EXPORT_SYMBOL_GPL(percpu_down_write);
