| Message ID | 20220805124522.706457-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: stacktrace: cleanups and improvements | expand |
Thanks for the improvements. For the whole series: Reviewed-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> On 8/5/22 07:45, Mark Rutland wrote: > Hi Marc, > > This series is a set of improvements for the recently merged stacktrace > rework queued in the kvmarm next branch, along the lines of my prior > suggestions. I'm hoping that it would be possible to queue this for > -rc1. > > Largely, the series decouples portions of the unwind code, making some > structural changes that remove the need for coupling (e.g. removing the > need for the stack type enumeration, and factoring out callbacks). This > should make it easier to make some further changes I have spoken about > previously (e.g. adding some metadata to the stack dump output), and I > have some patches building atop this which I intend to send out once > this is merged. > > The only (intended) functional change from this series is improved > boundary detection, where overlapping frame records will now be caught > and rejected. > > I've tested this natively (with the ftrace tests, LKDTM, and > /proc/self/stack), which seems happy. > > I've also tested the NVHE and PKVM unwinders by hacking a BUG() into the > hyp code shortly after the KVM hyp code is initialized, and in both > cases the output is unchanged. > > Since v1 [1]: > * Fix build warning with CONFIG_SDEI_INTERFACE=y && W=1 > * Fix typos > * Fix whitespace errors > * Apply tags from Kalesh Singh and Mark Brown > > [1] https://lore.kernel.org/r/20220801121209.2479449-1-mark.rutland@arm.com > > Thanks, > Mark. > > Mark Rutland (8): > arm64: stacktrace: simplify unwind_next_common() > arm64: stacktrace: rename unwind_next_common() -> > unwind_next_frame_record() > arm64: stacktrace: move SDEI stack helpers to stacktrace code > arm64: stacktrace: add stackinfo_on_stack() helper > arm64: stacktrace: rework stack boundary discovery > arm64: stacktrace: remove stack type from fp translator > arm64: stacktrace: track all stack boundaries explicitly > arm64: stacktrace: track hyp stacks in unwinder's address space > > arch/arm64/include/asm/processor.h | 2 +- > arch/arm64/include/asm/sdei.h | 17 -- > arch/arm64/include/asm/stacktrace.h | 71 +++++-- > arch/arm64/include/asm/stacktrace/common.h | 211 +++++++++------------ > arch/arm64/kernel/ptrace.c | 2 +- > arch/arm64/kernel/sdei.c | 32 ---- > arch/arm64/kernel/stacktrace.c | 66 ++++--- > arch/arm64/kvm/hyp/nvhe/stacktrace.c | 40 ++-- > arch/arm64/kvm/stacktrace.c | 135 +++++++------ > 9 files changed, 288 insertions(+), 288 deletions(-) >
On Fri, Aug 05, 2022 at 01:45:14PM +0100, Mark Rutland wrote: > This series is a set of improvements for the recently merged stacktrace > rework queued in the kvmarm next branch, along the lines of my prior > suggestions. I'm hoping that it would be possible to queue this for > -rc1. What justifies this for inclusion in -rc1? Although the series looks pretty good to me, it seems to be cleanup and refactoring which could wait for 5.21 (or whatever it ends up being called). What am I missing? Cheers, Will
On Mon, Aug 08, 2022 at 12:59:08PM +0100, Will Deacon wrote: > On Fri, Aug 05, 2022 at 01:45:14PM +0100, Mark Rutland wrote: > > This series is a set of improvements for the recently merged stacktrace > > rework queued in the kvmarm next branch, along the lines of my prior > > suggestions. I'm hoping that it would be possible to queue this for > > -rc1. > > What justifies this for inclusion in -rc1? Although the series looks pretty > good to me, it seems to be cleanup and refactoring which could wait for > 5.21 (or whatever it ends up being called). What am I missing? If you think this should wait, that's fine by me. I plan to build atop this with further rework and just wanted to get the trivial bits out of the way early. Thanks, Mark.
On Mon, Aug 08, 2022 at 01:11:15PM +0100, Mark Rutland wrote: > On Mon, Aug 08, 2022 at 12:59:08PM +0100, Will Deacon wrote: > > On Fri, Aug 05, 2022 at 01:45:14PM +0100, Mark Rutland wrote: > > > This series is a set of improvements for the recently merged stacktrace > > > rework queued in the kvmarm next branch, along the lines of my prior > > > suggestions. I'm hoping that it would be possible to queue this for > > > -rc1. > > > > What justifies this for inclusion in -rc1? Although the series looks pretty > > good to me, it seems to be cleanup and refactoring which could wait for > > 5.21 (or whatever it ends up being called). What am I missing? > > If you think this should wait, that's fine by me. I plan to build atop this > with further rework and just wanted to get the trivial bits out of the way > early. Although this stuff is largely cosmetic, I wouldn't say it's trivial! So yes, how about we stick this on a branch at -rc3 and you can build on top of that? It also means you can spin a v3 with the various minor open issues addressed. Will
On Mon, Aug 08, 2022 at 01:28:32PM +0100, Will Deacon wrote: > On Mon, Aug 08, 2022 at 01:11:15PM +0100, Mark Rutland wrote: > > On Mon, Aug 08, 2022 at 12:59:08PM +0100, Will Deacon wrote: > > > On Fri, Aug 05, 2022 at 01:45:14PM +0100, Mark Rutland wrote: > > > > This series is a set of improvements for the recently merged stacktrace > > > > rework queued in the kvmarm next branch, along the lines of my prior > > > > suggestions. I'm hoping that it would be possible to queue this for > > > > -rc1. > > > > > > What justifies this for inclusion in -rc1? Although the series looks pretty > > > good to me, it seems to be cleanup and refactoring which could wait for > > > 5.21 (or whatever it ends up being called). What am I missing? > > > > If you think this should wait, that's fine by me. I plan to build atop this > > with further rework and just wanted to get the trivial bits out of the way > > early. > > Although this stuff is largely cosmetic, I wouldn't say it's trivial! So > yes, how about we stick this on a branch at -rc3 and you can build on top > of that? > > It also means you can spin a v3 with the various minor open issues > addressed. That sounds great to me, thanks! I'll wait a few days before sending out a v3 in case there are any other impactful comments. Thanks, Mark.
On Mon, Aug 08, 2022 at 01:11:15PM +0100, Mark Rutland wrote: > On Mon, Aug 08, 2022 at 12:59:08PM +0100, Will Deacon wrote: > > On Fri, Aug 05, 2022 at 01:45:14PM +0100, Mark Rutland wrote: > > > suggestions. I'm hoping that it would be possible to queue this for > > > -rc1. > > What justifies this for inclusion in -rc1? Although the series looks pretty > > good to me, it seems to be cleanup and refactoring which could wait for > > 5.21 (or whatever it ends up being called). What am I missing? > If you think this should wait, that's fine by me. I plan to build atop this > with further rework and just wanted to get the trivial bits out of the way > early. Perhaps more applied at -rc1 rather than applied for -rc1? Though with Catalin being on holiday I'm guessing that actually happening is unlikely.
Hi Marc, This series is a set of improvements for the recently merged stacktrace rework queued in the kvmarm next branch, along the lines of my prior suggestions. I'm hoping that it would be possible to queue this for -rc1. Largely, the series decouples portions of the unwind code, making some structural changes that remove the need for coupling (e.g. removing the need for the stack type enumeration, and factoring out callbacks). This should make it easier to make some further changes I have spoken about previously (e.g. adding some metadata to the stack dump output), and I have some patches building atop this which I intend to send out once this is merged. The only (intended) functional change from this series is improved boundary detection, where overlapping frame records will now be caught and rejected. I've tested this natively (with the ftrace tests, LKDTM, and /proc/self/stack), which seems happy. I've also tested the NVHE and PKVM unwinders by hacking a BUG() into the hyp code shortly after the KVM hyp code is initialized, and in both cases the output is unchanged. Since v1 [1]: * Fix build warning with CONFIG_SDEI_INTERFACE=y && W=1 * Fix typos * Fix whitespace errors * Apply tags from Kalesh Singh and Mark Brown [1] https://lore.kernel.org/r/20220801121209.2479449-1-mark.rutland@arm.com Thanks, Mark. Mark Rutland (8): arm64: stacktrace: simplify unwind_next_common() arm64: stacktrace: rename unwind_next_common() -> unwind_next_frame_record() arm64: stacktrace: move SDEI stack helpers to stacktrace code arm64: stacktrace: add stackinfo_on_stack() helper arm64: stacktrace: rework stack boundary discovery arm64: stacktrace: remove stack type from fp translator arm64: stacktrace: track all stack boundaries explicitly arm64: stacktrace: track hyp stacks in unwinder's address space arch/arm64/include/asm/processor.h | 2 +- arch/arm64/include/asm/sdei.h | 17 -- arch/arm64/include/asm/stacktrace.h | 71 +++++-- arch/arm64/include/asm/stacktrace/common.h | 211 +++++++++------------ arch/arm64/kernel/ptrace.c | 2 +- arch/arm64/kernel/sdei.c | 32 ---- arch/arm64/kernel/stacktrace.c | 66 ++++--- arch/arm64/kvm/hyp/nvhe/stacktrace.c | 40 ++-- arch/arm64/kvm/stacktrace.c | 135 +++++++------ 9 files changed, 288 insertions(+), 288 deletions(-)