From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753592AbcGUReG (ORCPT ); Thu, 21 Jul 2016 13:34:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53368 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753476AbcGUReD (ORCPT ); Thu, 21 Jul 2016 13:34:03 -0400 Date: Thu, 21 Jul 2016 19:34:13 +0200 From: Oleg Nesterov To: "Paul E. McKenney" Cc: Peter Zijlstra , mingo@kernel.org, linux-kernel@vger.kernel.org, tj@kernel.org, john.stultz@linaro.org, dimitrysh@google.com, romlem@google.com, ccross@google.com, tkjos@google.com Subject: Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter() Message-ID: <20160721173412.GA22488@redhat.com> References: <20160715132709.GA27644@redhat.com> <20160715133928.GH7094@linux.vnet.ibm.com> <20160715134523.GB28589@redhat.com> <20160715153840.GI7094@linux.vnet.ibm.com> <20160715164938.GA4294@redhat.com> <20160715180140.GM7094@linux.vnet.ibm.com> <20160716171007.GA30000@redhat.com> <20160719205018.GB7094@linux.vnet.ibm.com> <20160720151357.GA32397@redhat.com> <20160720205851.GL7094@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160720205851.GL7094@linux.vnet.ibm.com> User-Agent: Mutt/1.5.18 (2008-05-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 21 Jul 2016 17:34:03 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/20, Paul E. McKenney wrote: > > On Wed, Jul 20, 2016 at 05:13:58PM +0200, Oleg Nesterov wrote: > > > rcu_sync_enter() or __rcu_sync_enter() is legal in any state, the latter > > won't block. > > Actually, I had no idea that __rcu_sync_enter() was intended for anything > other than internal use. > > Other than that, agreed, with the exception that it is illegal after > rcu_sync_dtor() has been called. Yes, sure. rcu_sync_dtor() "destroys" struct rcu_sync, and currently it is only called by destroy_super_work() right before kfree(). Nothing is legal after rcu_sync_dtor(). > > > > static void rcu_sync_call(struct rcu_sync *rsp) > > > > { > > > > // TODO: THIS IS SUBOPTIMAL. We want to call it directly > > > > // if rcu_blocking_is_gp() == T, but it has might_sleep(). > > > > > > Careful! This optimization works only for RCU-sched and RCU-bh. > > > With normal RCU, you can be tripped up by tasks preempted within RCU > > > read-side critical sections should CONFIG_PREEMPT=y. > > > > Yes, thanks, I understand ;) another reason why I do not want to add > > this optimization into the initial version. > > So I should take this as a request to export rcu_blocking_is_gp()? Would be nice ;) Or we can do something else. Nevermind, this needs another discussion. > > > > void rcu_sync_dtor(struct rcu_sync *rsp) > > > > { > > > > int gp_state; > > > > > > > > BUG_ON(rsp->gp_count); > > > > BUG_ON(rsp->gp_state == GP_PASSED); > > > > > > > > spin_lock_irq(&rsp->rss_lock); > > > > if (rsp->gp_state == GP_REPLAY) > > > > rsp->gp_state = GP_EXIT; > > > > > > OK, this ensures that the .wait() below will wait for the callback, but > > > it might result in some RCU read-side critical sections still being in > > > flight after rcu_sync_dtor() completes. > > > > Hmm. Obviously, the caller should prevent this somehow or it is simply > > buggy. Or I misunderstood. > > Hard to say without knowing what the permitted use cases are... > > Me, I would make rcu_sync_dtor() wait the extra grace period in this case. > It should be a low-probability race, and it reduces the _dtor-time > state space. > > What it looks like you are saying is that the caller must not only ensure > that there will never again be a __rcu_sync_enter(), rcu_sync_enter(), > or rcu_sync_exit() (or, I suppose, rcu_sync_dtor()) for this rcu_sync > structure, Yes, and > but must also ensure that any relevant RCU read-side critical > sections have completed. Ah, now I understand your concerns. Yes, yes, sure. The caller must ensure that all RCU read-side critical sections which might look at this rcu_sync via rcu_sync_is_idle() have completed. Currently the only caller of dtor() is percpu_free_rwsem(). So if you do, say, struct percpu_rw_semaphore *sem = kmalloc(...); ... percpu_free_rwsem(sem); kfree(sem); you obviously need to ensure that percpu_free_rwsem() can't be called before all readers fully complete their percpu_down_read/percpu_up_read critical sections, this includes the RCU read-side critical sections. And this doesn't doesn't really differ from the plain rw_semaphore. And can't resist... let me add another "TODO" note ;) we actually want to improve it a bit (probably just a "bool wait" arg) and kill the ugly super_block->destroy_work which currently does percpu_free_rwsem(). This should be simple. Oleg.