by Mark Rutland

[permalink] [raw]

Subject: Re: [PATCH v3 1/8] ftrace: Add DYNAMIC_FTRACE_WITH_CALL_OPS

On Tue, Jan 31, 2023 at 09:25:51AM +0800, Wangshaobo (bobo) wrote:
> 在 2023/1/30 18:25, Mark Rutland 写道:
> > On Sat, Jan 28, 2023 at 04:46:48PM +0800, Wangshaobo (bobo) wrote:
> > > 锟斤拷 2023/1/23 21:45, Mark Rutland 写锟斤拷:
> > > > +config DYNAMIC_FTRACE_WITH_CALL_OPS
> > > > + def_bool y
> > > > + depends on HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
> > > > +
> > > Hi Mark,
> >
> > Hi,
> >
> > > I have test your patches and it looks fine with my sample module,
> >
> > Thanks for testing!
> >
> > > but here setting DYNAMIC_FTRACE_WITH_CALL_OPS to y immutably may increase the
> > > .text section size by 5% or more, how about making this to optional^^
> >
> > We could consider making this optional. I had not made this optional so far as
> > in the future I'd like to make this the only implementation of ftrace on arm64
> > (once we can drop the old mcount version, and once we've sorted out the
> > incompatibility with CFI). In the mean time, it probably makes sense to have
> > the option at least to enable testing of each of the two forms.
> >
> > Is your concern that the overall kernel image size is larger, or do you care
> > specifically about the size of the .text section for some reason?
> >
> > Thanks,
> > Mark
> Embedded devices may pay more attention to Image size, and which may also
> indirectly affects performance, for more reason,

I appreciate those concerns, however:

a) For the Image size, the mcount_loc table and associated relocations already
imposes a much greater penalty. So I'd expect that where the size truly
matters, ftrace would be completely disabled anyway.

I'm currently looking at shrinking the mcount_loc table (and removing the
need for relocationgs), which should save much more space.

b) For performance, without data this is supposition. Everything so far
indicates that there is not a measureable performance difference, and from
other threads it's possible that the increased function alignment *aids*
performance.

If you have data to the contrary, I'm happy to investigate.

> I think making sense to have the option for testing is more important.

As above, I'm happy to add an option for functional testing of the ftrace
implementation, but I don't think that it's a good idea to use that as a size
or performance tweak.

Thanks,
Mark.

2023-02-23 09:28:56

by Chen-Yu Tsai

[permalink] [raw]

Subject: Re: [PATCH v3 0/8] arm64/ftrace: Add support for DYNAMIC_FTRACE_WITH_CALL_OPS

On Mon, Jan 23, 2023 at 01:45:55PM +0000, Mark Rutland wrote:
> Hi Catalin, Steve,
>
> I'm not sure how we want to merge this, so I've moved the core ftrace
> patch to the start of the series so that it can more easily be placed on
> a stable branch if we want that to go via the ftrace tree and the rest
> to go via arm64.
>
> This is cleanly pasing the ftrace selftests from v6.2-rc3 (results in
> the final patch).
>
> Aside from that, usual cover letter below.
>
> This series adds a new DYNAMIC_FTRACE_WITH_CALL_OPS mechanism, and
> enables support for this on arm64. This significantly reduces the
> overhead of tracing when a callsite/tracee has a single associated
> tracer, avoids a number of issues that make it undesireably and
> infeasible to use dynamically-allocated trampolines (e.g. branch range
> limitations), and makes it possible to implement support for
> DYNAMIC_FTRACE_WITH_DIRECT_CALLS in future.
>
> The main idea is to give each ftrace callsite an associated pointer to
> an ftrace_ops. The architecture's ftrace_caller trampoline can recover
> the ops pointer and invoke ops->func from this without needing to use
> ftrace_ops_list_func, which has to iterate through all registered ops.
>
> To make this work, we use -fpatchable-function-entry=M,N, there N NOPs
> are placed before the function entry point. On arm64 NOPs are always 4
> bytes, so by allocating 2 per-function NOPs, we have enough space to
> place a 64-bit value. So that we can manipulate the pointer atomically,
> we need to align instrumented functions to at least 8 bytes, which we
> can ensure using -falign-functions=8.

