| Message ID | 20191029165832.33606-1-mark.rutland@arm.com |
|---|---|
| Headers | show |
| Series | arm64: ftrace cleanup + FTRACE_WITH_REGS | expand |
Whole series, on top of Linus' HEAD 320000e72ec0613e164ce9 (5.4.0-rc5)
[ 0.418079] Testing dynamic ftrace: PASSED
[ 0.670416] Testing dynamic ftrace ops #1:
[ 0.751367] (1 0 1 0 0)
[ 0.751374] (1 1 2 0 0)
[ 0.857303] (2 1 3 0 281230)
[ 0.857327] (2 2 4 0 281332) PASSED
[ 0.930124] Testing dynamic ftrace ops #2:
[ 1.110333] (1 0 1 281189 0)
[ 1.110360] (1 1 2 281329 0)
[ 1.110815] (2 1 3 1 2)
[ 1.110841] (2 2 4 113 114) PASSED
[ 1.170653] Testing ftrace recursion: PASSED
[ 1.192250] Testing ftrace recursion safe: PASSED
[ 1.213819] Testing ftrace regs: PASSED
[ 1.235397] Testing tracer nop: PASSED
[ 1.235404] Testing tracer wakeup: PASSED
[ 1.363921] Testing tracer wakeup_rt: PASSED
[ 1.494283] Testing tracer wakeup_dl: PASSED
[ 1.623948] Testing tracer function_graph: PASSED
# tracer: function_graph
#
# CPU DURATION FUNCTION CALLS
# | | | | | | |
0) | wake_up_process() {
0) | try_to_wake_up() {
0) | select_task_rq_fair() {
0) | select_idle_sibling() {
0) 3.360 us | available_idle_cpu();
0) + 10.940 us | }
[...]
The graph tracer is the trickiest part to get working correctly, from my
experience. IOW: everything looks fine.
Whole series,
Tested-by: Torsten Duwe <duwe@suse.de>
Torsten
After yesterday's testing, now
Reviewed-by: Torsten Duwe <duwe@suse.de>
for the series.
Executive summary: where I used x28 as scratch register in ftrace_regs_caller
which I had to save for that reason, you switched to x10, which is so obvious
that I failed to see it. Then the PLT initialisation on module load, and
finally the ftrace_init_nop() hook that got you started initially. The rest
I'd call more or less cosmetic deviations from my v8. IOW: fine with me.
Torsten
On Thu, Oct 31, 2019 at 06:16:41PM +0100, Torsten Duwe wrote: > After yesterday's testing, now > > Reviewed-by: Torsten Duwe <duwe@suse.de> > > for the series. Thanks! I've folded that in and pushed out the updated branch. Since the only change this time around was only a trivial change in the linker script, I'll hold off sending a v3. I'm hoping I can get acks for the ftrace, module, and parisc bits soon... > Executive summary: where I used x28 as scratch register in ftrace_regs_caller > which I had to save for that reason, you switched to x10, which is so obvious > that I failed to see it. Then the PLT initialisation on module load, and > finally the ftrace_init_nop() hook that got you started initially. The rest > I'd call more or less cosmetic deviations from my v8. IOW: fine with me. Yup, that sounds about right. The other thing I did was expand the comments on the ABI details, as that can be quite subtle, but I guess that's arguably cosmetic, too. Thanks for working on this, and for bearing with me on this rework -- I hadn't intended that to drag on for so long. Thanks, Mark.
Hi Mark, On Tue, Oct 29, 2019 at 04:58:24PM +0000, Mark Rutland wrote: > Hi, > > This series is a reworked version of Torsten's FTRACE_WITH_REGS series > [1]. I've tried to rework the existing code in preparatory patches so > that the patchable-function-entry bits slot in with fewer surprises. > This version is based on v5.4-rc3, and can be found in my > arm64/ftrace-with-regs branch [2]. > > Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses > to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR > symbol, and more cleanly separates the one-time initialization of the > callsite from dynamic NOP<->CALL modification. Architectures which don't > implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. > > Recently parisc gained ftrace support using patchable-function-entry. > Patch 2 makes the handling of module callsite locations common in > kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and > removed the newly redundant bits from arch/parisc. > > Patches 3 and 4 move the module PLT initialization to module load time, > which simplifies runtime callsite modification. This also means that we > don't transitently mark the module text RW, and will allow for the > removal of module_disable_ro(). > > Patches 5 and 6 add some trivial infrastructure, with patch 7 finally > adding FTRACE_WITH_REGS support. Additional work will be required for > livepatching (e.g. implementing reliable stack trace), which is > commented as part of patch 7. > > Patch 8 is a trivial cleanup atop of the rest of the series, making the > code easier to read and less susceptible to config-specific breakage. > > Since v1 [3]: > * Add a couple of people to Cc > * Fold in Ard's Reviewed-by tag > * Rename ftrace_code_init_disabled() to ftrace_nop_initialize() > * Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc > * Update kerneldoc for rec parameters [..] I tested this series on parisc both with ftracing kernel internal functions and module functions. Both are working fine, so feel free to add my Tested-by: Sven Schnelle <svens@stackframe.org> Regards Sven
On Fri, Nov 01, 2019 at 04:39:30PM +0100, Sven Schnelle wrote: > On Tue, Oct 29, 2019 at 04:58:24PM +0000, Mark Rutland wrote: > > This series is a reworked version of Torsten's FTRACE_WITH_REGS series > > [1]. I've tried to rework the existing code in preparatory patches so > > that the patchable-function-entry bits slot in with fewer surprises. > > This version is based on v5.4-rc3, and can be found in my > > arm64/ftrace-with-regs branch [2]. > > > > Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses > > to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR > > symbol, and more cleanly separates the one-time initialization of the > > callsite from dynamic NOP<->CALL modification. Architectures which don't > > implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. > > > > Recently parisc gained ftrace support using patchable-function-entry. > > Patch 2 makes the handling of module callsite locations common in > > kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and > > removed the newly redundant bits from arch/parisc. > > Since v1 [3]: > > * Add a couple of people to Cc > > * Fold in Ard's Reviewed-by tag > > * Rename ftrace_code_init_disabled() to ftrace_nop_initialize() > > * Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc > > * Update kerneldoc for rec parameters > [..] > > I tested this series on parisc both with ftracing kernel internal functions and > module functions. Both are working fine, so feel free to add my > > Tested-by: Sven Schnelle <svens@stackframe.org> Thanks! That's much appreciated. I've applied that to patches 1 and 2, since the remainder of the series was confined to arch/arm64/. Mark.
Hi Mark, On 10/29/19 10:28 PM, Mark Rutland wrote: > Hi, > > This series is a reworked version of Torsten's FTRACE_WITH_REGS series > [1]. I've tried to rework the existing code in preparatory patches so > that the patchable-function-entry bits slot in with fewer surprises. > This version is based on v5.4-rc3, and can be found in my > arm64/ftrace-with-regs branch [2]. > > Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses > to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR > symbol, and more cleanly separates the one-time initialization of the > callsite from dynamic NOP<->CALL modification. Architectures which don't > implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. > > Recently parisc gained ftrace support using patchable-function-entry. > Patch 2 makes the handling of module callsite locations common in > kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and > removed the newly redundant bits from arch/parisc. > > Patches 3 and 4 move the module PLT initialization to module load time, > which simplifies runtime callsite modification. This also means that we > don't transitently mark the module text RW, and will allow for the > removal of module_disable_ro(). > > Patches 5 and 6 add some trivial infrastructure, with patch 7 finally > adding FTRACE_WITH_REGS support. Additional work will be required for > livepatching (e.g. implementing reliable stack trace), which is > commented as part of patch 7. > > Patch 8 is a trivial cleanup atop of the rest of the series, making the > code easier to read and less susceptible to config-specific breakage. I tested the whole series with my latest in-kernel ptrauth patches [1] and graph_tracer/function_graph_tracer works fine, So for the whole series, Tested-by: Amit Daniel Kachhap <amit.kachhap@arm.com> Also I gave few minor comments in the individual patches. With those comments, Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> Thanks, Amit Daniel [1]: https://patchwork.kernel.org/cover/11195085/ > > Since v1 [3]: > * Add a couple of people to Cc > * Fold in Ard's Reviewed-by tag > * Rename ftrace_code_init_disabled() to ftrace_nop_initialize() > * Move ftrace_init_nop() to <linux/ftrace.h>, with kerneldoc > * Update kerneldoc for rec parameters > > Thanks, > Mark. > > [1] https://lore.kernel.org/r/20190208150826.44EBC68DD2@newverein.lst.de > [2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs > [3] https://lore.kernel.org/r/20191021163426.9408-1-mark.rutland@arm.com > > Mark Rutland (7): > ftrace: add ftrace_init_nop() > module/ftrace: handle patchable-function-entry > arm64: module: rework special section handling > arm64: module/ftrace: intialize PLT at load time > arm64: insn: add encoder for MOV (register) > arm64: asm-offsets: add S_FP > arm64: ftrace: minimize ifdeffery > > Torsten Duwe (1): > arm64: implement ftrace with regs > > arch/arm64/Kconfig | 2 + > arch/arm64/Makefile | 5 ++ > arch/arm64/include/asm/ftrace.h | 23 +++++++ > arch/arm64/include/asm/insn.h | 3 + > arch/arm64/include/asm/module.h | 2 +- > arch/arm64/kernel/asm-offsets.c | 1 + > arch/arm64/kernel/entry-ftrace.S | 140 +++++++++++++++++++++++++++++++++++++-- > arch/arm64/kernel/ftrace.c | 123 ++++++++++++++++++++-------------- > arch/arm64/kernel/insn.c | 13 ++++ > arch/arm64/kernel/module-plts.c | 3 +- > arch/arm64/kernel/module.c | 57 +++++++++++++--- > arch/parisc/Makefile | 1 - > arch/parisc/kernel/module.c | 10 ++- > arch/parisc/kernel/module.lds | 7 -- > include/linux/ftrace.h | 40 ++++++++++- > kernel/module.c | 2 +- > kernel/trace/ftrace.c | 6 +- > 17 files changed, 355 insertions(+), 83 deletions(-) > delete mode 100644 arch/parisc/kernel/module.lds >
On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote: > On 10/29/19 10:28 PM, Mark Rutland wrote: > > This series is a reworked version of Torsten's FTRACE_WITH_REGS series > > [1]. I've tried to rework the existing code in preparatory patches so > > that the patchable-function-entry bits slot in with fewer surprises. > > This version is based on v5.4-rc3, and can be found in my > > arm64/ftrace-with-regs branch [2]. > > > > Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses > > to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR > > symbol, and more cleanly separates the one-time initialization of the > > callsite from dynamic NOP<->CALL modification. Architectures which don't > > implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. > > > > Recently parisc gained ftrace support using patchable-function-entry. > > Patch 2 makes the handling of module callsite locations common in > > kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and > > removed the newly redundant bits from arch/parisc. > > > > Patches 3 and 4 move the module PLT initialization to module load time, > > which simplifies runtime callsite modification. This also means that we > > don't transitently mark the module text RW, and will allow for the > > removal of module_disable_ro(). > > > > Patches 5 and 6 add some trivial infrastructure, with patch 7 finally > > adding FTRACE_WITH_REGS support. Additional work will be required for > > livepatching (e.g. implementing reliable stack trace), which is > > commented as part of patch 7. > > > > Patch 8 is a trivial cleanup atop of the rest of the series, making the > > code easier to read and less susceptible to config-specific breakage. > I tested the whole series with my latest in-kernel ptrauth patches [1] > and graph_tracer/function_graph_tracer works fine, So for the whole series, > Tested-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > > Also I gave few minor comments in the individual patches. With those > comments, > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> I don't think this means what you think it means. Please read: Documentation/process/submitting-patches.rst Will
On 11/4/19 6:26 PM, Will Deacon wrote: > On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote: >> On 10/29/19 10:28 PM, Mark Rutland wrote: >>> This series is a reworked version of Torsten's FTRACE_WITH_REGS series >>> [1]. I've tried to rework the existing code in preparatory patches so >>> that the patchable-function-entry bits slot in with fewer surprises. >>> This version is based on v5.4-rc3, and can be found in my >>> arm64/ftrace-with-regs branch [2]. >>> >>> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses >>> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR >>> symbol, and more cleanly separates the one-time initialization of the >>> callsite from dynamic NOP<->CALL modification. Architectures which don't >>> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. >>> >>> Recently parisc gained ftrace support using patchable-function-entry. >>> Patch 2 makes the handling of module callsite locations common in >>> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and >>> removed the newly redundant bits from arch/parisc. >>> >>> Patches 3 and 4 move the module PLT initialization to module load time, >>> which simplifies runtime callsite modification. This also means that we >>> don't transitently mark the module text RW, and will allow for the >>> removal of module_disable_ro(). >>> >>> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally >>> adding FTRACE_WITH_REGS support. Additional work will be required for >>> livepatching (e.g. implementing reliable stack trace), which is >>> commented as part of patch 7. >>> >>> Patch 8 is a trivial cleanup atop of the rest of the series, making the >>> code easier to read and less susceptible to config-specific breakage. >> I tested the whole series with my latest in-kernel ptrauth patches [1] >> and graph_tracer/function_graph_tracer works fine, So for the whole series, >> Tested-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> >> Also I gave few minor comments in the individual patches. With those >> comments, >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> Oops sorry I meant, Reviewed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > > I don't think this means what you think it means. Please read: Thanks for pointing it. > > Documentation/process/submitting-patches.rst > > Will > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, Nov 04, 2019 at 01:03:51PM +0000, Amit Kachhap wrote: > > > On 11/4/19 6:26 PM, Will Deacon wrote: > > On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote: > >> On 10/29/19 10:28 PM, Mark Rutland wrote: > >>> This series is a reworked version of Torsten's FTRACE_WITH_REGS series > >>> [1]. I've tried to rework the existing code in preparatory patches so > >>> that the patchable-function-entry bits slot in with fewer surprises. > >>> This version is based on v5.4-rc3, and can be found in my > >>> arm64/ftrace-with-regs branch [2]. > >>> > >>> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses > >>> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR > >>> symbol, and more cleanly separates the one-time initialization of the > >>> callsite from dynamic NOP<->CALL modification. Architectures which don't > >>> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. > >>> > >>> Recently parisc gained ftrace support using patchable-function-entry. > >>> Patch 2 makes the handling of module callsite locations common in > >>> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and > >>> removed the newly redundant bits from arch/parisc. > >>> > >>> Patches 3 and 4 move the module PLT initialization to module load time, > >>> which simplifies runtime callsite modification. This also means that we > >>> don't transitently mark the module text RW, and will allow for the > >>> removal of module_disable_ro(). > >>> > >>> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally > >>> adding FTRACE_WITH_REGS support. Additional work will be required for > >>> livepatching (e.g. implementing reliable stack trace), which is > >>> commented as part of patch 7. > >>> > >>> Patch 8 is a trivial cleanup atop of the rest of the series, making the > >>> code easier to read and less susceptible to config-specific breakage. > >> I tested the whole series with my latest in-kernel ptrauth patches [1] > >> and graph_tracer/function_graph_tracer works fine, So for the whole series, > >> Tested-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > >> > >> Also I gave few minor comments in the individual patches. With those > >> comments, > >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > Oops sorry I meant, > Reviewed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> Thanks! I've added the Tested-by for the whole series, and the Reviewed-by for patches 4 and 7. I haven't added it for patch 1 just yet; please reply to my comment there if you'd still like to add a Reviewed-by. Mark.
Hi, On 11/4/19 7:34 PM, Mark Rutland wrote: > On Mon, Nov 04, 2019 at 01:03:51PM +0000, Amit Kachhap wrote: >> >> >> On 11/4/19 6:26 PM, Will Deacon wrote: >>> On Sat, Nov 02, 2019 at 05:42:25PM +0530, Amit Daniel Kachhap wrote: >>>> On 10/29/19 10:28 PM, Mark Rutland wrote: >>>>> This series is a reworked version of Torsten's FTRACE_WITH_REGS series >>>>> [1]. I've tried to rework the existing code in preparatory patches so >>>>> that the patchable-function-entry bits slot in with fewer surprises. >>>>> This version is based on v5.4-rc3, and can be found in my >>>>> arm64/ftrace-with-regs branch [2]. >>>>> >>>>> Patch 1 adds an (optional) ftrace_init_nop(), which the core code uses >>>>> to initialize callsites. This allows us to avoid a synthetic MCOUNT_ADDR >>>>> symbol, and more cleanly separates the one-time initialization of the >>>>> callsite from dynamic NOP<->CALL modification. Architectures which don't >>>>> implement this get the existing ftrace_make_nop() with MCOUNT_ADDR. >>>>> >>>>> Recently parisc gained ftrace support using patchable-function-entry. >>>>> Patch 2 makes the handling of module callsite locations common in >>>>> kernel/module.c with a new FTRACE_CALLSITE_SECTION definition, and >>>>> removed the newly redundant bits from arch/parisc. >>>>> >>>>> Patches 3 and 4 move the module PLT initialization to module load time, >>>>> which simplifies runtime callsite modification. This also means that we >>>>> don't transitently mark the module text RW, and will allow for the >>>>> removal of module_disable_ro(). >>>>> >>>>> Patches 5 and 6 add some trivial infrastructure, with patch 7 finally >>>>> adding FTRACE_WITH_REGS support. Additional work will be required for >>>>> livepatching (e.g. implementing reliable stack trace), which is >>>>> commented as part of patch 7. >>>>> >>>>> Patch 8 is a trivial cleanup atop of the rest of the series, making the >>>>> code easier to read and less susceptible to config-specific breakage. >>>> I tested the whole series with my latest in-kernel ptrauth patches [1] >>>> and graph_tracer/function_graph_tracer works fine, So for the whole series, >>>> Tested-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >>>> >>>> Also I gave few minor comments in the individual patches. With those >>>> comments, >>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> Oops sorry I meant, >> Reviewed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > > Thanks! > > I've added the Tested-by for the whole series, and the Reviewed-by for > patches 4 and 7. I haven't added it for patch 1 just yet; please reply > to my comment there if you'd still like to add a Reviewed-by. Those were minor comments. Please go ahead and add the Reviewed-by's. Thanks, Amit > > Mark. >
On Tue, Oct 29, 2019 at 04:58:24PM +0000, Mark Rutland wrote:
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/ftrace-with-regs
I pulled the latest on this branch into the arm64 for-next/core.
Thanks.
--
Catalin