Messages in this thread Patch in this message | | | Date | Mon, 18 Nov 2019 15:19:35 -0800 | From | Davidlohr Bueso <> | Subject | Re: [PATCH 5/5] locking/percpu-rwsem: Remove the embedded rwsem |
| |
On Mon, 18 Nov 2019, Davidlohr Bueso wrote:
>On Wed, 13 Nov 2019, Peter Zijlstra wrote: 3>>bool __percpu_down_read(struct percpu_rw_semaphore *sem, bool try) >>{ >> if (__percpu_down_read_trylock(sem)) >>@@ -89,20 +156,10 @@ bool __percpu_down_read(struct percpu_rw >> if (try) >> return false; >> >>- /* >>- * We either call schedule() in the wait, or we'll fall through >>- * and reschedule on the preempt_enable() in percpu_down_read(). >>- */ >>- preempt_enable_no_resched(); >>- >>- /* >>- * Avoid lockdep for the down/up_read() we already have them. >>- */ >>- __down_read(&sem->rw_sem); >>- this_cpu_inc(*sem->read_count); >>- __up_read(&sem->rw_sem); >>- >>+ preempt_enable(); >>+ percpu_rwsem_wait(sem, /* .reader = */ true ); >> preempt_disable(); >>+ >> return true; >>} >>EXPORT_SYMBOL_GPL(__percpu_down_read); > >Do we really need to export symbol here? This function is only called >from percpu-rwsem.h.
Similarly, afaict we can get rid of __percpu_up_read() and put the slowpath all into percpu_up_read(). Also explicitly mention the single task nature of the writer (which is a better comment for the rcuwait_wake_up()).
diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h index f5ecf6a8a1dd..eda545f42fb8 100644 --- a/include/linux/percpu-rwsem.h +++ b/include/linux/percpu-rwsem.h @@ -43,7 +43,6 @@ is_static struct percpu_rw_semaphore name = { \ __DEFINE_PERCPU_RWSEM(name, static) extern bool __percpu_down_read(struct percpu_rw_semaphore *, bool); -extern void __percpu_up_read(struct percpu_rw_semaphore *); static inline void percpu_down_read(struct percpu_rw_semaphore *sem) { @@ -103,10 +102,23 @@ static inline void percpu_up_read(struct percpu_rw_semaphore *sem) /* * Same as in percpu_down_read(). */ - if (likely(rcu_sync_is_idle(&sem->rss))) + if (likely(rcu_sync_is_idle(&sem->rss))) { __this_cpu_dec(*sem->read_count); - else - __percpu_up_read(sem); /* Unconditional memory barrier */ + goto done; + } + + /* + * slowpath; reader will only ever wake a single blocked writer. + */ + smp_mb(); /* B matches C */ + /* + * In other words, if they see our decrement (presumably to + * aggregate zero, as that is the only time it matters) they + * will also see our critical section. + */ + __this_cpu_dec(*sem->read_count); + rcuwait_wake_up(&sem->writer); +done: preempt_enable(); } diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c index 851038468efb..a5150a876626 100644 --- a/kernel/locking/percpu-rwsem.c +++ b/kernel/locking/percpu-rwsem.c @@ -164,21 +164,6 @@ bool __percpu_down_read(struct percpu_rw_semaphore *sem, bool try) } EXPORT_SYMBOL_GPL(__percpu_down_read); -void __percpu_up_read(struct percpu_rw_semaphore *sem) -{ - smp_mb(); /* B matches C */ - /* - * In other words, if they see our decrement (presumably to aggregate - * zero, as that is the only time it matters) they will also see our - * critical section. - */ - __this_cpu_dec(*sem->read_count); - - /* Prod writer to re-evaluate readers_active_check() */ - rcuwait_wake_up(&sem->writer); -} -EXPORT_SYMBOL_GPL(__percpu_up_read); - #define per_cpu_sum(var) \ ({ \ typeof(var) __sum = 0; \
| |