Does this work all the time? Or is it influenced by other Kconfig
options?

I'm getting random misaligned patch-site warnings like the following:

Misaligned patch-site gic_handle_irq+0x0/0x12c
Misaligned patch-site __traceiter_initcall_level+0x0/0x60
Misaligned patch-site __traceiter_initcall_start+0x0/0x60
Misaligned patch-site __traceiter_initcall_finish+0x0/0x68
Misaligned patch-site do_one_initcall+0x0/0x300
Misaligned patch-site do_one_initcall+0x2b0/0x300
Misaligned patch-site match_dev_by_label+0x0/0x50
Misaligned patch-site match_dev_by_uuid+0x0/0x48
Misaligned patch-site wait_for_initramfs+0x0/0x68
Misaligned patch-site panic_show_mem+0x0/0x88
Misaligned patch-site 0xffffffd3b4fef074
[...]
(I assume the unresolved symbol(s) are from modules.)

The warnings were seen on next-20230223 and many versions before, with
Debian's GCC 12.2 cross compile toolchain. I also tried next-20230223
with Linaro's toolchains gcc-linaro-12.2.1-2023.01-x86_64_aarch64-linux-gnu
and gcc-linaro-13.0.0-2022.11-x86_64_aarch64-linux-gnu and the warnings
appeared as well.

Checking panic_show_mem in various places from the Debian GCC's build:

$ aarch64-linux-gnu-nm init/initramfs.o | grep panic_show_mem
0000000000000070 t panic_show_mem
$ aarch64-linux-gnu-nm init/built-in.a | grep panic_show_mem
0000000000000070 t panic_show_mem
$ aarch64-linux-gnu-nm built-in.a | grep panic_show_mem
0000000000000070 t panic_show_mem
$ aarch64-linux-gnu-nm vmlinux.a | grep panic_show_mem
0000000000000070 t panic_show_mem
$ aarch64-linux-gnu-nm vmlinux.o | grep panic_show_mem
0000000000001534 t panic_show_mem
$ aarch64-linux-gnu-nm vmlinux | grep panic_show_mem
ffffffc0080158dc t panic_show_mem

Looks like individual object files do have functions aligned at 8-byte
boundaries, but when all the object files are collected and linked
together into vmlinux.o, the higher alignment gets dropped and some
functions end up on 4-byte boundaries.

Regards
ChenYu

>
> Each callsite ends up looking like:
>
> # Aligned to 8 bytes
> func - 8:
> < pointer to ops >
> func:
> BTI // optional
> MOV X9, LR
> NOP // patched to `BL ftrace_caller`
> func_body:
>
> When entering ftrace_caller, the LR points at func_body, and the
> ftrace_ops can be recovered at a negative offset from this the LR value:
>
> BIC <tmp>, LR, 0x7 // Align down (skips BTI)
> LDR <tmp>, [<tmp>, #-16] // load ops pointer
>
> The ftrace_ops::func (and any other ftrace_ops fields) can then be
> recovered from this pointer to the ops.
>
> The first three patches enable the function alignment, working around
> cases where GCC drops alignment for cold functions or when building with
> '-Os'.
>
> The final four patches implement support for
> DYNAMIC_FTRACE_WITH_CALL_OPS on arm64. As noted in the final patch, this
> results in a significant reduction in overhead:
>
> Before this series:
>
> Number of tracers || Total time | Per-call average time (ns)
> Relevant | Irrelevant || (ns) | Total | Overhead
> =========+============++=============+==============+============
> 0 | 0 || 94,583 | 0.95 | -
> 0 | 1 || 93,709 | 0.94 | -
> 0 | 2 || 93,666 | 0.94 | -
> 0 | 10 || 93,709 | 0.94 | -
> 0 | 100 || 93,792 | 0.94 | -
> ---------+------------++-------------+--------------+------------
> 1 | 1 || 6,467,833 | 64.68 | 63.73
> 1 | 2 || 7,509,708 | 75.10 | 74.15
> 1 | 10 || 23,786,792 | 237.87 | 236.92
> 1 | 100 || 106,432,500 | 1,064.43 | 1063.38
> ---------+------------++-------------+--------------+------------
> 1 | 0 || 1,431,875 | 14.32 | 13.37
> 2 | 0 || 6,456,334 | 64.56 | 63.62
> 10 | 0 || 22,717,000 | 227.17 | 226.22
> 100 | 0 || 103,293,667 | 1032.94 | 1031.99
> ---------+------------++-------------+--------------+--------------
>
> Note: per-call overhead is estiamated relative to the baseline case
> with 0 relevant tracers and 0 irrelevant tracers.
>
> After this series:
>
> Number of tracers || Total time | Per-call average time (ns)
> Relevant | Irrelevant || (ns) | Total | Overhead
> =========+============++=============+==============+============
> 0 | 0 || 94,541 | 0.95 | -
> 0 | 1 || 93,666 | 0.94 | -
> 0 | 2 || 93,709 | 0.94 | -
> 0 | 10 || 93,667 | 0.94 | -
> 0 | 100 || 93,792 | 0.94 | -
> ---------+------------++-------------+--------------+------------
> 1 | 1 || 281,000 | 2.81 | 1.86
> 1 | 2 || 281,042 | 2.81 | 1.87
> 1 | 10 || 280,958 | 2.81 | 1.86
> 1 | 100 || 281,250 | 2.81 | 1.87
> ---------+------------++-------------+--------------+------------
> 1 | 0 || 280,959 | 2.81 | 1.86
> 2 | 0 || 6,502,708 | 65.03 | 64.08
> 10 | 0 || 18,681,209 | 186.81 | 185.87
> 100 | 0 || 103,550,458 | 1,035.50 | 1034.56
> ---------+------------++-------------+--------------+------------
>
> Note: per-call overhead is estiamated relative to the baseline case
> with 0 relevant tracers and 0 irrelevant tracers.
>
>
> This version of the series can be found in my kernel.org git repo:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git
>
> Tagged as:
>
> arm64-ftrace-per-callsite-ops-20230113
>
> Since v1 [1]:
> * Fold in Ack from Rafael
> * Update comments/commits with description of the GCC issue
> * Move the cold attribute changes to compiler_types.h
> * Drop the unnecessary changes to the weak attribute
> * Move declaration of ftrace_ops earlier
> * Clean up and improve commit messages
> * Regenerate statistics on misaligned text symbols
>
> Since v2 [2]:
> * Fold in Steve's Reviewed-by tag
> * Move core ftrace patch to the start of the series
> * Add ftrace selftest reults to final patch
> * Use FUNCTION_ALIGNMENT_4B by default
> * Fix commit message typos
>
> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
> [2] https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> Thanks,
> Mark.
>
> Mark Rutland (8):
> ftrace: Add DYNAMIC_FTRACE_WITH_CALL_OPS
> Compiler attributes: GCC cold function alignment workarounds
> ACPI: Don't build ACPICA with '-Os'
> arm64: Extend support for CONFIG_FUNCTION_ALIGNMENT
> arm64: insn: Add helpers for BTI
> arm64: patching: Add aarch64_insn_write_literal_u64()
> arm64: ftrace: Update stale comment
> arm64: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
>
> arch/arm64/Kconfig | 4 +
> arch/arm64/Makefile | 5 +-
> arch/arm64/include/asm/ftrace.h | 15 +--
> arch/arm64/include/asm/insn.h | 1 +
> arch/arm64/include/asm/linkage.h | 4 +-
> arch/arm64/include/asm/patching.h | 2 +
> arch/arm64/kernel/asm-offsets.c | 4 +
> arch/arm64/kernel/entry-ftrace.S | 32 +++++-
> arch/arm64/kernel/ftrace.c | 158 +++++++++++++++++++++++++++-
> arch/arm64/kernel/patching.c | 17 +++
> drivers/acpi/acpica/Makefile | 2 +-
> include/linux/compiler_attributes.h | 6 --
> include/linux/compiler_types.h | 27 +++++
> include/linux/ftrace.h | 18 +++-
> kernel/exit.c | 9 +-
> kernel/trace/Kconfig | 7 ++
> kernel/trace/ftrace.c | 109 ++++++++++++++++++-
> 17 files changed, 380 insertions(+), 40 deletions(-)
>
> --
> 2.30.2
>
>