Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751867AbdITOxA (ORCPT ); Wed, 20 Sep 2017 10:53:00 -0400 Received: from mx2.suse.de ([195.135.220.15]:54157 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751773AbdITOw7 (ORCPT ); Wed, 20 Sep 2017 10:52:59 -0400 Date: Wed, 20 Sep 2017 07:52:54 -0700 From: Davidlohr Bueso To: Prateek Sood Cc: mingo@kernel.org, longman@redhat.com, peterz@infradead.org, parri.andrea@gmail.com, linux-kernel@vger.kernel.org, sramana@codeaurora.org Subject: Re: [PATCH] rwsem: fix missed wakeup due to reordering of load Message-ID: <20170920145254.GA3406@linux-80c1.suse> References: <1504794658-15397-1-git-send-email-prsood@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <1504794658-15397-1-git-send-email-prsood@codeaurora.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1481 Lines: 45 On Thu, 07 Sep 2017, Prateek Sood wrote: > /* >+ * __rwsem_down_write_failed_common(sem) >+ * rwsem_optimistic_spin(sem) >+ * osq_unlock(sem->osq) >+ * ... >+ * atomic_long_add_return(&sem->count) >+ * >+ * - VS - >+ * >+ * __up_write() >+ * if (atomic_long_sub_return_release(&sem->count) < 0) >+ * rwsem_wake(sem) >+ * osq_is_locked(&sem->osq) >+ * >+ * And __up_write() must observe !osq_is_locked() when it observes the >+ * atomic_long_add_return() in order to not miss a wakeup. >+ * >+ * This boils down to: >+ * >+ * [S.rel] X = 1 [RmW] r0 = (Y += 0) >+ * MB RMB >+ * [RmW] Y += 1 [L] r1 = X >+ * >+ * exists (r0=1 /\ r1=0) >+ */ >+ smp_rmb(); Instead, how about just removing the release from atomic_long_sub_return_release() such that the osq load is not hoisted over the atomic compound (along with Peter's comment): diff --git a/include/asm-generic/rwsem.h b/include/asm-generic/rwsem.h index 6c6a2141f271..487ce31078ff 100644 --- a/include/asm-generic/rwsem.h +++ b/include/asm-generic/rwsem.h @@ -101,7 +101,7 @@ static inline void __up_read(struct rw_semaphore *sem) */ static inline void __up_write(struct rw_semaphore *sem) { - if (unlikely(atomic_long_sub_return_release(RWSEM_ACTIVE_WRITE_BIAS, + if (unlikely(atomic_long_sub_return(RWSEM_ACTIVE_WRITE_BIAS, &sem->count) < 0)) rwsem_wake(sem); }