Re: [PATCH 1/2] rcu: Show the real fqs_state

[Petr Mladek]

On Tue 2015-09-08 12:59:15, Paul E. McKenney wrote:
> On Mon, Sep 07, 2015 at 04:58:27PM +0200, Petr Mladek wrote:
> > On Fri 2015-09-04 16:24:22, Paul E. McKenney wrote:
> > > On Fri, Sep 04, 2015 at 02:11:29PM +0200, Petr Mladek wrote:
[...]

> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 69ab7ce2cf7b..04234936d897 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -1949,16 +1949,15 @@ static bool rcu_gp_fqs_check_wake(struct rcu_state *rsp, int *gfp)
> > >  /*
> > >   * Do one round of quiescent-state forcing.
> > >   */
> > > -static int rcu_gp_fqs(struct rcu_state *rsp, int fqs_state_in)
> > > +static void rcu_gp_fqs(struct rcu_state *rsp)
> > >  {
> > > -	int fqs_state = fqs_state_in;
> > >  	bool isidle = false;
> > >  	unsigned long maxj;
> > >  	struct rcu_node *rnp = rcu_get_root(rsp);
> > >  
> > >  	WRITE_ONCE(rsp->gp_activity, jiffies);
> > >  	rsp->n_force_qs++;
> > > -	if (fqs_state == RCU_SAVE_DYNTICK) {
> > > +	if (rsp->gp_state == RCU_SAVE_DYNTICK) {
> > 
> > This will never happen because rcu_gp_kthread() modifies rsp->gp_state
> > many times. The last value before calling rcu_gp_fqs() is
> > RCU_GP_DOING_FQS.
> > 
> > I think about passing this information via a separate bool.
> > 
> > [...]
> > 
> > > diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> > > index d5f58e717c8b..9faad70a8246 100644
> > > --- a/kernel/rcu/tree.h
> > > +++ b/kernel/rcu/tree.h
> > > @@ -417,12 +417,11 @@ struct rcu_data {
> > >  	struct rcu_state *rsp;
> > >  };
> > >  
> > > -/* Values for fqs_state field in struct rcu_state. */
> > > +/* Values for gp_state field in struct rcu_state. */
> > >  #define RCU_GP_IDLE		0	/* No grace period in progress. */
> > 
> > This value seems to be used instead of the new RCU_GP_WAIT_INIT.
> > 
> > >  #define RCU_GP_INIT		1	/* Grace period being
> > >  #initialized. */
> > 
> > This value is unused.
> > 
> > >  #define RCU_SAVE_DYNTICK	2	/* Need to scan dyntick
> > >  #state. */
> > 
> > This one is not longer preserved when merged with the other state.
> > 
> > >  #define RCU_FORCE_QS		3	/* Need to force quiescent
> > >  #state. */
> > 
> > The meaning of this one is strange. If I get it correctly,
> > it is set after the state was forced. But the comment suggests
> > that it is before.
> > 
> > By other words, these states seems to get obsoleted by
> > 
> > /* Values for rcu_state structure's gp_flags field. */
> > #define RCU_GP_WAIT_INIT 0	/* Initial state. */
> > #define RCU_GP_WAIT_GPS  1	/* Wait for grace-period start. */
> > #define RCU_GP_DONE_GPS  2	/* Wait done for grace-period start. */
> > #define RCU_GP_WAIT_FQS  3	/* Wait for force-quiescent-state time. */
> > #define RCU_GP_DOING_FQS 4	/* Wait done for force-quiescent-state time. */
> > #define RCU_GP_CLEANUP   5	/* Grace-period cleanup started. */
> > #define RCU_GP_CLEANED   6	/* Grace-period cleanup complete. */
> > 
> > 
> > Please, find below your commit updated with my ideas:
> > 
> > 	+ used bool save_dyntick instead of RCU_SAVE_DYNTICK
> > 	  and RCU_FORCE_QS states
> > 	+ rename RCU_GP_WAIT_INIT -> RCU_GP_IDLE
> > 	+ remove all the obsolete states
> > 
> > I am sorry if I handled "Signed-off-by" flags a wrong way. It is
> > basically your patch with few small updates from me. I am not sure
> > what is the right process in this case. Feel free to use Reviewed-by
> > instead of Signed-off-by with my name.
> > 
> > Well, I guess that this is not the final state ;-)
> 
> Good points, but perhaps an easier solution would be to have a
> "firsttime" argument to rcu_gp_fqs() that said whether or not this
> was the first call to rcu_gp_fqs() during the current grace period.
> If this is the first call, then take the "if" branch that passes
> dyntick_save_progress_counter() to force_qs_rnp(), otherwise take the
> other branch.

This seems to be the most elegant solution at the moment.

> But I am not generating the patch today, just flew across the Pacific
> yesterday.  ;-)

Please, find below the updated patch where I used the first_time
parameter.

Again, I am not sure about the commit person and Signed-off-by
tags. Many parts of the patch are yours. Feel free to update
them.