Re: [PATCH 1/2] rcu: Show the real fqs_state
On Wed, Sep 09, 2015 at 02:39:50PM +0200, Petr Mladek wrote:
> 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_IDLE0 /* No grace period in progress.
> > > > */
> > >
> > > This value seems to be used instead of the new RCU_GP_WAIT_INIT.
> > >
> > > > #define RCU_GP_INIT1 /* 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.
Thank you and here you go! Starting testing, and will let you
know how it goes.
Thanx, Paul
commit bbe84e224959475fd5be9e9c18aede3a6abe4ab9
Author: Petr Mladek
Date: Wed Sep 9
Re: [PATCH 1/2] rcu: Show the real fqs_state
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.
>From 7d7f2ee97a451f5cb055901a3bf22fec23a53bff Mon Sep 17 00:00:00 2001
From: "Paul E. McKenney"
Date: Fri, 4 Sep 2015 16:24:22 -0700
Subject: [PATCH] rcu: Finish folding ->fqs_state into ->gp_state
Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
into kthread") started the process of folding the old ->fqs_state
into ->gp_state, but did not complete it. This situation does not
cause any malfunction, but can result in extremely confusing trace
output. This commit completes this task of eliminating ->fqs_state
in favor of ->gp_state.
The old fqs_state had one side effect. It was used to decide whether
to collect dyntick-idle snapshots. For this purpose, we add a boolean
Re: [PATCH 1/2] rcu: Show the real fqs_state
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.
>From 7d7f2ee97a451f5cb055901a3bf22fec23a53bff Mon Sep 17 00:00:00 2001
From: "Paul E. McKenney"
Date: Fri, 4 Sep 2015 16:24:22 -0700
Subject: [PATCH] rcu: Finish folding ->fqs_state into ->gp_state
Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
into kthread") started the process of folding the old ->fqs_state
into ->gp_state, but did not complete it. This situation does not
cause any malfunction, but can result in extremely confusing trace
output. This commit completes this task of eliminating ->fqs_state
in favor of ->gp_state.
The old fqs_state had one side effect. It was used to decide whether
to collect dyntick-idle snapshots. For this
Re: [PATCH 1/2] rcu: Show the real fqs_state
On Wed, Sep 09, 2015 at 02:39:50PM +0200, Petr Mladek wrote:
> 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_IDLE0 /* No grace period in progress.
> > > > */
> > >
> > > This value seems to be used instead of the new RCU_GP_WAIT_INIT.
> > >
> > > > #define RCU_GP_INIT1 /* 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.
Thank you and here you go! Starting testing, and will let you
know how it goes.
Thanx, Paul
commit bbe84e224959475fd5be9e9c18aede3a6abe4ab9
Author: Petr Mladek
Date:
Re: [PATCH 1/2] rcu: Show the real fqs_state
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:
> > > The value of "fqs_state" in struct rcu_state is always RCU_GP_IDLE.
> > >
> > > The real state is stored in a local variable in rcu_gp_kthread().
> > > It is modified by rcu_gp_fqs() via parameter and return value.
> > > But the actual value is never stored to rsp->fqs_state.
> > >
> > > The result is that print_one_rcu_state() does not show the real
> > > state.
> > >
> > > This code has been added 3 years ago by the commit 4cdfc175c25c89ee
> > > ("rcu: Move quiescent-state forcing into kthread"). I guess that it
> > > was an overlook or optimization.
> > >
> > > Anyway, the value seems to be manipulated only by the thread, except
> > > for shoving the status. I do not see any risk in updating it directly
> > > in the struct.
> > >
> > > Signed-off-by: Petr Mladek
> >
> > Good catch, but how about the following fix instead?
> >
> > Thanx, Paul
> >
> >
> >
> > rcu: Finish folding ->fqs_state into ->gp_state
> >
> > Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
> > into kthread") started the process of folding the old ->fqs_state
> > into ->gp_state, but did not complete it. This situation does not
> > cause any malfunction, but can result in extremely confusing trace
> > output. This commit completes this task of eliminating ->fqs_state
> > in favor of ->gp_state.
>
> It makes sense but it breaks dynticks handling in rcu_gp_fqs(), see
> below.
Indeed, more confusion on my part!
> > Reported-by: Petr Mladek
> > Signed-off-by: Paul E. McKenney
> >
> > 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_IDLE0 /* No grace period in progress.
> > */
>
> This value seems to be used instead of the new RCU_GP_WAIT_INIT.
>
> > #define RCU_GP_INIT1 /* 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
Re: [PATCH 1/2] rcu: Show the real fqs_state
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:
> > > The value of "fqs_state" in struct rcu_state is always RCU_GP_IDLE.
> > >
> > > The real state is stored in a local variable in rcu_gp_kthread().
> > > It is modified by rcu_gp_fqs() via parameter and return value.
> > > But the actual value is never stored to rsp->fqs_state.
> > >
> > > The result is that print_one_rcu_state() does not show the real
> > > state.
> > >
> > > This code has been added 3 years ago by the commit 4cdfc175c25c89ee
> > > ("rcu: Move quiescent-state forcing into kthread"). I guess that it
> > > was an overlook or optimization.
> > >
> > > Anyway, the value seems to be manipulated only by the thread, except
> > > for shoving the status. I do not see any risk in updating it directly
> > > in the struct.
> > >
> > > Signed-off-by: Petr Mladek
> >
> > Good catch, but how about the following fix instead?
> >
> > Thanx, Paul
> >
> >
> >
> > rcu: Finish folding ->fqs_state into ->gp_state
> >
> > Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
> > into kthread") started the process of folding the old ->fqs_state
> > into ->gp_state, but did not complete it. This situation does not
> > cause any malfunction, but can result in extremely confusing trace
> > output. This commit completes this task of eliminating ->fqs_state
> > in favor of ->gp_state.
>
> It makes sense but it breaks dynticks handling in rcu_gp_fqs(), see
> below.
Indeed, more confusion on my part!
> > Reported-by: Petr Mladek
> > Signed-off-by: Paul E. McKenney
> >
> > 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_IDLE0 /* No grace period in progress.
> > */
>
> This value seems to be used instead of the new RCU_GP_WAIT_INIT.
>
> > #define RCU_GP_INIT1 /* 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
Re: [PATCH 1/2] rcu: Show the real fqs_state
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:
> > The value of "fqs_state" in struct rcu_state is always RCU_GP_IDLE.
> >
> > The real state is stored in a local variable in rcu_gp_kthread().
> > It is modified by rcu_gp_fqs() via parameter and return value.
> > But the actual value is never stored to rsp->fqs_state.
> >
> > The result is that print_one_rcu_state() does not show the real
> > state.
> >
> > This code has been added 3 years ago by the commit 4cdfc175c25c89ee
> > ("rcu: Move quiescent-state forcing into kthread"). I guess that it
> > was an overlook or optimization.
> >
> > Anyway, the value seems to be manipulated only by the thread, except
> > for shoving the status. I do not see any risk in updating it directly
> > in the struct.
> >
> > Signed-off-by: Petr Mladek
>
> Good catch, but how about the following fix instead?
>
> Thanx, Paul
>
>
>
> rcu: Finish folding ->fqs_state into ->gp_state
>
> Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
> into kthread") started the process of folding the old ->fqs_state
> into ->gp_state, but did not complete it. This situation does not
> cause any malfunction, but can result in extremely confusing trace
> output. This commit completes this task of eliminating ->fqs_state
> in favor of ->gp_state.
It makes sense but it breaks dynticks handling in rcu_gp_fqs(), see
below.
>
> Reported-by: Petr Mladek
> Signed-off-by: Paul E. McKenney
>
> 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 ;-)
>From 61a1bf6659f4f4c0c4021f185bc156f8c83f9ea5 Mon Sep 17 00:00:00 2001
From: "Paul E. McKenney"
Date: Fri, 4 Sep 2015 16:24:22 -0700
Subject: [PATCH] rcu: Finish folding ->fqs_state into ->gp_state
Commit commit 4cdfc175c25c89ee ("rcu: Move
Re: [PATCH 1/2] rcu: Show the real fqs_state
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:
> > The value of "fqs_state" in struct rcu_state is always RCU_GP_IDLE.
> >
> > The real state is stored in a local variable in rcu_gp_kthread().
> > It is modified by rcu_gp_fqs() via parameter and return value.
> > But the actual value is never stored to rsp->fqs_state.
> >
> > The result is that print_one_rcu_state() does not show the real
> > state.
> >
> > This code has been added 3 years ago by the commit 4cdfc175c25c89ee
> > ("rcu: Move quiescent-state forcing into kthread"). I guess that it
> > was an overlook or optimization.
> >
> > Anyway, the value seems to be manipulated only by the thread, except
> > for shoving the status. I do not see any risk in updating it directly
> > in the struct.
> >
> > Signed-off-by: Petr Mladek
>
> Good catch, but how about the following fix instead?
>
> Thanx, Paul
>
>
>
> rcu: Finish folding ->fqs_state into ->gp_state
>
> Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
> into kthread") started the process of folding the old ->fqs_state
> into ->gp_state, but did not complete it. This situation does not
> cause any malfunction, but can result in extremely confusing trace
> output. This commit completes this task of eliminating ->fqs_state
> in favor of ->gp_state.
It makes sense but it breaks dynticks handling in rcu_gp_fqs(), see
below.
>
> Reported-by: Petr Mladek
> Signed-off-by: Paul E. McKenney
>
> 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 ;-)
>From 61a1bf6659f4f4c0c4021f185bc156f8c83f9ea5 Mon Sep 17 00:00:00 2001
From: "Paul E. McKenney"
Date: Fri, 4 Sep 2015 16:24:22 -0700
Subject: [PATCH] rcu: Finish
Re: [PATCH 1/2] rcu: Show the real fqs_state
On Fri, Sep 04, 2015 at 02:11:29PM +0200, Petr Mladek wrote:
> The value of "fqs_state" in struct rcu_state is always RCU_GP_IDLE.
>
> The real state is stored in a local variable in rcu_gp_kthread().
> It is modified by rcu_gp_fqs() via parameter and return value.
> But the actual value is never stored to rsp->fqs_state.
>
> The result is that print_one_rcu_state() does not show the real
> state.
>
> This code has been added 3 years ago by the commit 4cdfc175c25c89ee
> ("rcu: Move quiescent-state forcing into kthread"). I guess that it
> was an overlook or optimization.
>
> Anyway, the value seems to be manipulated only by the thread, except
> for shoving the status. I do not see any risk in updating it directly
> in the struct.
>
> Signed-off-by: Petr Mladek
Good catch, but how about the following fix instead?
Thanx, Paul
rcu: Finish folding ->fqs_state into ->gp_state
Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
into kthread") started the process of folding the old ->fqs_state
into ->gp_state, but did not complete it. This situation does not
cause any malfunction, but can result in extremely confusing trace
output. This commit completes this task of eliminating ->fqs_state
in favor of ->gp_state.
Reported-by: Petr Mladek
Signed-off-by: Paul E. McKenney
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
@@ -97,7 +97,7 @@ struct rcu_state sname##_state = { \
.level = { ##_state.node[0] }, \
.rda = ##_data, \
.call = cr, \
- .fqs_state = RCU_GP_IDLE, \
+ .gp_state = RCU_GP_IDLE, \
.gpnum = 0UL - 300UL, \
.completed = 0UL - 300UL, \
.orphan_lock = __RAW_SPIN_LOCK_UNLOCKED(##_state.orphan_lock), \
@@ -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) {
/* Collect dyntick-idle snapshots. */
if (is_sysidle_rcu_state(rsp)) {
isidle = true;
@@ -1967,7 +1966,7 @@ static int rcu_gp_fqs(struct rcu_state *rsp, int
fqs_state_in)
force_qs_rnp(rsp, dyntick_save_progress_counter,
, );
rcu_sysidle_report_gp(rsp, isidle, maxj);
- fqs_state = RCU_FORCE_QS;
+ rsp->gp_state = RCU_FORCE_QS;
} else {
/* Handle dyntick-idle and offline CPUs. */
isidle = true;
@@ -1981,7 +1980,6 @@ static int rcu_gp_fqs(struct rcu_state *rsp, int
fqs_state_in)
READ_ONCE(rsp->gp_flags) & ~RCU_GP_FLAG_FQS);
raw_spin_unlock_irq(>lock);
}
- return fqs_state;
}
/*
@@ -2045,7 +2043,7 @@ static void rcu_gp_cleanup(struct rcu_state *rsp)
/* Declare grace period done. */
WRITE_ONCE(rsp->completed, rsp->gpnum);
trace_rcu_grace_period(rsp->name, rsp->completed, TPS("end"));
- rsp->fqs_state = RCU_GP_IDLE;
+ rsp->gp_state = RCU_GP_IDLE;
rdp = this_cpu_ptr(rsp->rda);
/* Advance CBs to reduce false positives below. */
needgp = rcu_advance_cbs(rsp, rnp, rdp) || needgp;
@@ -2063,7 +2061,6 @@ static void rcu_gp_cleanup(struct rcu_state *rsp)
*/
static int __noreturn rcu_gp_kthread(void *arg)
{
- int fqs_state;
int gf;
unsigned long j;
int ret;
@@ -2095,7 +2092,7 @@ static int __noreturn rcu_gp_kthread(void *arg)
}
/* Handle quiescent-state forcing. */
- fqs_state = RCU_SAVE_DYNTICK;
+ rsp->gp_state = RCU_SAVE_DYNTICK;
j = jiffies_till_first_fqs;
if (j > HZ) {
j = HZ;
@@ -2123,7 +2120,7 @@ static int __noreturn rcu_gp_kthread(void *arg)
trace_rcu_grace_period(rsp->name,
READ_ONCE(rsp->gpnum),
TPS("fqsstart"));
- fqs_state = rcu_gp_fqs(rsp, fqs_state);
+ rcu_gp_fqs(rsp);
trace_rcu_grace_period(rsp->name,
READ_ONCE(rsp->gpnum),
Re: [PATCH 1/2] rcu: Show the real fqs_state
On Fri, Sep 04, 2015 at 02:11:29PM +0200, Petr Mladek wrote:
> The value of "fqs_state" in struct rcu_state is always RCU_GP_IDLE.
>
> The real state is stored in a local variable in rcu_gp_kthread().
> It is modified by rcu_gp_fqs() via parameter and return value.
> But the actual value is never stored to rsp->fqs_state.
>
> The result is that print_one_rcu_state() does not show the real
> state.
>
> This code has been added 3 years ago by the commit 4cdfc175c25c89ee
> ("rcu: Move quiescent-state forcing into kthread"). I guess that it
> was an overlook or optimization.
>
> Anyway, the value seems to be manipulated only by the thread, except
> for shoving the status. I do not see any risk in updating it directly
> in the struct.
>
> Signed-off-by: Petr Mladek
Good catch, but how about the following fix instead?
Thanx, Paul
rcu: Finish folding ->fqs_state into ->gp_state
Commit commit 4cdfc175c25c89ee ("rcu: Move quiescent-state forcing
into kthread") started the process of folding the old ->fqs_state
into ->gp_state, but did not complete it. This situation does not
cause any malfunction, but can result in extremely confusing trace
output. This commit completes this task of eliminating ->fqs_state
in favor of ->gp_state.
Reported-by: Petr Mladek
Signed-off-by: Paul E. McKenney
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
@@ -97,7 +97,7 @@ struct rcu_state sname##_state = { \
.level = { ##_state.node[0] }, \
.rda = ##_data, \
.call = cr, \
- .fqs_state = RCU_GP_IDLE, \
+ .gp_state = RCU_GP_IDLE, \
.gpnum = 0UL - 300UL, \
.completed = 0UL - 300UL, \
.orphan_lock = __RAW_SPIN_LOCK_UNLOCKED(##_state.orphan_lock), \
@@ -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) {
/* Collect dyntick-idle snapshots. */
if (is_sysidle_rcu_state(rsp)) {
isidle = true;
@@ -1967,7 +1966,7 @@ static int rcu_gp_fqs(struct rcu_state *rsp, int
fqs_state_in)
force_qs_rnp(rsp, dyntick_save_progress_counter,
, );
rcu_sysidle_report_gp(rsp, isidle, maxj);
- fqs_state = RCU_FORCE_QS;
+ rsp->gp_state = RCU_FORCE_QS;
} else {
/* Handle dyntick-idle and offline CPUs. */
isidle = true;
@@ -1981,7 +1980,6 @@ static int rcu_gp_fqs(struct rcu_state *rsp, int
fqs_state_in)
READ_ONCE(rsp->gp_flags) & ~RCU_GP_FLAG_FQS);
raw_spin_unlock_irq(>lock);
}
- return fqs_state;
}
/*
@@ -2045,7 +2043,7 @@ static void rcu_gp_cleanup(struct rcu_state *rsp)
/* Declare grace period done. */
WRITE_ONCE(rsp->completed, rsp->gpnum);
trace_rcu_grace_period(rsp->name, rsp->completed, TPS("end"));
- rsp->fqs_state = RCU_GP_IDLE;
+ rsp->gp_state = RCU_GP_IDLE;
rdp = this_cpu_ptr(rsp->rda);
/* Advance CBs to reduce false positives below. */
needgp = rcu_advance_cbs(rsp, rnp, rdp) || needgp;
@@ -2063,7 +2061,6 @@ static void rcu_gp_cleanup(struct rcu_state *rsp)
*/
static int __noreturn rcu_gp_kthread(void *arg)
{
- int fqs_state;
int gf;
unsigned long j;
int ret;
@@ -2095,7 +2092,7 @@ static int __noreturn rcu_gp_kthread(void *arg)
}
/* Handle quiescent-state forcing. */
- fqs_state = RCU_SAVE_DYNTICK;
+ rsp->gp_state = RCU_SAVE_DYNTICK;
j = jiffies_till_first_fqs;
if (j > HZ) {
j = HZ;
@@ -2123,7 +2120,7 @@ static int __noreturn rcu_gp_kthread(void *arg)
trace_rcu_grace_period(rsp->name,
READ_ONCE(rsp->gpnum),
TPS("fqsstart"));
- fqs_state = rcu_gp_fqs(rsp, fqs_state);
+ rcu_gp_fqs(rsp);
trace_rcu_grace_period(rsp->name,
