Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp3773936imm; Sun, 13 May 2018 20:00:12 -0700 (PDT) X-Google-Smtp-Source: AB8JxZo0MN8+L3WTgEgtogPk9AkpfEm+dNfTq33OEsHjjWGRf0HdVqD+cMjTT1BJXjwIRcnSgJcx X-Received: by 2002:a17:902:bc48:: with SMTP id t8-v6mr7864714plz.133.1526266812400; Sun, 13 May 2018 20:00:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1526266812; cv=none; d=google.com; s=arc-20160816; b=uFMbZVDX/4gnKhF7e8KjKdl4qKTuld0zqqEgCoERttaWDa7DZREczQ4RtfUQVmY2AS WgLIp7KSKZoLh96OXfE/OJyuWjoIz0vf2qGXY54GHvbMwYaaczDfHPg8yIV4tuQ+6Hoa r+qh+vRnd0DcY6aHytNtIvlDDs/VNilE9js+JCAzGfyp1qDjPXbvSOYrE5GyGww0cNAd EXGN7qFZN2PmGZtzRJZ/bTNOVso1CVfsq3tG8O7HjziQyMZp14vFJ0GyzN2H6bLc37xx UJF2R4/IO3UjK4/b+4LPFh8h1lfMr+OZZB1rJeBF3FOSJVRHzSw4G1G7ZfauUeW1t9q9 T/PQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=Ygy/sd7T/IMEaaocERi6Uy4tmULTJPuXx/e5h1wrs6c=; b=kW/ESN1BOt2cY9EZ2mzvSeVXSe3vFkrS2tAVff7S7lER1dZ22r5RisCZ64HcIEB6hm VyPYAc1agTrN206ljB7/lel45PQNM/n1ouy4FlfN/wIPmJbQGYHZFlet6f4Omk9vMk8O 5c8MBeYDW2CpIvQ4WuU6JKr6rwH5h8MjhDnI24xOR/u2Y1xTB1b7iooFe9F3UcSNiAtZ wjosXbZkwtzSgzqx2s6bEuO+eSv1ajnPEIplFJnprn2NI4uePnK0FikHK64SxAvq9i/P dyzs+QFdCIyrxleQZCMeogcRAmMlANWBz1ZFXD6h4iiQwOgWVuV/A5jmjoMov3056fFf WTpg== 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 Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id h128-v6si6864043pgc.545.2018.05.13.19.59.57; Sun, 13 May 2018 20:00:12 -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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752043AbeENC7p (ORCPT + 99 others); Sun, 13 May 2018 22:59:45 -0400 Received: from lgeamrelo12.lge.com ([156.147.23.52]:41527 "EHLO lgeamrelo11.lge.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751953AbeENC7o (ORCPT ); Sun, 13 May 2018 22:59:44 -0400 Received: from unknown (HELO lgeamrelo01.lge.com) (156.147.1.125) by 156.147.23.52 with ESMTP; 14 May 2018 11:59:42 +0900 X-Original-SENDERIP: 156.147.1.125 X-Original-MAILFROM: byungchul.park@lge.com Received: from unknown (HELO ?10.177.220.135?) (10.177.220.135) by 156.147.1.125 with ESMTP; 14 May 2018 11:59:41 +0900 X-Original-SENDERIP: 10.177.220.135 X-Original-MAILFROM: byungchul.park@lge.com Subject: Re: [PATCH] rcu: Report a quiescent state when it's exactly in the state To: Joel Fernandes , "Paul E. McKenney" Cc: 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 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> From: Byungchul Park Message-ID: <9f2e445b-15b0-d1fa-832c-f801efc34d03@lge.com> Date: Mon, 14 May 2018 11:59:41 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180511224138.GA89902@joelaf.mtv.corp.google.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2018-05-12 오전 7:41, 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)) > 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 Hello guys, The classification for nivcsw/nvcsw used in scheduler core, Joel, you showed us is different from that used in when we distinguish between non preemption/voluntary preemption/preemption/full and so on, even they use the same word, "voluntary" though. The name, rcu_note_voluntary_context_switch_lite() used in RCU has a lot to do with the latter, the term of preemption. Furthermore, I think the function should be called even when calling schedule() for sleep as well. I think it would be better to change the function name to something else to prevent confusing, it's up to Paul tho. :) > 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, > > - Joel > > -- Thanks, Byungchul