Re: [Crash-utility] [PATCH v5 0/4] arm64: more improvement of bt -f

----- Original Message ----- Takahiro, Sorry for the previous empty response -- I sent it by mistake while putting this 3-message collaborative response together. Well, that's certainly unfortunate. Things like: (1) I want to know whether the active cpu was properly shut down by the kdump mechanism. (2) I would like to actually "test" that the crash utility's transition from the IRQ stack back to the process stack works as expected. (especially given the relative rarity of kernel crashes that actually occur while operating on IRQ stacks) (3) There are several other dumping mechanisms that can be utilized on ARM64 systems, so again, I'd like it to be obvious that this is kdump. (4) When doing a filtered "foreach bt" it would be helpful to simply make it obvious that a task was an active, non-crashing, task -- with more information than a "hanging" exception frame dump. (5) All of the other architectures do it. ... [ cut ] ... OK, we're just going to have to agree to disagree. Maybe so, but I absolutely do think so. and is essentially a backport of the kernel's dump_backtrace() code. Right, but the current loop remains basically the same as the kernel's. I'm not following when you say it "will miss" a frame. It's just following the links established by the architecture. If you line up the backtrace output of the current vs. your patch, there are no missing frames, but rather you essentially move the "el1_da()" stack frame entry to a lower stack address, and your "bt -f" implementation doesn't dump the raw exception frame data. I understand what you're saying, and certainly appreciate the depth of knowledge that you have, but quite frankly I don't find the changes significant enough to reinvent the wheel. ... [ cut ] ... Well, yes, that's true in the case of an exception frame generated by the IPI interrupt (or any interrupt), where the eframe contents are incidental to the interrupt's arrival, and not nearly as interesting as an in-line kernel exception frame generated by a NULL pointer reference, BUG(), etc., where the contents of the exception frame are completely related to the problem/bug at hand. And in that case, the current code does dump the raw contents just before the register translation. It's not a major item, but it certainly just dumps the raw memory in between frame like anywhere else in the "bt -f" output. Seems strange to just omit it there. I'm not following -- your patch (as well as the current version) both show ffff800020b0bb80-ffff800020b0bc10 below do_mem_abort(): ... #5 [ffff800020b0bb70] do_mem_abort at ffff000008081288 ffff800020b0bb70: ffff800020b0bd40 el1_da+24 ffff800020b0bb80: cpu_cgrp_subsys+152 0000000000000063 ffff800020b0bb90: ffff800020b0bd40 sysrq_handle_crash+32 ffff800020b0bba0: 0000000000000002 textbuf.34610 ffff800020b0bbb0: ffff800020b0bbd0 kallsyms_token_index+43128 ffff800020b0bbc0: 000000000000000f 0000000100000000 ffff800020b0bbd0: ffff800020b0bc70 vprintk_default+56 ffff800020b0bbe0: cpu_cgrp_subsys+152 0000000000000063 ffff800020b0bbf0: sysrq_crash_op 0000000000000009 ffff800020b0bc00: 0000000000000000 0000000000000015 ffff800020b0bc10: 0000000000000120 0000000000000040 #6 [ffff800020b0bc20] el1_da at ffff000008084564 --- <Exception in kernel> --- PC: ffff000008457fc8 [sysrq_handle_crash+32] LR: ffff000008457fb4 [sysrq_handle_crash+12] SP: ffff800020b0bd40 PSTATE: 60400149 ... The current code does the above, but dumps the raw exception frame before printing a stack entry for el1_da(): ... #5 [ffff800020b0bb70] do_mem_abort at ffff00000808128c ffff800020b0bb70: ffff800020b0bd40 el1_da+24 ffff800020b0bb80: cpu_cgrp_subsys+152 0000000000000063 ffff800020b0bb90: ffff800020b0bd40 sysrq_handle_crash+32 ffff800020b0bba0: 0000000000000002 textbuf.34610 ffff800020b0bbb0: ffff800020b0bbd0 kallsyms_token_index+43128 ffff800020b0bbc0: 000000000000000f 0000000100000000 ffff800020b0bbd0: ffff800020b0bc70 vprintk_default+56 ffff800020b0bbe0: cpu_cgrp_subsys+152 0000000000000063 ffff800020b0bbf0: sysrq_crash_op 0000000000000009 ffff800020b0bc00: 0000000000000000 0000000000000015 ffff800020b0bc10: 0000000000000120 0000000000000040 ffff800020b0bc20: 0000000000000001 0000000000000000 ffff800020b0bc30: log_wait+8 0000000000000000 ffff800020b0bc40: 0000000000000000 00000000000047d4 ffff800020b0bc50: ffff800022f337a4 0000000000000000 ffff800020b0bc60: 0000000000000106 0000000000000001 ffff800020b0bc70: 0000000000000002 0000000000000106 ffff800020b0bc80: log_buf_len cont ffff800020b0bc90: 0000ffff83cc28f0 text.34829+13 ffff800020b0bca0: sys_write 0000ffff83d266c0 ffff800020b0bcb0: 0000000000000006 cpu_cgrp_subsys+152 ffff800020b0bcc0: 0000000000000063 sysrq_crash_op ffff800020b0bcd0: 0000000000000009 0000000000000000 ffff800020b0bce0: 0000000000000015 0000000000000120 ffff800020b0bcf0: 0000000000000040 sys_call_table ffff800020b0bd00: ffff800020b08000 ffff800020b0bd40 ffff800020b0bd10: sysrq_handle_crash+12 ffff800020b0bd40 ffff800020b0bd20: sysrq_handle_crash+32 0000000060400149 ffff800020b0bd30: cpu_cgrp_subsys+152 [kmalloc-1024] #6 [ffff800020b0bd40] el1_da at ffff000008084568 PC: ffff000008457fc8 [sysrq_handle_crash+32] LR: ffff000008457fb4 [sysrq_handle_crash+12] SP: ffff800020b0bd40 PSTATE: 60400149 ... Yes, again I understand that you feel that the el1_da() frame should be at ffff800020b0bc20, but on the other hand, the link register/pc under do_mem_abort() points to ffff800020b0bd40, and I kind of prefer to show that. I *know* you disagree. ... [ cut ] ... I don't see where "bt -F" gets involved in arm64_print_exception_frame()? Are you referring to this, which I agree is OK: @@ -2130,10 +2343,11 @@ arm64_print_exception_frame(struct bt_info *bt, ulong pt_regs, int mode, FILE *o } if (is_64_bit) { - fprintf(ofp, "ORIG_X0: %016lx SYSCALLNO: %lx", - (ulong)regs->orig_x0, (ulong)regs->syscallno); - if (mode == USER_MODE) + if (mode == USER_MODE) { + fprintf(ofp, "ORIG_X0: %016lx SYSCALLNO: %lx", + (ulong)regs->orig_x0, (ulong)regs->syscallno); fprintf(ofp, " PSTATE: %08lx", (ulong)regs->pstate); + } fprintf(ofp, "\n"); } Anyway, I'm going to be away for the next few days, and will revisit this all again next week. But here's a thought: There already are "bt -o" and "bt -O" for x86 and x86_64, which allow for the existence of coexisting but separate back trace methods. I'm thinking that we can keep the current code, while also introducing your patchset as an "optional" back trace method. There would be arm64_back_trace_cmd_v1() and arm64_back_trace_cmd_v2(), arm64_unwind_frame_v1() and arm64_unwind_frame_v2(), and any other functions that can't comfortably be utilized by both methods. And then "bt -o" would use the optional version, or the default method selection could be toggled with "bt -O". And again, I hope we don't lose you. During the KASLR negotiations, you've already seen what an obstinate ass I can be. But it's really part of my job as a gatekeeper. Thanks, Dave "Simplicity, simplicity, simplicity." - Henry David Thoreau