Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp2205501imm; Sat, 12 May 2018 07:41:18 -0700 (PDT) X-Google-Smtp-Source: AB8JxZrHBvXbrB6sdTqQ1tSohgrFYmIH3wcJbtD2fM92de8LoCBLjCFAgDls1YT5EJe9yqAjKVA6 X-Received: by 2002:a62:6a0a:: with SMTP id f10-v6mr3301009pfc.99.1526136077929; Sat, 12 May 2018 07:41:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1526136077; cv=none; d=google.com; s=arc-20160816; b=MdBv+PqLeJ6lPQF3DZToR64xy2d/gOkVdsux/sWJe0mvpz1dw/0mPm26VTWydbSfvr XjLFfM6pRyrtKHFFzBMknUFDcuHuc72uxcRW8f1XwSbtyKNzkjOlRDIU/8yZ2cPNv2zN r9azViHTX8tlaocuZ/RqZhFF3y7NrGDhwMzDBl2JL9XHEtEqJtnjb9bkXSfM4fHPctSY VFWO7D1FzZleNdiA1N7V31AE58c1wLk3a90xdv74VnuQk649mjsQw/zez92Z56AGmQcZ BfFe3asUS2Ys9/wlO6ZetKXIVzEQHVuqBq/DkJDBl9xhWm+44LZUmJmv1uZonBll27UF qX5g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:message-id:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:reply-to:subject:cc:to:from:date :arc-authentication-results; bh=MQ1b4GCSU9pL4QhpNHVXDt4qGwUVxMIeAZzR8o95J+Y=; b=CpUavLn6xk1A/K33HToBLKOE8so3QgoREx9aij94HJJPI1lvC/VC/7oeLZZMd2/Xu3 qJIkBd/4G/olPLYyfYSLXGIxyA6g1wPstahjVsg5YV9XUhmztqE9skL2Kg/In3q80Nl/ RzvYt7ebF5QAnqvS3mfD7hl8jvb/4HHvkXeFpYEDYjrQvFodxL8yIG49XjUQr0+Dfcdu Ad85KDCdBeMVIbDdu3zY/0oRowQPct9cxK1ZFAapLtKsZRW9wa4HcX7DpZn3TFSzfAGX BdBdUPwwB0cpqICGYJdvS5fTW540XJDozupNnVyypDfSGIJVRbvdexLfD1QbCLGV/gnB EAtA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=ibm.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w25-v6si4352071pgv.526.2018.05.12.07.41.03; Sat, 12 May 2018 07:41:17 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=ibm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751654AbeELOj5 (ORCPT + 99 others); Sat, 12 May 2018 10:39:57 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:34720 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751533AbeELOj4 (ORCPT ); Sat, 12 May 2018 10:39:56 -0400 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4CEdAKs025481 for ; Sat, 12 May 2018 10:39:55 -0400 Received: from e16.ny.us.ibm.com (e16.ny.us.ibm.com [129.33.205.206]) by mx0a-001b2d01.pphosted.com with ESMTP id 2hwvtj0xgd-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sat, 12 May 2018 10:39:55 -0400 Received: from localhost by e16.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 12 May 2018 10:39:54 -0400 Received: from b01cxnp22035.gho.pok.ibm.com (9.57.198.25) by e16.ny.us.ibm.com (146.89.104.203) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Sat, 12 May 2018 10:39:50 -0400 Received: from b01ledav003.gho.pok.ibm.com (b01ledav003.gho.pok.ibm.com [9.57.199.108]) by b01cxnp22035.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id w4CEdnYI56623302; Sat, 12 May 2018 14:39:49 GMT Received: from b01ledav003.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id F1817B2046; Sat, 12 May 2018 11:41:47 -0400 (EDT) Received: from paulmck-ThinkPad-W541 (unknown [9.85.188.179]) by b01ledav003.gho.pok.ibm.com (Postfix) with ESMTP id 9FAA9B204E; Sat, 12 May 2018 11:41:47 -0400 (EDT) Received: by paulmck-ThinkPad-W541 (Postfix, from userid 1000) id 1752916C125B; Sat, 12 May 2018 07:41:19 -0700 (PDT) Date: Sat, 12 May 2018 07:41:19 -0700 From: "Paul E. McKenney" To: Joel Fernandes Cc: Byungchul Park , jiangshanlai@gmail.com, josh@joshtriplett.org, rostedt@goodmis.org, mathieu.desnoyers@efficios.com, linux-kernel@vger.kernel.org, kernel-team@lge.com, peterz@infradead.org Subject: Re: [PATCH] rcu: Report a quiescent state when it's exactly in the state Reply-To: paulmck@linux.vnet.ibm.com References: <1526027434-21237-1-git-send-email-byungchul.park@lge.com> <3af4cec0-4019-e3ac-77f9-8631252fb6da@lge.com> <20180511161746.GX26088@linux.vnet.ibm.com> <20180511224138.GA89902@joelaf.mtv.corp.google.com> <20180512050824.GF26088@linux.vnet.ibm.com> <20180512063037.GC192642@joelaf.mtv.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180512063037.GC192642@joelaf.mtv.corp.google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 x-cbid: 18051214-0024-0000-0000-00000357E834 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00009012; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000260; SDB=6.01031245; UDB=6.00527102; IPR=6.00810377; MB=3.00021071; MTD=3.00000008; XFM=3.00000015; UTC=2018-05-12 14:39:53 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18051214-0025-0000-0000-000047FCA884 Message-Id: <20180512144119.GJ26088@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-12_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1805120147 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 11, 2018 at 11:30:37PM -0700, Joel Fernandes wrote: > On Fri, May 11, 2018 at 10:08:24PM -0700, Paul E. McKenney wrote: > > On Fri, May 11, 2018 at 03:41:38PM -0700, Joel Fernandes wrote: > > > On Fri, May 11, 2018 at 09:17:46AM -0700, Paul E. McKenney wrote: > > > > On Fri, May 11, 2018 at 09:57:54PM +0900, Byungchul Park wrote: > > > > > Hello folks, > > > > > > > > > > I think I wrote the title in a misleading way. > > > > > > > > > > Please change the title to something else such as, > > > > > "rcu: Report a quiescent state when it's in the state" or, > > > > > "rcu: Add points reporting quiescent states where proper" or so on. > > > > > > > > > > On 2018-05-11 오후 5:30, Byungchul Park wrote: > > > > > >We expect a quiescent state of TASKS_RCU when cond_resched_tasks_rcu_qs() > > > > > >is called, no matter whether it actually be scheduled or not. However, > > > > > >it currently doesn't report the quiescent state when the task enters > > > > > >into __schedule() as it's called with preempt = true. So make it report > > > > > >the quiescent state unconditionally when cond_resched_tasks_rcu_qs() is > > > > > >called. > > > > > > > > > > > >And in TINY_RCU, even though the quiescent state of rcu_bh also should > > > > > >be reported when the tick interrupt comes from user, it doesn't. So make > > > > > >it reported. > > > > > > > > > > > >Lastly in TREE_RCU, rcu_note_voluntary_context_switch() should be > > > > > >reported when the tick interrupt comes from not only user but also idle, > > > > > >as an extended quiescent state. > > > > > > > > > > > >Signed-off-by: Byungchul Park > > > > > >--- > > > > > > include/linux/rcupdate.h | 4 ++-- > > > > > > kernel/rcu/tiny.c | 6 +++--- > > > > > > kernel/rcu/tree.c | 4 ++-- > > > > > > 3 files changed, 7 insertions(+), 7 deletions(-) > > > > > > > > > > > >diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > > > > >index ee8cf5fc..7432261 100644 > > > > > >--- a/include/linux/rcupdate.h > > > > > >+++ b/include/linux/rcupdate.h > > > > > >@@ -195,8 +195,8 @@ static inline void exit_tasks_rcu_finish(void) { } > > > > > > */ > > > > > > #define cond_resched_tasks_rcu_qs() \ > > > > > > do { \ > > > > > >- if (!cond_resched()) \ > > > > > >- rcu_note_voluntary_context_switch_lite(current); \ > > > > > >+ rcu_note_voluntary_context_switch_lite(current); \ > > > > > >+ cond_resched(); \ > > > > > > > > Ah, good point. > > > > > > > > Peter, I have to ask... Why is "cond_resched()" considered a preemption > > > > while "schedule()" is not? > > > > > > Infact something interesting I inferred from the __schedule loop related to > > > your question: > > > > > > switch_count can either be set to prev->invcsw or prev->nvcsw. If we can > > > assume that switch_count reflects whether the context switch is involuntary > > > or voluntary, > > > > > > task-running-state preempt switch_count > > > 0 (running) 1 involuntary > > > 0 0 involuntary > > > 1 0 voluntary > > > 1 1 involuntary > > > > > > According to the above table, both the task's running state and the preempt > > > parameter to __schedule should be used together to determine if the switch is > > > a voluntary one or not. > > > > > > So this code in rcu_note_context_switch should really be: > > > if (!preempt && !(current->state & TASK_RUNNING)) > > I should have writte here- !preempt && current->state > > > > rcu_note_voluntary_context_switch_lite(current); > > > > > > According to the above table, cond_resched always classifies as an > > > involuntary switch which makes sense to me. Even though cond_resched is > > > explicitly called, its still sort of involuntary in the sense its not called > > > into the scheduler for sleeping, but rather for seeing if something else can > > > run instead (a preemption point). Infact none of the task deactivation in the > > > __schedule loop will run if cond_resched is used. > > > > > > I agree that if schedule was called directly but with TASK_RUNNING=1, then > > > that could probably be classified an involuntary switch too... > > > > > > Also since we're deciding to call rcu_note_voluntary_context_switch_lite > > > unconditionally, then IMO this comment on that macro: > > > > > > /* > > > * Note a voluntary context switch for RCU-tasks benefit. This is a > > > * macro rather than an inline function to avoid #include hell. > > > */ > > > #ifdef CONFIG_TASKS_RCU > > > #define rcu_note_voluntary_context_switch_lite(t) > > > > > > Should be changed to: > > > > > > /* > > > * Note a attempt to perform a voluntary context switch for RCU-tasks > > > * benefit. This is called even in situations where a context switch > > > * didn't really happen even though it was requested. This is a > > > * macro rather than an inline function to avoid #include hell. > > > */ > > > #ifdef CONFIG_TASKS_RCU > > > #define rcu_note_voluntary_context_switch_lite(t) > > > > > > Right? > > > > > > Correct me if I'm wrong about anything, thanks, > > > > The starting point for me is that Tasks RCU is a special-purpose mechanism > > for freeing trampolines in PREEMPT=y kernels. The approach is to arrange > > for the trampoline to be inaccessible to future execution, wait for a > > tasks-RCU grace period, then free the trampoline. So a tasks-RCU grace > > period must wait until all tasks have spent at least some time outside > > of a trampoline. My understanding is that trampolines cannot contain > > preemption points, such as cond_resched() and cond_resched_tasks_rcu_qs(), > > so we want to count them as quiescent states regardless of whether or > > not any associated context switch is counted as involuntary. > > > > What situations lead to the second line of your table above? > > The sched_yield() system call, but trampolines don't do system calls, > > either, as far as I know. > > > > So it looks to me like that test can leave out the TASK_RUNNING check. > > I don't know much about tasks-RCU to comment more, sorry. Probably a few more > reading nights for me to catch up with that. Its possible the check is not > needed and tasks-RCU can survive without it, but I was thinking from a > correctness and future-proofing stand point... I generally don't like > inconsistencies. The check in the __schedule loop is as: > > if (!preempt && prev->state) { > .... > // switch_count = voluntary context switch counter pointer > .... > } else { > .... > > // switch_count = involuntary context switch counter pointer > .... > } > > // context switch really happening > if (prev != next) { > .... > ++switch_count; > } > > The first conditional if (!preempt...) above is what I was referring to which > also checks the state. > > Also this issue aside, I was more trying to answer your question about why > schedule() is or isn't a preemption point, by sharing the table but I > possibly caused more confusion, sorry :-(. I'll let Peter and Steven chime in > since they know more than me about that and will just shutup and listen > instead of being more noisy.. :-D Don't get me wrong, this discussion was quite useful to me. We probably need to at least change the comments, and perhaps the code as well. But I agree that we need input from Peter and Steven to make much more forward progress. Thanx, Paul