Messages in this thread | | | Date | Tue, 29 Oct 2019 19:47:39 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH] locking/percpu_rwsem: Rewrite to not use rwsem |
| |
On Wed, Aug 07, 2019 at 11:56:58AM +0200, Oleg Nesterov wrote:
> and either way, with or without 2 queues, what do you think about the code > below?
Sorry for being so tardy with this thread.. having once again picked up the patch, I found your email.
> This way the new reader does wake_up() only in the very unlikely case when > it races with the new writer which sets sem->block = 1 right after > this_cpu_inc().
Ah, by waiting early, you avoid spurious wakeups when __percpu_down_read() happens after a successful percpu_down_write(). Nice!
I've made these changes. Now let me go have a play with that second waitqueue.
> ------------------------------------------------------------------------------- > > static inline void percpu_down_read(struct percpu_rw_semaphore *sem) > { > might_sleep(); > rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_); > > preempt_disable(); > > if (likely(rcu_sync_is_idle(&sem->rss))) > __this_cpu_inc(*sem->read_count); > else > __percpu_down_read(sem, false); > > preempt_enable(); > } > > static inline void percpu_up_read(struct percpu_rw_semaphore *sem) > { > rwsem_release(&sem->dep_map, 1, _RET_IP_); > > preempt_disable(); > > if (likely(rcu_sync_is_idle(&sem->rss))) > __this_cpu_dec(*sem->read_count); > else > __percpu_up_read(sem); > > preempt_enable(); > }
I like that symmetry, but see below ...
> // both called and return with preemption disabled > > bool __percpu_down_read(struct percpu_rw_semaphore *sem, bool try) > { > > if (atomic_read_acquire(&sem->block)) { > again: > preempt_enable(); > __wait_event(sem->waiters, !atomic_read_acquire(&sem->block)); > preempt_disable(); > } > > __this_cpu_inc(*sem->read_count); > > smp_mb(); > > if (likely(!atomic_read_acquire(&sem->block))) > return true; > > __percpu_up_read(sem); > > if (try) > return false; > > goto again; > } > > void __percpu_up_read(struct percpu_rw_semaphore *sem) > { > smp_mb(); > > __this_cpu_dec(*sem->read_count); > preempt_enable(); > wake_up(&sem->waiters); preempt_disable()
and this (sadly) means there's a bunch of back-to-back preempt_disable()+preempt_enable() calls. Leaving out the preempt_disable() here makes it ugly again :/
Admittedly, this is PREEMPT_RT only, but given that is >< close to mainline we'd better get it right.
> } >
| |