Re: [PATCH 2/2] rcu/tree: Clarify comments about FQS loop reporting quiescent states
On Thu, Jul 30, 2020 at 09:21:52PM -0400, Joel Fernandes wrote:
> On Thu, Jul 30, 2020 at 09:35:20AM -0700, Paul E. McKenney wrote:
> > On Wed, Jul 29, 2020 at 11:25:19PM -0400, Joel Fernandes wrote:
> > > On Wed, Jul 29, 2020 at 11:02 PM Joel Fernandes (Google)
> > > wrote:
> > > >
> > > > At least since v4.19, the FQS loop no longer reports quiescent states
> > >
> > > I meant here, "FQS loop no longer reports quiescent states for offline
> > > CPUs."
> > >
> > > Sorry,
> >
> > You did have me going there for a bit. ;-)
> >
> > No period (".") at the end though, unless you fix up the following
> > to start a new sentence.
>
> Ok.
>
> > > > unless it is a dire situation where an offlined CPU failed to report
> > > > a quiescent state. Let us clarify the comment in rcu_gp_init() inorder
> > > > to keep the comment current.
> >
> > How about the following for this last sentence?
> >
> > "This commit therefore fixes the comment in rcu_gp_init() to match
> > the current code."
>
> As per:
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
>
> It says:
> Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead
> of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as
> if you are giving orders to the codebase to change its behaviour.
>
> May be I should make it "Fix the comment in rcu_gp_init() to match the
> current code"?
What submitting-patches.rst is objecting to is starting the commit
log with "This patch...". I am suggesting something quite different,
namely providing a clear indication of the transition from problem
statement to solution.
> > > > Signed-off-by: Joel Fernandes (Google)
> > > > ---
> > > > kernel/rcu/tree.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 1e51962b565b..929568ff5989 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -1701,8 +1701,8 @@ static bool rcu_gp_init(void)
> > > >
> > > > /*
> > > > * Apply per-leaf buffered online and offline operations to the
> > > > -* rcu_node tree. Note that this new grace period need not wait
> > > > -* for subsequent online CPUs, and that quiescent-state forcing
> > > > +* rcu_node tree. Note that this new grace period need not
> > > > wait for
> > > > +* subsequent online CPUs, and that RCU hooks in CPU offlining
> > > > path
> > > > * will handle subsequent offline CPUs.
> >
> > How about something like this?
> >
> > ... Note that this new grace period ned not wait for subsequent
> > online CPUs, and that RCU hooks in the CPU offlining path, when
> > combined with checks in this function, will handle CPUs that
> > are currently going offline and that go offline later.
>
> Sounds good to me. I think s/and that go/or that go/ though.
Good point! Another approach would be s/and that/and those that/
but yours works.
> I will make these changes and send v3, let me know though if you object.
Sounds good!
Thanx, Paul
> thanks,
>
> - Joel
>
>
> > Thanx, Paul
> >
> > > > */
> > > > rcu_state.gp_state = RCU_GP_ONOFF;
> > > > --
> > > > 2.28.0.rc0.142.g3c755180ce-goog
> > > >
Re: [PATCH 2/2] rcu/tree: Clarify comments about FQS loop reporting quiescent states
On Thu, Jul 30, 2020 at 09:35:20AM -0700, Paul E. McKenney wrote:
> On Wed, Jul 29, 2020 at 11:25:19PM -0400, Joel Fernandes wrote:
> > On Wed, Jul 29, 2020 at 11:02 PM Joel Fernandes (Google)
> > wrote:
> > >
> > > At least since v4.19, the FQS loop no longer reports quiescent states
> >
> > I meant here, "FQS loop no longer reports quiescent states for offline
> > CPUs."
> >
> > Sorry,
>
> You did have me going there for a bit. ;-)
>
> No period (".") at the end though, unless you fix up the following
> to start a new sentence.
Ok.
> > > unless it is a dire situation where an offlined CPU failed to report
> > > a quiescent state. Let us clarify the comment in rcu_gp_init() inorder
> > > to keep the comment current.
>
> How about the following for this last sentence?
>
> "This commit therefore fixes the comment in rcu_gp_init() to match
> the current code."
As per:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
It says:
Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead
of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as
if you are giving orders to the codebase to change its behaviour.
May be I should make it "Fix the comment in rcu_gp_init() to match the
current code"?
> > > Signed-off-by: Joel Fernandes (Google)
> > > ---
> > > kernel/rcu/tree.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 1e51962b565b..929568ff5989 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -1701,8 +1701,8 @@ static bool rcu_gp_init(void)
> > >
> > > /*
> > > * Apply per-leaf buffered online and offline operations to the
> > > -* rcu_node tree. Note that this new grace period need not wait
> > > -* for subsequent online CPUs, and that quiescent-state forcing
> > > +* rcu_node tree. Note that this new grace period need not wait
> > > for
> > > +* subsequent online CPUs, and that RCU hooks in CPU offlining
> > > path
> > > * will handle subsequent offline CPUs.
>
> How about something like this?
>
> ... Note that this new grace period ned not wait for subsequent
> online CPUs, and that RCU hooks in the CPU offlining path, when
> combined with checks in this function, will handle CPUs that
> are currently going offline and that go offline later.
Sounds good to me. I think s/and that go/or that go/ though.
I will make these changes and send v3, let me know though if you object.
thanks,
- Joel
> Thanx, Paul
>
> > > */
> > > rcu_state.gp_state = RCU_GP_ONOFF;
> > > --
> > > 2.28.0.rc0.142.g3c755180ce-goog
> > >
Re: [PATCH 2/2] rcu/tree: Clarify comments about FQS loop reporting quiescent states
On Wed, Jul 29, 2020 at 11:25:19PM -0400, Joel Fernandes wrote:
> On Wed, Jul 29, 2020 at 11:02 PM Joel Fernandes (Google)
> wrote:
> >
> > At least since v4.19, the FQS loop no longer reports quiescent states
>
> I meant here, "FQS loop no longer reports quiescent states for offline CPUs."
>
> Sorry,
You did have me going there for a bit. ;-)
No period (".") at the end though, unless you fix up the following
to start a new sentence.
> > unless it is a dire situation where an offlined CPU failed to report
> > a quiescent state. Let us clarify the comment in rcu_gp_init() inorder
> > to keep the comment current.
How about the following for this last sentence?
"This commit therefore fixes the comment in rcu_gp_init() to match
the current code."
> > Signed-off-by: Joel Fernandes (Google)
> > ---
> > kernel/rcu/tree.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 1e51962b565b..929568ff5989 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -1701,8 +1701,8 @@ static bool rcu_gp_init(void)
> >
> > /*
> > * Apply per-leaf buffered online and offline operations to the
> > -* rcu_node tree. Note that this new grace period need not wait
> > -* for subsequent online CPUs, and that quiescent-state forcing
> > +* rcu_node tree. Note that this new grace period need not wait for
> > +* subsequent online CPUs, and that RCU hooks in CPU offlining path
> > * will handle subsequent offline CPUs.
How about something like this?
... Note that this new grace period ned not wait for subsequent
online CPUs, and that RCU hooks in the CPU offlining path, when
combined with checks in this function, will handle CPUs that
are currently going offline and that go offline later.
Thanx, Paul
> > */
> > rcu_state.gp_state = RCU_GP_ONOFF;
> > --
> > 2.28.0.rc0.142.g3c755180ce-goog
> >
Re: [PATCH 2/2] rcu/tree: Clarify comments about FQS loop reporting quiescent states
On Wed, Jul 29, 2020 at 11:02 PM Joel Fernandes (Google) wrote: > > At least since v4.19, the FQS loop no longer reports quiescent states I meant here, "FQS loop no longer reports quiescent states for offline CPUs." Sorry, - Joel > unless it is a dire situation where an offlined CPU failed to report > a quiescent state. Let us clarify the comment in rcu_gp_init() inorder > to keep the comment current. > > Signed-off-by: Joel Fernandes (Google) > --- > kernel/rcu/tree.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 1e51962b565b..929568ff5989 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -1701,8 +1701,8 @@ static bool rcu_gp_init(void) > > /* > * Apply per-leaf buffered online and offline operations to the > -* rcu_node tree. Note that this new grace period need not wait > -* for subsequent online CPUs, and that quiescent-state forcing > +* rcu_node tree. Note that this new grace period need not wait for > +* subsequent online CPUs, and that RCU hooks in CPU offlining path > * will handle subsequent offline CPUs. > */ > rcu_state.gp_state = RCU_GP_ONOFF; > -- > 2.28.0.rc0.142.g3c755180ce-goog >
