Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1281745 > unrolled thread

Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable tracing

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Will Deacon <will.deacon@arm.com> - 2015-12-02 13:40 +0100
    Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Will Deacon <will.deacon@arm.com> - 2015-12-02 14:20 +0100
      Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable  tracing libin <huawei.libin@huawei.com> - 2015-12-03 10:50 +0100
        Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Will Deacon <will.deacon@arm.com> - 2015-12-03 12:50 +0100
          Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Steven Rostedt <rostedt@goodmis.org> - 2015-12-03 16:10 +0100
    Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Steven Rostedt <rostedt@goodmis.org> - 2015-12-02 15:10 +0100
    Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable  tracing libin <huawei.libin@huawei.com> - 2015-12-03 10:30 +0100
      Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Will Deacon <will.deacon@arm.com> - 2015-12-03 10:40 +0100
        Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Will Deacon <will.deacon@arm.com> - 2015-12-03 16:10 +0100
          Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Steven Rostedt <rostedt@goodmis.org> - 2015-12-03 16:40 +0100
            Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable  tracing libin <huawei.libin@huawei.com> - 2015-12-04 02:10 +0100
        Re: [PATCH] arm64: ftrace: stop using kstop_machine to  enable/disable tracing Steven Rostedt <rostedt@goodmis.org> - 2015-12-03 16:10 +0100

#1281745 — Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable tracing

On Sat, Nov 28, 2015 at 03:50:09PM +0800, Li Bin wrote:
> On arm64, kstop_machine which is hugely disruptive to a running
> system is not needed to convert nops to ftrace calls or back,
> because that modifed code is a single 32bit instructions which
> is impossible to cross cache (or page) boundaries, and the used str
> instruction is single-copy atomic.

This commit message is misleading, since the single-copy atomicity
guarantees don't apply to the instruction-side. Instead, the architecture
calls out a handful of safe instructions in "Concurrent modification and
execution of instructions".

