From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751486AbdJ1DuA (ORCPT ); Fri, 27 Oct 2017 23:50:00 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:49794 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750851AbdJ1Dt5 (ORCPT ); Fri, 27 Oct 2017 23:49:57 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 6C0CB60116 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=neeraju@codeaurora.org Subject: Re: Query regarding srcu_funnel_exp_start() To: paulmck@linux.vnet.ibm.com Cc: josh@joshtriplett.org, rostedt@goodmis.org, mathieu.desnoyers@efficios.com, jiangshanlai@gmail.com, LKML References: <481c3c8d-2385-2e38-dfe6-0cb9b6855b8a@codeaurora.org> <20171027122601.GJ3659@linux.vnet.ibm.com> <946852c9-88cb-7363-e74d-82a1efbd3be1@codeaurora.org> <20171027222039.GP3659@linux.vnet.ibm.com> From: Neeraj Upadhyay Message-ID: Date: Sat, 28 Oct 2017 09:19:52 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20171027222039.GP3659@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/28/2017 03:50 AM, Paul E. McKenney wrote: > On Fri, Oct 27, 2017 at 10:15:04PM +0530, Neeraj Upadhyay wrote: >> On 10/27/2017 05:56 PM, Paul E. McKenney wrote: >>> On Fri, Oct 27, 2017 at 02:23:07PM +0530, Neeraj Upadhyay wrote: >>>> Hi, >>>> >>>> One query regarding srcu_funnel_exp_start() function in >>>> kernel/rcu/srcutree.c. >>>> >>>> static void srcu_funnel_exp_start(struct srcu_struct *sp, struct >>>> srcu_node *snp, >>>> unsigned long s) >>>> { >>>> >>>> if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s)) >>>> sp->srcu_gp_seq_needed_exp = s; >>>> >>>> } >>>> >>>> Why is sp->srcu_gp_seq_needed_exp set to 's' if srcu_gp_seq_needed_exp is >= >>>> 's'. Shouldn't srcu_gp_seq_needed_exp be equal to the greater of both? >>> >>> Let's suppose that it is incorrect as currently written. Can you >>> construct a test case demonstrating a failure of some sort, then provide >>> a fix? >> >> Will check this. Might take some time to build a test case. > > Fair enough! > > I suggest checking to see if kernel/rcu/rcuperf.c can do what you need for > this test. (Might not with a single test, but perhaps a before-and-after > comparison. Or maybe you really do need to add some test code somewhere.) > Thanks for the suggestion, will try that out. >>> To start with, if it is currently incorrect, what would be the nature >>> of the failure? >>> >>> Thanx, Paul >>> >> >> Hi Paul, >> >> I see below scenario, where new gp won't be expedited. Please correct >> me if I am missing something here. >> >> 1. CPU0 calls synchronize_srcu_expedited() >> >> synchronize_srcu_expedited() >> __synchronize_srcu() >> __call_srcu() >> s = rcu_seq_snap(&sp->srcu_gp_seq); // lets say >> srcu_gp_seq = 0; >> // s = 0x100 > > Looks like you have one hex digit and then two binary digits, but why not? > (RCU_SEQ_STATE_MASK is 3 rather than 0xff > Yeah, sorry I confused myself while representing the values. 0x100 need to be replaced with b'100' and 0x200 with b'1000'. >> sdp->srcu_gp_seq_needed = s // 0x100 >> needgp = true >> sdp->srcu_gp_seq_needed_exp = s // 0x100 >> srcu_funnel_gp_start() >> sp->srcu_gp_seq_needed_exp = s; >> srcu_gp_start(sp); >> rcu_seq_start(&sp->srcu_gp_seq); >> >> 2. CPU1 calls normal synchronize_srcu() >> >> synchronize_srcu() >> __synchronize_srcu(sp, true) >> __call_srcu() >> s = rcu_seq_snap(&sp->srcu_gp_seq); // srcu_gp_seq = 1 >> // s= 0x200 >> sdp->srcu_gp_seq_needed = s; // 0x200 >> srcu_funnel_gp_start() >> smp_store_release(&sp->srcu_gp_seq_needed, s); // 0x200 >> >> 3. CPU3 calls synchronize_srcu_expedited() >> >> synchronize_srcu_expedited() >> __synchronize_srcu() >> __call_srcu() >> s = rcu_seq_snap(&sp->srcu_gp_seq); // srcu_gp_seq = 1 >> // s = 0x200 >> sdp->srcu_gp_seq_needed_exp = s // 0x200 >> srcu_funnel_exp_start(sp, sdp->mynode, s); >> // sp->srcu_gp_seq_needed_exp = 0x100 >> // s = 0x200 ; sp->srcu_gp_seq_needed_exp is not updated >> if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s)) >> sp->srcu_gp_seq_needed_exp = s; > > Seems plausible, but you should be able to show the difference in > grace-period duration with a test. > Ok sure, will attempt that. > While you are in srcu_funnel_exp_start(), should it be rechecking > rcu_seq_done(&sp->srcu_gp_seq, s) as well as the current > ULONG_CMP_GE(snp->srcu_gp_seq_needed_exp, s) under the lock? > Why or why not? > > Thanx, Paul > Hi Paul, I don't see how it will impact. I have put markers in code snippet below to explain my points. My understanding is * rcu_seq_done check @a is a fastpath return, and avoid contention for snp lock, if the gp has already elapsed. * Checking it @b, inside srcu_node lock might not make any difference, as sp->srcu_gp_seq counter portion is updated under srcu_struct lock. Also, we cannot lock srcu_struct at this point, as it will cause lock contention among multiple CPUs. * Checking rcu_seq_done @c also does not impact, as we have already done all the work of traversing the entire parent chain and if rcu_seq_done() is true srcu_gp_seq_needed_exp will be greater than or equal to 's'. srcu_gp_end() raw_spin_lock_irq_rcu_node(sp); rcu_seq_end(&sp->srcu_gp_seq); gpseq = rcu_seq_current(&sp->srcu_gp_seq); if (ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, gpseq)) sp->srcu_gp_seq_needed_exp = gpseq; raw_spin_unlock_irq_rcu_node(sp); static void srcu_funnel_exp_start(...) { for (; snp != NULL; snp = snp->srcu_parent) { if (rcu_seq_done(&sp->srcu_gp_seq, s) || /* a */ ULONG_CMP_GE(READ_ONCE(snp->srcu_gp_seq_needed_exp), s)) return; raw_spin_lock_irqsave_rcu_node(snp, flags); /* b */ if (ULONG_CMP_GE(snp->srcu_gp_seq_needed_exp, s)) { raw_spin_unlock_irqrestore_rcu_node(snp, flags); return; } raw_spin_unlock_irqrestore_rcu_node(snp, flags); } raw_spin_lock_irqsave_rcu_node(sp, flags); /* c */ if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s)) sp->srcu_gp_seq_needed_exp = s; raw_spin_unlock_irqrestore_rcu_node(sp, flags); } Thanks Neeraj -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation