Messages in this thread | | | Date | Wed, 25 Oct 2023 08:59:33 +0800 | Subject | Re: [PATCH 2/2] stop_machine: Apply smp_store_release() to multi_stop_data::state | From | Rong Tao <> |
| |
On 10/24/23 7:01 PM, Mark Rutland wrote: > On Fri, Oct 20, 2023 at 10:43:34PM +0800, Rong Tao wrote: >> From: Rong Tao <rongtao@cestc.cn> >> >> Replace smp_wmb()+WRITE_ONCE() with smp_store_release() and add comment. >> >> Signed-off-by: Rong Tao <rongtao@cestc.cn> >> --- >> kernel/stop_machine.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c >> index 268c2e581698..cdf4a3fe0348 100644 >> --- a/kernel/stop_machine.c >> +++ b/kernel/stop_machine.c >> @@ -183,8 +183,10 @@ static void set_state(struct multi_stop_data *msdata, >> { >> /* Reset ack counter. */ >> atomic_set(&msdata->thread_ack, msdata->num_threads); >> - smp_wmb(); >> - WRITE_ONCE(msdata->state, newstate); >> + /* This smp_store_release() pair with READ_ONCE() in multi_cpu_stop(). >> + * Avoid potential access multi_stop_data::state race behaviour. >> + */ >> + smp_store_release(&msdata->state, newstate); > This doesn't match coding style: > > /* > * Block comments should look like this, with a leading '/*' line > * before the text and a traling '*/' line afterwards. > */ > > See https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting Thanks, Mark, I'll fix the comment in next patch version. > > I don't think the "Avoid potential access multi_stop_data::state race > behaviour." text is all that helpful, and I think we can drop that. > > In general, it's unusual to pair a smp_store_release() with READ_ONCE(), and > for that to work it relies on dependency ordering and/or hazarding on the > reader side (e.g. the atomic_dec_and_test() is ordered after the READ_ONCE() > since it's an RMW and there's a control dependency, but a plain read could be > reordered w.r.t. the READ_ONCE()). So we probably need to explain that if we're > going to comment on that smp_store_release(). > > Peter, might it be worth replacing the READ_ONCE() with smp_load_acquire() at > the same time? I know it's not strictly necessary given the ordering we have > today, but it would at least be obvious.
After I wait for Peter to reply to this message, I will write a patch based on Peter's suggestion.
Rong Tao.
> > Mark. > >> } >> >> /* Last one to ack a state moves to the next state. */ >> -- >> 2.41.0 >>
| |