Now, those safe instructions *do* include NOP, B and BL, so that should
be sufficient for ftrace provided that we don't patch condition codes
(and I don't think we do).

> Cc: <stable@vger.kernel.org> # 3.18+

I don't think this is stable material.

Will

> Signed-off-by: Li Bin <huawei.libin@huawei.com>
> ---
>  arch/arm64/kernel/ftrace.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index c851be7..9669b33 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -93,6 +93,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>  	return ftrace_modify_code(pc, old, new, true);
>  }
>  
> +void arch_ftrace_update_code(int command)
> +{
> +	ftrace_modify_all_code(command);
> +}
> +
>  int __init ftrace_dyn_arch_init(void)
>  {
>  	return 0;
> -- 
> 1.7.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1281774

On Wed, Dec 02, 2015 at 12:36:54PM +0000, Will Deacon wrote:
> On Sat, Nov 28, 2015 at 03:50:09PM +0800, Li Bin wrote:
> > On arm64, kstop_machine which is hugely disruptive to a running
> > system is not needed to convert nops to ftrace calls or back,
> > because that modifed code is a single 32bit instructions which
> > is impossible to cross cache (or page) boundaries, and the used str
> > instruction is single-copy atomic.
> 
> This commit message is misleading, since the single-copy atomicity
> guarantees don't apply to the instruction-side. Instead, the architecture
> calls out a handful of safe instructions in "Concurrent modification and
> execution of instructions".
> 
> Now, those safe instructions *do* include NOP, B and BL, so that should
> be sufficient for ftrace provided that we don't patch condition codes
> (and I don't think we do).

Thinking about this some more, you also need to fix the validate=1 case
in ftrace_modify_code so that it can run outside of stop_machine. We
currently rely on that to deal with concurrent modifications (e.g.
module unloading).

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282870 — Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable tracing

on 2015/12/2 21:16, Will Deacon wrote:
> On Wed, Dec 02, 2015 at 12:36:54PM +0000, Will Deacon wrote:
>> On Sat, Nov 28, 2015 at 03:50:09PM +0800, Li Bin wrote:
>>> On arm64, kstop_machine which is hugely disruptive to a running
>>> system is not needed to convert nops to ftrace calls or back,
>>> because that modifed code is a single 32bit instructions which
>>> is impossible to cross cache (or page) boundaries, and the used str
>>> instruction is single-copy atomic.
>> This commit message is misleading, since the single-copy atomicity
>> guarantees don't apply to the instruction-side. Instead, the architecture
>> calls out a handful of safe instructions in "Concurrent modification and
>> execution of instructions".
>>
>> Now, those safe instructions *do* include NOP, B and BL, so that should
>> be sufficient for ftrace provided that we don't patch condition codes
>> (and I don't think we do).
> Thinking about this some more, you also need to fix the validate=1 case
> in ftrace_modify_code so that it can run outside of stop_machine. We
> currently rely on that to deal with concurrent modifications (e.g.
> module unloading).

I'm not sure it is really a problem, but on x86, which using breakpoints method,
add_break() that run outside of stop_machine also has similar code.

static int add_break(unsigned long ip, const char *old)
{
        unsigned char replaced[MCOUNT_INSN_SIZE];
        unsigned char brk = BREAKPOINT_INSTRUCTION;

        if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
                return -EFAULT;

        /* Make sure it is what we expect it to be */
        if (memcmp(replaced, old, MCOUNT_INSN_SIZE) != 0)
                return -EINVAL;

        return ftrace_write(ip, &brk, 1);
}

Or I misunderstand what you mean?

Thanks,
Li Bin

> Will
>
> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282972

On Thu, Dec 03, 2015 at 05:39:56PM +0800, libin wrote:
> on 2015/12/2 21:16, Will Deacon wrote:
> > On Wed, Dec 02, 2015 at 12:36:54PM +0000, Will Deacon wrote:
> >> On Sat, Nov 28, 2015 at 03:50:09PM +0800, Li Bin wrote:
> >>> On arm64, kstop_machine which is hugely disruptive to a running
> >>> system is not needed to convert nops to ftrace calls or back,
> >>> because that modifed code is a single 32bit instructions which
> >>> is impossible to cross cache (or page) boundaries, and the used str
> >>> instruction is single-copy atomic.
> >> This commit message is misleading, since the single-copy atomicity
> >> guarantees don't apply to the instruction-side. Instead, the architecture
> >> calls out a handful of safe instructions in "Concurrent modification and
> >> execution of instructions".
> >>
> >> Now, those safe instructions *do* include NOP, B and BL, so that should
> >> be sufficient for ftrace provided that we don't patch condition codes
> >> (and I don't think we do).
> > Thinking about this some more, you also need to fix the validate=1 case
> > in ftrace_modify_code so that it can run outside of stop_machine. We
> > currently rely on that to deal with concurrent modifications (e.g.
> > module unloading).
> 
> I'm not sure it is really a problem, but on x86, which using breakpoints method,
> add_break() that run outside of stop_machine also has similar code.

Yeah, having now read through that, I also can't see any locking issues.
We should remove the comment suggesting otherwise.

> static int add_break(unsigned long ip, const char *old)
> {
>         unsigned char replaced[MCOUNT_INSN_SIZE];
>         unsigned char brk = BREAKPOINT_INSTRUCTION;
> 
>         if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
>                 return -EFAULT;
> 
>         /* Make sure it is what we expect it to be */
>         if (memcmp(replaced, old, MCOUNT_INSN_SIZE) != 0)
>                 return -EINVAL;
> 
>         return ftrace_write(ip, &brk, 1);
> }
> 
> Or I misunderstand what you mean?

Hmm, so this should all be fine if we exclusively use the probe_kernel_*
functions and handle the -EFAULT gracefully. Now, that leaves an
interesting scenario with the flush_icache_range call in
aarch64_insn_patch_text_nosync, since that's not run with
KERNEL_DS/pagefault_disable() and so we'll panic if the text disappears
underneath us.

So we probably need to add that code and call __flush_cache_user_range
instead.

What do you think?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283104

On Thu, 3 Dec 2015 11:48:24 +0000
Will Deacon <will.deacon@arm.com> wrote:

> Hmm, so this should all be fine if we exclusively use the probe_kernel_*
> functions and handle the -EFAULT gracefully. Now, that leaves an
> interesting scenario with the flush_icache_range call in
> aarch64_insn_patch_text_nosync, since that's not run with
> KERNEL_DS/pagefault_disable() and so we'll panic if the text disappears
> underneath us.

Nothing should remove the text from underneath you if everything
matches up fine before that. Module unloading will block on the
ftrace_lock if it has any functions that can be traced.

-- Steve


> 
> So we probably need to add that code and call __flush_cache_user_range
> instead.
> 
> What do you think?
> 
> Will

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281831

On Wed, 2 Dec 2015 12:36:55 +0000
Will Deacon <will.deacon@arm.com> wrote:


> > Cc: <stable@vger.kernel.org> # 3.18+  
> 
> I don't think this is stable material.
> 

No, it definitely isn't. This is a new feature not a fix.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282836 — Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable tracing

on 2015/12/2 20:36, Will Deacon wrote:
> On Sat, Nov 28, 2015 at 03:50:09PM +0800, Li Bin wrote:
>> On arm64, kstop_machine which is hugely disruptive to a running
>> system is not needed to convert nops to ftrace calls or back,
>> because that modifed code is a single 32bit instructions which
>> is impossible to cross cache (or page) boundaries, and the used str
>> instruction is single-copy atomic.
> This commit message is misleading, since the single-copy atomicity
> guarantees don't apply to the instruction-side. Instead, the architecture
> calls out a handful of safe instructions in "Concurrent modification and
> execution of instructions".

Right, thank you for your comments.

> Now, those safe instructions *do* include NOP, B and BL, so that should
> be sufficient for ftrace provided that we don't patch condition codes
> (and I don't think we do).

Yes, and so far this assumption has no probem, but in order to avoid exceeding these
safe insturctions in the future, we can use aarch64_insn_hotpatch_safe() to verify the
instruction to determine whether needs stop_machine() to synchronize or use
aarch64_insn_patch_text directly. Right or I am missing something?

Thanks,
Li Bin

>> Cc: <stable@vger.kernel.org> # 3.18+
> I don't think this is stable material.
>
> Will
>
>> Signed-off-by: Li Bin <huawei.libin@huawei.com>
>> ---
>>  arch/arm64/kernel/ftrace.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
>> index c851be7..9669b33 100644
>> --- a/arch/arm64/kernel/ftrace.c
>> +++ b/arch/arm64/kernel/ftrace.c
>> @@ -93,6 +93,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>>  	return ftrace_modify_code(pc, old, new, true);
>>  }
>>  
>> +void arch_ftrace_update_code(int command)
>> +{
>> +	ftrace_modify_all_code(command);
>> +}
>> +
>>  int __init ftrace_dyn_arch_init(void)
>>  {
>>  	return 0;
>> -- 
>> 1.7.1
>>
> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1282851

On Thu, Dec 03, 2015 at 05:21:22PM +0800, libin wrote:
> 
> on 2015/12/2 20:36, Will Deacon wrote:
> > On Sat, Nov 28, 2015 at 03:50:09PM +0800, Li Bin wrote:
> >> On arm64, kstop_machine which is hugely disruptive to a running
> >> system is not needed to convert nops to ftrace calls or back,
> >> because that modifed code is a single 32bit instructions which
> >> is impossible to cross cache (or page) boundaries, and the used str
> >> instruction is single-copy atomic.
> > This commit message is misleading, since the single-copy atomicity
> > guarantees don't apply to the instruction-side. Instead, the architecture
> > calls out a handful of safe instructions in "Concurrent modification and
> > execution of instructions".
> 
> Right, thank you for your comments.
> 
> > Now, those safe instructions *do* include NOP, B and BL, so that should
> > be sufficient for ftrace provided that we don't patch condition codes
> > (and I don't think we do).
> 
> Yes, and so far this assumption has no probem, but in order to avoid exceeding these
> safe insturctions in the future, we can use aarch64_insn_hotpatch_safe() to verify the
> instruction to determine whether needs stop_machine() to synchronize or use
> aarch64_insn_patch_text directly. Right or I am missing something?

I think you're missing the case where the instruction changes under our
feet after we've read it but before we've replaced it (e.g. due to module
unloading). I think that's why ftrace_modify_code has the comment about
lack of locking thanks to stop_machine.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283106

On Thu, Dec 03, 2015 at 10:05:25AM -0500, Steven Rostedt wrote:
> On Thu, 3 Dec 2015 09:38:21 +0000
> Will Deacon <will.deacon@arm.com> wrote:
> > I think you're missing the case where the instruction changes under our
> > feet after we've read it but before we've replaced it (e.g. due to module
> > unloading). I think that's why ftrace_modify_code has the comment about
> > lack of locking thanks to stop_machine.
> 
> Note, ftrace has a module notifier that is called when a module is
> being unloaded and before the text goes away. This code grabs the
> ftrace_lock mutex and removes the module functions from the ftrace
> list, such that it will no longer do any modifications to that module's
> text.
> 
> The update to make functions be traced is done under the ftrace_lock
> mutex as well.
> 
> You do not need to worry about module text disappearing from
> underneath you while you do your modifications.

Good.

> Now, if there's comments that suggest otherwise, they need to be
> updated.

Yeah, I think the comments on x86 and arm64 are out of date. They also
mention the freeing of __init sections -- is that still a concern?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283138

On Thu, 3 Dec 2015 15:09:26 +0000
Will Deacon <will.deacon@arm.com> wrote:

> Yeah, I think the comments on x86 and arm64 are out of date. They also
> mention the freeing of __init sections -- is that still a concern?

No we black list them, any section that we are not sure will be there
when we expect it to has recordmcount.c nop out the calls to mcount and
they are ignored. Remember the e1000e bug? This was the reproducer.

Anyway, on my todo list is to allow init sections to be traced. To do
so would mean that I need to add generic code that lets ftrace know to
remove init sections at boot up. Right now (or at least the last time I
checked, which was back in 2009), every arch had its own way of freeing
init memory. If that has changed, or I can just place a hook where it
happens (which is probably the easy part), I can allow init code to be
traced too.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283489 — Re: [PATCH] arm64: ftrace: stop using kstop_machine to enable/disable tracing

Thanks very much to Will and Steve for the wonderful comments, I will modify the
commit message, and remove the misleading comments about module text
disappearing case.

Thanks again,
Li Bin

on 2015/12/3 23:31, Steven Rostedt wrote:
> On Thu, 3 Dec 2015 15:09:26 +0000
> Will Deacon <will.deacon@arm.com> wrote:
>
>> Yeah, I think the comments on x86 and arm64 are out of date. They also
>> mention the freeing of __init sections -- is that still a concern?
> No we black list them, any section that we are not sure will be there
> when we expect it to has recordmcount.c nop out the calls to mcount and
> they are ignored. Remember the e1000e bug? This was the reproducer.
>
> Anyway, on my todo list is to allow init sections to be traced. To do
> so would mean that I need to add generic code that lets ftrace know to
> remove init sections at boot up. Right now (or at least the last time I
> checked, which was back in 2009), every arch had its own way of freeing
> init memory. If that has changed, or I can just place a hook where it
> happens (which is probably the easy part), I can allow init code to be
> traced too.
>
> -- Steve
>
>
> .
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283111

On Thu, 3 Dec 2015 09:38:21 +0000
Will Deacon <will.deacon@arm.com> wrote:


> I think you're missing the case where the instruction changes under our
> feet after we've read it but before we've replaced it (e.g. due to module
> unloading). I think that's why ftrace_modify_code has the comment about
> lack of locking thanks to stop_machine.

Note, ftrace has a module notifier that is called when a module is
being unloaded and before the text goes away. This code grabs the
ftrace_lock mutex and removes the module functions from the ftrace
list, such that it will no longer do any modifications to that module's
text.

The update to make functions be traced is done under the ftrace_lock
mutex as well.

You do not need to worry about module text disappearing from
underneath you while you do your modifications.

Now, if there's comments that suggest otherwise, they need to be
updated.

-- Steve
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web