From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751451AbdFFMCE (ORCPT ); Tue, 6 Jun 2017 08:02:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42322 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751345AbdFFMCC (ORCPT ); Tue, 6 Jun 2017 08:02:02 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 439092628 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=pbonzini@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 439092628 Subject: Re: [PATCH RFC tip/core/rcu 1/2] srcu: Allow use of Tiny/Tree SRCU from both process and interrupt context To: Peter Zijlstra , "Paul E. McKenney" Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, jiangshanlai@gmail.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, fweisbec@gmail.com, oleg@redhat.com, kvm@vger.kernel.org, Linus Torvalds References: <20170605220919.GA27820@linux.vnet.ibm.com> <1496700591-30177-1-git-send-email-paulmck@linux.vnet.ibm.com> <20170606110915.g7zlzoj5iidhpp7g@hirez.programming.kicks-ass.net> From: Paolo Bonzini Message-ID: Date: Tue, 6 Jun 2017 14:01:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <20170606110915.g7zlzoj5iidhpp7g@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 06 Jun 2017 12:02:02 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06/06/2017 13:09, Peter Zijlstra wrote: >> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c >> index 36e1f82faed1..681bf6bc04a5 100644 >> --- a/kernel/rcu/srcutiny.c >> +++ b/kernel/rcu/srcutiny.c >> @@ -35,8 +35,8 @@ >> >> static int init_srcu_struct_fields(struct srcu_struct *sp) >> { >> - sp->srcu_lock_nesting[0] = 0; >> - sp->srcu_lock_nesting[1] = 0; >> + atomic_set(&sp->srcu_lock_nesting[0], 0); >> + atomic_set(&sp->srcu_lock_nesting[1], 0); >> init_swait_queue_head(&sp->srcu_wq); >> sp->srcu_gp_seq = 0; >> rcu_segcblist_init(&sp->srcu_cblist); >> @@ -86,7 +86,8 @@ EXPORT_SYMBOL_GPL(init_srcu_struct); >> */ >> void cleanup_srcu_struct(struct srcu_struct *sp) >> { >> - WARN_ON(sp->srcu_lock_nesting[0] || sp->srcu_lock_nesting[1]); >> + WARN_ON(atomic_read(&sp->srcu_lock_nesting[0]) || >> + atomic_read(&sp->srcu_lock_nesting[1])); >> flush_work(&sp->srcu_work); >> WARN_ON(rcu_seq_state(sp->srcu_gp_seq)); >> WARN_ON(sp->srcu_gp_running); >> @@ -97,7 +98,7 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct); >> >> /* >> * Counts the new reader in the appropriate per-CPU element of the >> - * srcu_struct. Must be called from process context. >> + * srcu_struct. >> * Returns an index that must be passed to the matching srcu_read_unlock(). >> */ >> int __srcu_read_lock(struct srcu_struct *sp) >> @@ -105,21 +106,19 @@ int __srcu_read_lock(struct srcu_struct *sp) >> int idx; >> >> idx = READ_ONCE(sp->srcu_idx); >> - WRITE_ONCE(sp->srcu_lock_nesting[idx], sp->srcu_lock_nesting[idx] + 1); >> + atomic_inc(&sp->srcu_lock_nesting[idx]); >> return idx; >> } >> EXPORT_SYMBOL_GPL(__srcu_read_lock); >> >> /* >> * Removes the count for the old reader from the appropriate element of >> - * the srcu_struct. Must be called from process context. >> + * the srcu_struct. >> */ >> void __srcu_read_unlock(struct srcu_struct *sp, int idx) >> { >> - int newval = sp->srcu_lock_nesting[idx] - 1; >> - >> - WRITE_ONCE(sp->srcu_lock_nesting[idx], newval); >> - if (!newval && READ_ONCE(sp->srcu_gp_waiting)) >> + if (atomic_dec_return_relaxed(&sp->srcu_lock_nesting[idx]) == 0 && >> + READ_ONCE(sp->srcu_gp_waiting)) >> swake_up(&sp->srcu_wq); >> } >> EXPORT_SYMBOL_GPL(__srcu_read_unlock); >> @@ -148,7 +147,7 @@ void srcu_drive_gp(struct work_struct *wp) >> idx = sp->srcu_idx; >> WRITE_ONCE(sp->srcu_idx, !sp->srcu_idx); >> WRITE_ONCE(sp->srcu_gp_waiting, true); /* srcu_read_unlock() wakes! */ >> - swait_event(sp->srcu_wq, !READ_ONCE(sp->srcu_lock_nesting[idx])); >> + swait_event(sp->srcu_wq, !atomic_read(&sp->srcu_lock_nesting[idx])); >> WRITE_ONCE(sp->srcu_gp_waiting, false); /* srcu_read_unlock() cheap. */ >> rcu_seq_end(&sp->srcu_gp_seq); > > I'm not entirely sure this is actually needed. TINY_SRCU is !PREEMPT && > !SMP. So that means all we need is to be safe from IRQs. > > Now, do we (want) support things like: > > > srcu_read_lock(); > > > srcu_read_lock(); > > srcu_read_unlock(); > > > srcu_read_unlock(); > > > > _OR_ > > do we already (or want to) mandate that SRCU usage in IRQs must be > balanced? That is, if it is used from IRQ context it must do an equal > amount of srcu_read_lock() and srcu_read_unlock()s? > > Because if we have the balance requirement (as we do for > preempt_disable()) then even on load-store architectures the current > code should be sufficient (since if an interrupt does as many dec's as > it does inc's, the actual value will not change over an interrupt, and > our load from before the interrupt is still valid). Good point! So the srcutiny part should not be necessary. I'll reply to the other email now. Paolo