Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp2650616imm; Sat, 12 May 2018 17:10:24 -0700 (PDT) X-Google-Smtp-Source: AB8JxZrTRQtDxzlkRetqNQWWi3Q3VGBZ14l4bqMFhZV0l7UndgoVck8XVET4Jhp0FB6Obra+ZjIH X-Received: by 2002:a65:4484:: with SMTP id l4-v6mr3101235pgq.103.1526170224928; Sat, 12 May 2018 17:10:24 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1526170224; cv=none; d=google.com; s=arc-20160816; b=if6486sdn7uTJSoCyBQZmUZ5sjllng7sb4SQA4lGs/e7hoUkSSXrOPHAmrHGkDpG5S lJ8vOHDedOP8KZHw0i30hzA87hYTIsHBlT+gVL2Wy0dy6W54fLUlN3ApccCsCyxqFen7 24HZFwappsojrtZ3WcLS7VuR1ZTykXgfvEqLI62SHXGBb4/FZeCLe9K3iADSRcA6a47j E22WjcP2JyWyZth7VUOjm/1GiI79Bs9IBbDF/obp2ScmnO82X5LAGwtYs3AyV2G8NaW+ 6F9CMh8wfKYfD8Q4RnWnVNmJgCRCC5wzf6jIJVRfmORNRs3GBibxWGCZVvMGcbnDyWtC Af/g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature :arc-authentication-results; bh=k98wBiT2MAFGcFeqMcYH6uhOUl0Kr4QlLKu6l0XzJag=; b=02Xs/vsvrVnj+jtZxRTEm1qkc6uAqJw1WaCR+RIFiQE/zs95zHHhF8OZNMdIt6Fven wDiL67wctapXDWwOFsVs+FZGgCKjtdfMHzSbEoFNjzjMzt4hF6l++HMhV/9Hg2zzHFGs yt5126LVGN8Mvz+DsHACn5KWtQeUtqDYQYRSh9KXKLqR8da2KSmGfbkZUcOSG5Q7JGWR l093YAR4OBXwZomN4zK/Su3wFN55PBup47eDCQwds8jWAS5NAulubrE5SJu4h2hFomYi hYujBQzXdV00LCDBZtjEuu0ftARzzNjtJAjtVj8a85+KDjHiVmABfIP3mQ4B4PXCVeBS sSew== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@joelfernandes-org.20150623.gappssmtp.com header.s=20150623 header.b=CrXD7oA+; 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 Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id e71-v6si6934815pfj.250.2018.05.12.17.09.57; Sat, 12 May 2018 17:10:24 -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; dkim=pass header.i=@joelfernandes-org.20150623.gappssmtp.com header.s=20150623 header.b=CrXD7oA+; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751654AbeEMAJv (ORCPT + 99 others); Sat, 12 May 2018 20:09:51 -0400 Received: from mail-pl0-f65.google.com ([209.85.160.65]:43854 "EHLO mail-pl0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751010AbeEMAJu (ORCPT ); Sat, 12 May 2018 20:09:50 -0400 Received: by mail-pl0-f65.google.com with SMTP id c41-v6so74961plj.10 for ; Sat, 12 May 2018 17:09:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=k98wBiT2MAFGcFeqMcYH6uhOUl0Kr4QlLKu6l0XzJag=; b=CrXD7oA+6ucMXFq3Sn7kzHwJdN82KFpZQUAHY33j+kYiTt8nJtnun0SIuI54hIfDpq XbplAs+GY0rCC0u1hHbNpLlFjxcGJE1hz1I5vyD/BlRZrmVJDLukpv90DtYG0uQj7VJY AoM/5K3y8l65jNc/Y+bGF/+CTq2A8307JRLgjmCHGckOuzA4M2gJL7A4DczabIw3Fa8E t/k/cKS1cKig2vpz9LyicHWebKWAMHCYVmaESDpoKl1a/l5dtPvIm1qb4gGhx8dk45r+ ZjIQ1mmPI4LpGFzoxW8f2G+ieZr9SpGa9KHFw8UqOMT6tIpnbqDBapNxbM+BoDDH6ba7 AgPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=k98wBiT2MAFGcFeqMcYH6uhOUl0Kr4QlLKu6l0XzJag=; b=GTv6rgRNEe4VYa5A0ZnbEXiBoG4kRDfFCjKAveRmp/GTjpaRGMBcjmIXLE580E9kf5 tAAQ5GKohbnh4hi//9+CqGkMFLi4wbZxEEAM8HhVYNhdCEUtyTEGcv+dTR222TJDXbtA nb3+aYk/Oyh+JcTF8mWoCXK3+xGULyVpurCTZGJFxs9XohFQPrqtfZxcFu0/EOR7SSrc De5p8hY8boc3Q9EQNabJYnuUTlWTXTXksFRxC9OlIJE8mLKwKJMsoloV5e9B/rYlRc3J JrCw72rqjI095i1pMx+S6Ded7XgbUZ3wsZzEkw/zEihvrUaYRedfcON2Vo8Kte7FiIPi Du8Q== X-Gm-Message-State: ALKqPwcCd1l05awvCKbAiKxfqvkXvhqQGqFrMW+hB5hlAh570VIWMyU8 6SnPmPF7fLhtA+wpbtKNLa16NaY/B0U= X-Received: by 2002:a17:902:b582:: with SMTP id a2-v6mr4033968pls.371.1526170189687; Sat, 12 May 2018 17:09:49 -0700 (PDT) Received: from localhost ([2620:0:1000:1600:3122:ea9c:d178:eb]) by smtp.gmail.com with ESMTPSA id w8-v6sm14328903pfk.112.2018.05.12.17.09.48 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 12 May 2018 17:09:49 -0700 (PDT) Date: Sat, 12 May 2018 17:09:48 -0700 From: Joel Fernandes To: "Paul E. McKenney" 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 Message-ID: <20180513000948.GE192642@joelaf.mtv.corp.google.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> <20180512144119.GJ26088@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180512144119.GJ26088@linux.vnet.ibm.com> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, May 12, 2018 at 07:41:19AM -0700, Paul E. McKenney wrote: > 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 Cool, I'm glad, thanks. > need to at least change the comments, and perhaps the code as well. But Ok. I can make this kind of a comment in my clean up patch series just to clarify how the macro is used for. > I agree that we need input from Peter and Steven to make much more forward > progress. I saw Steven's email just now about cond_resched and stuff and it makes sense. thanks, - Joel