| Message ID | 20220406141649.728971-1-guoren@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3] riscv: patch_text: Fixup last cpu should be master | expand |
On Wed, Apr 06, 2022 at 10:16:49PM +0800, guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > These patch_text implementations are using stop_machine_cpuslocked > infrastructure with atomic cpu_count. The original idea: When the > master CPU patch_text, the others should wait for it. But current > implementation is using the first CPU as master, which couldn't > guarantee the remaining CPUs are waiting. This patch changes the > last CPU as the master to solve the potential risk. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > Cc: <stable@vger.kernel.org> > --- > arch/riscv/kernel/patch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) What commit id does this change fix?
On Wed, 06 Apr 2022 11:13:36 PDT (-0700), Greg KH wrote: > On Wed, Apr 06, 2022 at 10:16:49PM +0800, guoren@kernel.org wrote: >> From: Guo Ren <guoren@linux.alibaba.com> >> >> These patch_text implementations are using stop_machine_cpuslocked >> infrastructure with atomic cpu_count. The original idea: When the >> master CPU patch_text, the others should wait for it. But current >> implementation is using the first CPU as master, which couldn't >> guarantee the remaining CPUs are waiting. This patch changes the >> last CPU as the master to solve the potential risk. >> >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> >> Signed-off-by: Guo Ren <guoren@kernel.org> >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> >> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> >> Cc: <stable@vger.kernel.org> >> --- >> arch/riscv/kernel/patch.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > What commit id does this change fix? I think it's been there since the beginning of our text patching, so Fixes: 043cb41a85de ("riscv: introduce interfaces to patch kernel code") seems like the best bet, but I'll go take another look before merging it. That's confusing here, as I acked it, but that was for an earlier version that touched more than one arch so it was more ambiguous as to which tree it was going through (IIRC I said one of those "LMK if you want it through my tree, but here's an Ack in case someone else wants to take it" sort of things, as I usually do when it's ambiguous). Without a changelog, cover letter, or the other patches in the set it's kind of hard to tell, though ;)
On Thu, Apr 7, 2022 at 2:13 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Apr 06, 2022 at 10:16:49PM +0800, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > These patch_text implementations are using stop_machine_cpuslocked > > infrastructure with atomic cpu_count. The original idea: When the > > master CPU patch_text, the others should wait for it. But current > > implementation is using the first CPU as master, which couldn't > > guarantee the remaining CPUs are waiting. This patch changes the > > last CPU as the master to solve the potential risk. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: <stable@vger.kernel.org> > > --- > > arch/riscv/kernel/patch.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > What commit id does this change fix? Thx for pointing this out, I would follow the rule to add Cc: <stable@vger.kernel.org>. >
On Thu, Apr 7, 2022 at 3:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote: > > On Wed, 06 Apr 2022 11:13:36 PDT (-0700), Greg KH wrote: > > On Wed, Apr 06, 2022 at 10:16:49PM +0800, guoren@kernel.org wrote: > >> From: Guo Ren <guoren@linux.alibaba.com> > >> > >> These patch_text implementations are using stop_machine_cpuslocked > >> infrastructure with atomic cpu_count. The original idea: When the > >> master CPU patch_text, the others should wait for it. But current > >> implementation is using the first CPU as master, which couldn't > >> guarantee the remaining CPUs are waiting. This patch changes the > >> last CPU as the master to solve the potential risk. > >> > >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > >> Signed-off-by: Guo Ren <guoren@kernel.org> > >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > >> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > >> Cc: <stable@vger.kernel.org> > >> --- > >> arch/riscv/kernel/patch.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > What commit id does this change fix? > > I think it's been there since the beginning of our text patching, so > > Fixes: 043cb41a85de ("riscv: introduce interfaces to patch kernel code") Yes, it the riscv origin. > > seems like the best bet, but I'll go take another look before merging > it. That's confusing here, as I acked it, but that was for an earlier > version that touched more than one arch so it was more ambiguous as to > which tree it was going through (IIRC I said one of those "LMK if you > want it through my tree, but here's an Ack in case someone else wants to > take it" sort of things, as I usually do when it's ambiguous). Thx for the clarification, I would remove the acked in the next version. > > Without a changelog, cover letter, or the other patches in the set it's > kind of hard to tell, though ;) Okay, I should add a changelog for the patch with cover letter.
On Wed, 06 Apr 2022 07:16:49 PDT (-0700), guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > These patch_text implementations are using stop_machine_cpuslocked > infrastructure with atomic cpu_count. The original idea: When the > master CPU patch_text, the others should wait for it. But current > implementation is using the first CPU as master, which couldn't > guarantee the remaining CPUs are waiting. This patch changes the > last CPU as the master to solve the potential risk. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > Cc: <stable@vger.kernel.org> > --- > arch/riscv/kernel/patch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > index 0b552873a577..765004b60513 100644 > --- a/arch/riscv/kernel/patch.c > +++ b/arch/riscv/kernel/patch.c > @@ -104,7 +104,7 @@ static int patch_text_cb(void *data) > struct patch_insn *patch = data; > int ret = 0; > > - if (atomic_inc_return(&patch->cpu_count) == 1) { > + if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) { > ret = > patch_text_nosync(patch->addr, &patch->insn, > GET_INSN_LENGTH(patch->insn)); Thanks, this is on fixes.
On Thu, 21 Apr 2022 15:57:32 PDT (-0700), Palmer Dabbelt wrote: > On Wed, 06 Apr 2022 07:16:49 PDT (-0700), guoren@kernel.org wrote: >> From: Guo Ren <guoren@linux.alibaba.com> >> >> These patch_text implementations are using stop_machine_cpuslocked >> infrastructure with atomic cpu_count. The original idea: When the >> master CPU patch_text, the others should wait for it. But current >> implementation is using the first CPU as master, which couldn't >> guarantee the remaining CPUs are waiting. This patch changes the >> last CPU as the master to solve the potential risk. >> >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> >> Signed-off-by: Guo Ren <guoren@kernel.org> >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> >> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> >> Cc: <stable@vger.kernel.org> >> --- >> arch/riscv/kernel/patch.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c >> index 0b552873a577..765004b60513 100644 >> --- a/arch/riscv/kernel/patch.c >> +++ b/arch/riscv/kernel/patch.c >> @@ -104,7 +104,7 @@ static int patch_text_cb(void *data) >> struct patch_insn *patch = data; >> int ret = 0; >> >> - if (atomic_inc_return(&patch->cpu_count) == 1) { >> + if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) { >> ret = >> patch_text_nosync(patch->addr, &patch->insn, >> GET_INSN_LENGTH(patch->insn)); > > Thanks, this is on fixes. Sorry, I forgot to add the Fixes and stable tags. I just fixed that up, but I'm going to hold off on this one until next week's PR to make sure it has time to go through linux-next.
On Sat, Apr 23, 2022 at 12:02 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 21 Apr 2022 15:57:32 PDT (-0700), Palmer Dabbelt wrote: > > On Wed, 06 Apr 2022 07:16:49 PDT (-0700), guoren@kernel.org wrote: > >> From: Guo Ren <guoren@linux.alibaba.com> > >> > >> These patch_text implementations are using stop_machine_cpuslocked > >> infrastructure with atomic cpu_count. The original idea: When the > >> master CPU patch_text, the others should wait for it. But current > >> implementation is using the first CPU as master, which couldn't > >> guarantee the remaining CPUs are waiting. This patch changes the > >> last CPU as the master to solve the potential risk. > >> > >> Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > >> Signed-off-by: Guo Ren <guoren@kernel.org> > >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > >> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org> > >> Cc: <stable@vger.kernel.org> > >> --- > >> arch/riscv/kernel/patch.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c > >> index 0b552873a577..765004b60513 100644 > >> --- a/arch/riscv/kernel/patch.c > >> +++ b/arch/riscv/kernel/patch.c > >> @@ -104,7 +104,7 @@ static int patch_text_cb(void *data) > >> struct patch_insn *patch = data; > >> int ret = 0; > >> > >> - if (atomic_inc_return(&patch->cpu_count) == 1) { > >> + if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) { > >> ret = > >> patch_text_nosync(patch->addr, &patch->insn, > >> GET_INSN_LENGTH(patch->insn)); > > > > Thanks, this is on fixes. > > Sorry, I forgot to add the Fixes and stable tags. I just fixed that up, > but I'm going to hold off on this one until next week's PR to make sure > it has time to go through linux-next. https://lore.kernel.org/linux-riscv/20220407073323.743224-3-guoren@kernel.org/
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c index 0b552873a577..765004b60513 100644 --- a/arch/riscv/kernel/patch.c +++ b/arch/riscv/kernel/patch.c @@ -104,7 +104,7 @@ static int patch_text_cb(void *data) struct patch_insn *patch = data; int ret = 0; - if (atomic_inc_return(&patch->cpu_count) == 1) { + if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) { ret = patch_text_nosync(patch->addr, &patch->insn, GET_INSN_LENGTH(patch->insn));