We would like to use text_poke_bp in ftrace. It might be called also during
boot when the interupts are disabled. We need to enable them for syncing
the cores on each CPU. Otherwise, there might be a deadlock, see the
warning in "smp_call_function_many", kernel/smp.c:371.
This change is taken from the current code in arch/x86/kernel/ftrace.c.
Signed-off-by: Petr Mladek <[email protected]>
---
arch/x86/kernel/alternative.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index f714316..13cae15 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -629,6 +629,20 @@ static void do_sync_core(void *info)
sync_core();
}
+static void run_sync(void)
+{
+ int enable_irqs = irqs_disabled();
+
+ /* We may be called with interrupts disbled (on bootup). */
+ if (enable_irqs)
+ local_irq_enable();
+
+ on_each_cpu(do_sync_core, NULL, 1);
+
+ if (enable_irqs)
+ local_irq_disable();
+}
+
static bool bp_patching_in_progress;
static void *bp_int3_handler, *bp_int3_addr;
@@ -688,7 +702,7 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
text_poke_part(addr, &int3, sizeof(int3));
- on_each_cpu(do_sync_core, NULL, 1);
+ run_sync();
if (len - sizeof(int3) > 0) {
/* patch all but the first byte */
@@ -700,13 +714,13 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
* not necessary and we'd be safe even without it. But
* better safe than sorry (plus there's not only Intel).
*/
- on_each_cpu(do_sync_core, NULL, 1);
+ run_sync();
}
/* patch the first byte */
text_poke_part(addr, opcode, sizeof(int3));
- on_each_cpu(do_sync_core, NULL, 1);
+ run_sync();
bp_patching_in_progress = false;
smp_wmb();
--
1.8.4
(2013/10/18 23:27), Petr Mladek wrote:
> We would like to use text_poke_bp in ftrace. It might be called also during
> boot when the interupts are disabled. We need to enable them for syncing
> the cores on each CPU. Otherwise, there might be a deadlock, see the
> warning in "smp_call_function_many", kernel/smp.c:371.
Steven, is this really needed?
I think if this is the special use(e.g. boottime test),
we'd better to run it after boot...
Thank you,
>
> This change is taken from the current code in arch/x86/kernel/ftrace.c.
>
> Signed-off-by: Petr Mladek <[email protected]>
> ---
> arch/x86/kernel/alternative.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index f714316..13cae15 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -629,6 +629,20 @@ static void do_sync_core(void *info)
> sync_core();
> }
>
> +static void run_sync(void)
> +{
> + int enable_irqs = irqs_disabled();
> +
> + /* We may be called with interrupts disbled (on bootup). */
> + if (enable_irqs)
> + local_irq_enable();
> +
> + on_each_cpu(do_sync_core, NULL, 1);
> +
> + if (enable_irqs)
> + local_irq_disable();
> +}
> +
> static bool bp_patching_in_progress;
> static void *bp_int3_handler, *bp_int3_addr;
>
> @@ -688,7 +702,7 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
>
> text_poke_part(addr, &int3, sizeof(int3));
>
> - on_each_cpu(do_sync_core, NULL, 1);
> + run_sync();
>
> if (len - sizeof(int3) > 0) {
> /* patch all but the first byte */
> @@ -700,13 +714,13 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
> * not necessary and we'd be safe even without it. But
> * better safe than sorry (plus there's not only Intel).
> */
> - on_each_cpu(do_sync_core, NULL, 1);
> + run_sync();
> }
>
> /* patch the first byte */
> text_poke_part(addr, opcode, sizeof(int3));
>
> - on_each_cpu(do_sync_core, NULL, 1);
> + run_sync();
>
> bp_patching_in_progress = false;
> smp_wmb();
>
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
On Sun, 20 Oct 2013 00:02:32 +0900
Masami Hiramatsu <[email protected]> wrote:
> (2013/10/18 23:27), Petr Mladek wrote:
> > We would like to use text_poke_bp in ftrace. It might be called also during
> > boot when the interupts are disabled. We need to enable them for syncing
> > the cores on each CPU. Otherwise, there might be a deadlock, see the
> > warning in "smp_call_function_many", kernel/smp.c:371.
>
> Steven, is this really needed?
> I think if this is the special use(e.g. boottime test),
> we'd better to run it after boot...
>
It's used to convert the calls to mcount to nops. But maybe a better
thing to do is to check if we only have a single CPU:
static void run_sync(void)
{
if (num_online_cpus() != 1)
on_each_cpu(do_sync_core, NULL, 1);
}
I believe that the only time we call this function with interrupts
disabled is before SMP is set up. Thus, the above change would handle
that case.
-- Steve
[ Added Paul because he'll understand this ]
On Sat, 19 Oct 2013 15:16:58 -0400
Steven Rostedt <[email protected]> wrote:
> On Sun, 20 Oct 2013 00:02:32 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
> > (2013/10/18 23:27), Petr Mladek wrote:
> > > We would like to use text_poke_bp in ftrace. It might be called also during
> > > boot when the interupts are disabled. We need to enable them for syncing
> > > the cores on each CPU. Otherwise, there might be a deadlock, see the
> > > warning in "smp_call_function_many", kernel/smp.c:371.
> >
> > Steven, is this really needed?
> > I think if this is the special use(e.g. boottime test),
> > we'd better to run it after boot...
> >
>
> It's used to convert the calls to mcount to nops. But maybe a better
> thing to do is to check if we only have a single CPU:
>
> static void run_sync(void)
> {
> if (num_online_cpus() != 1)
Hmm, to be more robust to handle our future "ideal" machines, perhaps
this should be:
/* Ideally we would like to run on zero CPUS! */
if (num_online_cpus() < 2)
-- Steve
> on_each_cpu(do_sync_core, NULL, 1);
> }
>
>
> I believe that the only time we call this function with interrupts
> disabled is before SMP is set up. Thus, the above change would handle
> that case.
>
> -- Steve
On Sat, Oct 19, 2013 at 03:19:19PM -0400, Steven Rostedt wrote:
>
> [ Added Paul because he'll understand this ]
>
> On Sat, 19 Oct 2013 15:16:58 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Sun, 20 Oct 2013 00:02:32 +0900
> > Masami Hiramatsu <[email protected]> wrote:
> >
> > > (2013/10/18 23:27), Petr Mladek wrote:
> > > > We would like to use text_poke_bp in ftrace. It might be called also during
> > > > boot when the interupts are disabled. We need to enable them for syncing
> > > > the cores on each CPU. Otherwise, there might be a deadlock, see the
> > > > warning in "smp_call_function_many", kernel/smp.c:371.
> > >
> > > Steven, is this really needed?
> > > I think if this is the special use(e.g. boottime test),
> > > we'd better to run it after boot...
> > >
> >
> > It's used to convert the calls to mcount to nops. But maybe a better
> > thing to do is to check if we only have a single CPU:
> >
> > static void run_sync(void)
> > {
> > if (num_online_cpus() != 1)
>
> Hmm, to be more robust to handle our future "ideal" machines, perhaps
> this should be:
>
> /* Ideally we would like to run on zero CPUS! */
> if (num_online_cpus() < 2)
To be really safe, shouldn't you use complex numbers? Just in case
you end up running on a system with 5i-3 CPUs or something. ;-)
Thanx, Paul
> -- Steve
>
>
> > on_each_cpu(do_sync_core, NULL, 1);
> > }
> >
> >
> > I believe that the only time we call this function with interrupts
> > disabled is before SMP is set up. Thus, the above change would handle
> > that case.
> >
> > -- Steve
>
On Sat, 19 Oct 2013 14:33:50 -0700
"Paul E. McKenney" <[email protected]> wrote:
> > /* Ideally we would like to run on zero CPUS! */
> > if (num_online_cpus() < 2)
>
> To be really safe, shouldn't you use complex numbers? Just in case
> you end up running on a system with 5i-3 CPUs or something. ;-)
>
> Thanx, Paul
I did think about that, but I didn't know the algorithm to test such a
case, which is why I added you to the Cc. Hoping you would come up with
a better conditional.
-- Steve
On Sat, 19 Oct 2013 14:33:50 -0700
"Paul E. McKenney" <[email protected]> wrote:
> > > It's used to convert the calls to mcount to nops. But maybe a better
> > > thing to do is to check if we only have a single CPU:
> > >
> > > static void run_sync(void)
> > > {
> > > if (num_online_cpus() != 1)
> >
> > Hmm, to be more robust to handle our future "ideal" machines, perhaps
> > this should be:
> >
> > /* Ideally we would like to run on zero CPUS! */
> > if (num_online_cpus() < 2)
>
Bah! And for such a simple computation, I got it wrong.
/* Ideally we would like to run on zero CPUS! */
if (num_online_cpus > 1)
But I guess the question comes. If we are running on zero CPUS, should
we perform the "on_each_cpu(do_sync_core, NULL, 1);" or not? Same goes
with 5i-3 CPUS, or negative number CPUs. If we need to do on_each_cpu(),
then I guess the != 1 will suffice.
-- Steve
On Sat, Oct 19, 2013 at 06:02:39PM -0400, Steven Rostedt wrote:
> On Sat, 19 Oct 2013 14:33:50 -0700
> "Paul E. McKenney" <[email protected]> wrote:
>
>
> > > > It's used to convert the calls to mcount to nops. But maybe a better
> > > > thing to do is to check if we only have a single CPU:
> > > >
> > > > static void run_sync(void)
> > > > {
> > > > if (num_online_cpus() != 1)
> > >
> > > Hmm, to be more robust to handle our future "ideal" machines, perhaps
> > > this should be:
> > >
> > > /* Ideally we would like to run on zero CPUS! */
> > > if (num_online_cpus() < 2)
> >
>
> Bah! And for such a simple computation, I got it wrong.
>
>
> /* Ideally we would like to run on zero CPUS! */
> if (num_online_cpus > 1)
>
> But I guess the question comes. If we are running on zero CPUS, should
> we perform the "on_each_cpu(do_sync_core, NULL, 1);" or not? Same goes
> with 5i-3 CPUS, or negative number CPUs. If we need to do on_each_cpu(),
> then I guess the != 1 will suffice.
Makes sense to me! Whoever adds the ability to run on zero, negative,
or complex numbers of CPUs can adjust on_each_cpu() accordingly.
Thanx, Paul
(2013/10/21 0:42), Paul E. McKenney wrote:
> On Sat, Oct 19, 2013 at 06:02:39PM -0400, Steven Rostedt wrote:
>> On Sat, 19 Oct 2013 14:33:50 -0700
>> "Paul E. McKenney" <[email protected]> wrote:
>>
>>
>>>>> It's used to convert the calls to mcount to nops. But maybe a better
>>>>> thing to do is to check if we only have a single CPU:
>>>>>
>>>>> static void run_sync(void)
>>>>> {
>>>>> if (num_online_cpus() != 1)
>>>>
>>>> Hmm, to be more robust to handle our future "ideal" machines, perhaps
>>>> this should be:
>>>>
>>>> /* Ideally we would like to run on zero CPUS! */
>>>> if (num_online_cpus() < 2)
>>>
>>
>> Bah! And for such a simple computation, I got it wrong.
>>
>>
>> /* Ideally we would like to run on zero CPUS! */
>> if (num_online_cpus > 1)
>>
>> But I guess the question comes. If we are running on zero CPUS, should
>> we perform the "on_each_cpu(do_sync_core, NULL, 1);" or not? Same goes
>> with 5i-3 CPUS, or negative number CPUs. If we need to do on_each_cpu(),
>> then I guess the != 1 will suffice.
>
> Makes sense to me! Whoever adds the ability to run on zero, negative,
> or complex numbers of CPUs can adjust on_each_cpu() accordingly.
Thanks for making it clear!
Petr, could you update your patch according to this discussion?
I think temporally enabling irq is not a good idea.
BTW, adding an assertion(BUG_ON(irq_disabled()) at the top of text_poke_bp)
will be good for debugging.
Thanks again!
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]