Re: [PATCH] rcu: Report a quiescent state when it's exactly in the state

[Joel Fernandes]

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 <byungchul.park@xxxxxxx>
> > >---
> > >  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
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