Re: [PATCH] stop_machine: pass curstate to ack_state()

[Rong Tao]

On 10/25/23 8:50 AM, Rong Tao wrote:
> On 10/25/23 8:43 AM, Rong Tao wrote:
> > The multi_cpu_stop() state machine uses multi_stop_data::state to hold
> > the current state, and this is read and written atomically except in
> > ack_state(), which performs a non-atomic read.
> >
> > As ack_state() only performs this non-atomic read when there is a single
> > writer, this is benign, but it makes reasoning about the state machine a
> > little harder.
> >
> > Remove the non-atomic read and pass the (atomically read) curstate in
> > instead. This makes it clear that we do not expect any racy writes, and
> > avoids a redundant load.
>
> I'd prefer if we make this comment:
>
> stop_machine: pass curstate to ack_state()
OK, thanks a lot, i'll try that.
> > Acked-by: Mark Rutland <mark.rutland@xxxxxxx>
> > Signed-off-by: Rong Tao <rongtao@xxxxxxxx>
> > ---
> >  kernel/stop_machine.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> > index cedb17ba158a..268c2e581698 100644
> > --- a/kernel/stop_machine.c
> > +++ b/kernel/stop_machine.c
> > @@ -188,10 +188,11 @@ static void set_state(struct multi_stop_data 
> > *msdata,
> >  }
> >
> >  /* Last one to ack a state moves to the next state. */
> > -static void ack_state(struct multi_stop_data *msdata)
> > +static void ack_state(struct multi_stop_data *msdata,
> > +              enum multi_stop_state curstate)
> >  {
> >      if (atomic_dec_and_test(&msdata->thread_ack))
> > -        set_state(msdata, msdata->state + 1);
> > +        set_state(msdata, curstate + 1);
> >  }
> >
> >  notrace void __weak stop_machine_yield(const struct cpumask *cpumask)
> > @@ -242,7 +243,7 @@ static int multi_cpu_stop(void *data)
> >              default:
> >                  break;
> >              }
> > -            ack_state(msdata);
> > +            ack_state(msdata, curstate);
> >          } else if (curstate > MULTI_STOP_PREPARE) {
> >              /*
> >               * At this stage all other CPUs we depend on must spin