0 0 0 ffffffff819f9480 RU 0.0 0 0 [swapper/0] 0 0 1 ffff880169411fa0 RU 0.0 0 0 [swapper/1]

Hi Tao, On Thu, Mar 07, 2024 at 08:52:40PM +0800, Tao Liu wrote: > Previously we can only view the stack unwinding for the tasks which are > running on each CPUs. This patch will enable the ability to view > arbitrary tasks stack unwinding. > > After crash get initialized, "info threads" will output like the > following: > > crash> info threads > Id Target Id Frame > 1 CPU 0 native_safe_halt () at arch/x86/include/asm/irqflags.h:54 > ... > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, reserved=reserved@entry=false) at block/blk-mq.c:640 > ... > 13 CPU 12 <unavailable> in ?? () > 14 CPU 13 native_safe_halt () at arch/x86/include/asm/irqflags.h:54 > ... > > crash> ps > PID PPID CPU TASK ST %MEM VSZ RSS COMM > > 0 0 0 ffffffff819f9480 RU 0.0 0 0 [swapper/0] > > 0 0 1 ffff880169411fa0 RU 0.0 0 0 [swapper/1] > ... > 0 0 23 ffff8801694e0000 RU 0.0 0 0 [swapper/23] > 1 0 13 ffff880169b30000 IN 0.0 193052 4180 systemd > > "info threads" show the tasks which are currently running on each CPU. If we'd > like to view systemd task's stack unwinding, which is inactive status, we > do the following: > > crash> set 1 > or > crash> set ffff880169b30000 > > Then the register cache of systemd will be swapped into CPU 13: > > crash> info threads > Id Target Id Frame > 1 CPU 0 native_safe_halt () at arch/x86/include/asm/irqflags.h:54 > ... > 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, reserved=reserved@entry=false) at block/blk-mq.c:640 > ... > 13 CPU 12 <unavailable> in ?? () > * 14 CPU 13 0xffffffff816a8f65 in context_switch (rq=0x0, next=0x0, prev=0xffff880169b30000) at kernel/sched/core.c:2527 > ... > > And we can view the stack unwinding of systemd: > > crash> bt > PID: 1 TASK: ffff880169b30000 CPU: 13 COMMAND: "systemd" > #0 [ffff880169b3bd58] __schedule at ffffffff816a8f65 > #1 [ffff880169b3bdc0] schedule at ffffffff816a94e9 > #2 [ffff880169b3bdd0] schedule_hrtimeout_range_clock at ffffffff816a86fd > #3 [ffff880169b3be68] schedule_hrtimeout_range at ffffffff816a8733 > #4 [ffff880169b3be78] ep_poll at ffffffff8124bb7e > #5 [ffff880169b3bf30] sys_epoll_wait at ffffffff8124d00d > #6 [ffff880169b3bf80] system_call_fastpath at ffffffff816b5009 > RIP: 00007f0449407923 RSP: 00007ffc35a3c378 RFLAGS: 00010246 > RAX: 00000000000000e8 RBX: ffffffff816b5009 RCX: 0000000000000071 > RDX: 000000000000001d RSI: 00007ffc35a3d5a0 RDI: 0000000000000004 > RBP: 00007ffc35a3d810 R8: 0000000000000000 R9: 0000000000000000 > R10: 00000000ffffffff R11: 0000000000000293 R12: 0000563ca2ebe980 > R13: 0000000000000003 R14: ffffffffffffffff R15: 0000000000000001 > ORIG_RAX: 00000000000000e8 CS: 0033 SS: 002b > crash> gdb bt > #0 0xffffffff816a8f65 in context_switch (rq=0x0, next=0x0, prev=0xffff880169b30000) at kernel/sched/core.c:2527 > #1 __schedule () at kernel/sched/core.c:3540 > #2 0xffffffff816a94e9 in schedule () at kernel/sched/core.c:3577 > #3 0xffffffff816a86fd in schedule_hrtimeout_range_clock (expires=expires@entry=0x0, delta=delta@entry=0, mode=mode@entry=HRTIMER_MODE_ABS, clock=clock@entry=1) at kernel/hrtimer.c:1724 > #4 0xffffffff816a8733 in schedule_hrtimeout_range (expires=expires@entry=0x0, delta=delta@entry=0, mode=mode@entry=HRTIMER_MODE_ABS) at kernel/hrtimer.c:1778 > #5 0xffffffff8124bb7e in ep_poll (ep=0xffff880fd861f8c0, events=events@entry=0x7ffc35a3d5a0, maxevents=maxevents@entry=29, timeout=timeout@entry=-1) at fs/eventpoll.c:1669 > #6 0xffffffff8124d00d in SYSC_epoll_wait (timeout=<optimized out>, maxevents=29, events=<optimized out>, epfd=<optimized out>) at fs/eventpoll.c:2043 > #7 SyS_epoll_wait (epfd=<optimized out>, events=140721208415648, maxevents=29, timeout=4294967295) at fs/eventpoll.c:2008 > #8 <signal handler called> > #9 0x00007f0449407923 in ?? () > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > --- > crash_target.c | 50 ++++++++++++++++------------- > defs.h | 3 +- > gdb-10.2.patch | 85 +------------------------------------------------- > kernel.c | 22 +++++++++++++ > task.c | 5 +-- > 5 files changed, 57 insertions(+), 108 deletions(-) > > diff --git a/crash_target.c b/crash_target.c > index d06383f..8efe2d6 100644 > --- a/crash_target.c > +++ b/crash_target.c > @@ -29,10 +29,10 @@ extern "C" int gdb_readmem_callback(unsigned long, void *, int, int); > extern "C" int crash_get_nr_cpus(void); > extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname, > int regsize, void *val); > -extern "C" int gdb_change_cpu_context (unsigned int cpu); > extern "C" void gdb_refresh_regcache(unsigned int cpu); > extern "C" int set_cpu(int cpu, int print_context); > - > +extern "C" int crash_set_thread(ulong); > +extern "C" int gdb_change_thread_context (ulong); > > /* The crash target. */ > > @@ -110,11 +110,13 @@ crash_target::xfer_partial (enum target_object object, const char *annex, > > #define CRASH_INFERIOR_PID 1 > > +crash_target *target = NULL; > + > void > crash_target_init (void) > { > int nr_cpus = crash_get_nr_cpus(); > - crash_target *target = new crash_target (); > + target = new crash_target (); > > /* Own the target until it is successfully pushed. */ > target_ops_up target_holder (target); > @@ -137,29 +139,35 @@ crash_target_init (void) > reinit_frame_cache (); > } > > -/* > - * Change gdb's thread context to the thread on given CPU > - **/ > extern "C" int > -gdb_change_cpu_context(unsigned int cpu) > +gdb_change_thread_context (ulong task) > { > - ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > - inferior *inf = current_inferior (); > - thread_info *tp = find_thread_ptid (inf, ptid); > - > - if (tp == nullptr) > + int tried = 0; > + inferior* inf = current_inferior(); > + int cpu = crash_set_thread(task); > + if (cpu < 0) > return FALSE; > > - /* Making sure that crash's context is same */ > - set_cpu(cpu, FALSE); > - > - /* Switch to the thread */ > - switch_to_thread(tp); > - > - /* Fetch/Refresh thread's registers */ > - gdb_refresh_regcache(cpu); > + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > - return TRUE; > +retry: > + thread_info *tp = find_thread_ptid (inf, ptid); > + if (tp == nullptr && !tried) { > + thread_info *thread = add_thread_silent(target, > + ptid_t(CRASH_INFERIOR_PID, 0, cpu)); A minor comment. Is it possible to do it without needing 'target' as global ? I see there's 'inf->current_top_target()', but it's return type if target_ops, don't know if it can be used.

> + tried++; > + if (thread) { > + goto retry; > + } > + } > + > + if (tp == nullptr && tried) > + return FALSE; > + > + target_fetch_registers(get_thread_regcache(tp), -1); > + switch_to_thread(tp); > + reinit_frame_cache (); > + return TRUE; > } > > /* Refresh regcache of gdb thread on given CPU > diff --git a/defs.h b/defs.h > index 06bd27d..1ddfc5e 100644 > --- a/defs.h > +++ b/defs.h > @@ -6083,6 +6083,7 @@ void sort_tgid_array(void); > int sort_by_tgid(const void *, const void *); > int in_irq_ctx(ulonglong, int, ulong); > void check_stack_overflow(void); > +int gdb_change_thread_context (ulong); > > /* > * extensions.c > @@ -6123,6 +6124,7 @@ void dump_kernel_table(int); > void dump_bt_info(struct bt_info *, char *where); > void dump_log(int); > void parse_kernel_version(char *); > +int crash_set_thread(ulong); > > #define LOG_LEVEL(v) ((v) & 0x07) > #define SHOW_LOG_LEVEL (0x1) > @@ -8191,7 +8193,6 @@ enum ppc64_regnum { > }; > > /* crash_target.c */ > -extern int gdb_change_cpu_context (unsigned int cpu); > extern void gdb_refresh_regcache (unsigned int cpu); > > #endif /* !GDB_COMMON */ > diff --git a/gdb-10.2.patch b/gdb-10.2.patch > index 138413f..3e6677d 100644 > --- a/gdb-10.2.patch > +++ b/gdb-10.2.patch > @@ -16058,35 +16058,6 @@ exit 0 > m10200-dis.c > m10200-opc.c > m10300-dis.c > ---- gdb-10.2/gdb/thread.c.orig > -+++ gdb-10.2/gdb/thread.c > -@@ -58,6 +58,11 @@ static int highest_thread_num; > - /* The current/selected thread. */ > - static thread_info *current_thread_; > - > -+#ifdef CRASH_MERGE > -+/* Function to set cpu, defined by crash-utility */ > -+extern "C" void set_cpu (int); > -+#endif > -+ > - /* RAII type used to increase / decrease the refcount of each thread > - in a given list of threads. */ > - > -@@ -1896,7 +1901,13 @@ thread_command (const char *tidstr, int from_tty) > - { > - ptid_t previous_ptid = inferior_ptid; > - > -- thread_select (tidstr, parse_thread_id (tidstr, NULL)); > -+ struct thread_info* thread_id = parse_thread_id (tidstr, NULL); > -+ > -+#ifdef CRASH_MERGE > -+ set_cpu(thread_id->ptid.tid()); > -+#endif > -+ > -+ thread_select (tidstr, thread_id); > - > - /* Print if the thread has not changed, otherwise an event will > - be sent. */ > --- gdb-10.2/gdb/ui-file.h.orig > +++ gdb-10.2/gdb/ui-file.h > @@ -195,6 +195,7 @@ public: > @@ -16139,58 +16110,4 @@ exit 0 > + (dynamic_cast<stdio_file *>gdb_stderr)->set_stream(original_stderr_stream); > } > > - /* > ---- gdb-10.2/gdb/thread.c.orig > -+++ gdb-10.2/gdb/thread.c > -@@ -60,7 +60,7 @@ static thread_info *current_thread_; > - > - #ifdef CRASH_MERGE > - /* Function to set cpu, defined by crash-utility */ > --extern "C" void set_cpu (int); > -+extern "C" void set_cpu(int cpu, int print_context); > - #endif > - > - /* RAII type used to increase / decrease the refcount of each thread > -@@ -1098,6 +1098,9 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, > - uiout->table_body (); > - } > - > -+#ifdef CRASH_MERGE > -+ int current_cpu = current_thread ? current_thread->ptid.tid() : 0; > -+#endif > - for (inferior *inf : all_inferiors ()) > - for (thread_info *tp : inf->threads ()) > - { > -@@ -1113,6 +1116,11 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, > - > - ui_out_emit_tuple tuple_emitter (uiout, NULL); > - > -+ /* Switch to the thread's CPU in crash */ > -+#ifdef CRASH_MERGE > -+ set_cpu(tp->ptid.tid(), FALSE); > -+#endif > -+ > - if (!uiout->is_mi_like_p ()) > - { > - if (tp == current_thread) > -@@ -1185,6 +1193,11 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, > - /* This end scope restores the current thread and the frame > - selected before the "info threads" command, and it finishes the > - ui-out list or table. */ > -+ > -+#ifdef CRASH_MERGE > -+ /* Restore crash's context */ > -+ set_cpu(current_cpu, FALSE); > -+#endif > - } > - > - if (pid == -1 && requested_threads == NULL) > -@@ -1904,7 +1917,7 @@ thread_command (const char *tidstr, int from_tty) > - struct thread_info* thread_id = parse_thread_id (tidstr, NULL); > - > - #ifdef CRASH_MERGE > -- set_cpu(thread_id->ptid.tid()); > -+ set_cpu(thread_id->ptid.tid(), FALSE); > - #endif > - > - thread_select (tidstr, thread_id); > + /* > \ No newline at end of file > diff --git a/kernel.c b/kernel.c > index c7de73a..49d422b 100644 > --- a/kernel.c > +++ b/kernel.c > @@ -6543,6 +6543,28 @@ set_cpu(int cpu, int print_context) > show_context(CURRENT_CONTEXT()); > } > > +int > +crash_set_thread(ulong task) > +{ > + bool found = FALSE; > + struct task_context *tc = FIRST_CONTEXT(); > + > + for (int i = 0; i < RUNNING_TASKS(); i++, tc++) { > + if (tc->task == task) { > + found = TRUE; > + break; > + } > + } > + > + if (!found) > + return -1; > + > + if (CURRENT_TASK() == tc->task) > + return tc->processor; > + > + set_context(tc->task, NO_PID, TRUE); > + return tc->processor; > +} > > /* > * Collect the irq_desc[] entry along with its associated handler and > diff --git a/task.c b/task.c > index a405b05..ef79f53 100644 > --- a/task.c > +++ b/task.c > @@ -715,7 +715,8 @@ task_init(void) > * crash_target::fetch_registers, so CPU 0's registers are shown as > * <unavailable> in gdb mode > * */ > - gdb_refresh_regcache(0); > + for (int i = 0; i < get_cpus_online(); i++) > + gdb_refresh_regcache(i); Does x86 require all regcache(s) to be refreshed at init time ? Till v4 of my ppc series, I also was doing refresh of all CPUs, but it's better to do it when required, and there was a review in v4, patch 5 from Lianbo, quoting it here: > In addition, I haven't tested how far it takes the time in the > multi-core environment, is it possible to implement a smaller > granularity refresh operation? For example: When we need switch to > another cpu, execute the refresh operation for corresponding regcache. CPU 0's refresh is required due to gdb having already tried to initialised registers before we initialised crash_target This might add a small noticeable delay on vmcores from big systems. Adds around 4.6 seconds for a ppc64 vmcore with 64 CPUs, in crash init time, while reading it on a fast 48 cpu Power8 system. This overhead is less for systems with less CPUs, eg. ~0.43 seconds, on a vmcore of 8 CPUs x86_64 system.

Thanks, Aditya Gupta > > tt->flags |= TASK_INIT_DONE; > } > @@ -5315,7 +5316,7 @@ set_context(ulong task, ulong pid, uint update_gdb_thread) > > /* change the selected thread in gdb, according to current context */ > if (update_gdb_thread) > - return gdb_change_cpu_context(tc->processor); > + return gdb_change_thread_context(tc->task); > else > return TRUE; > } else { > -- > 2.40.1 >

Hi Tao, On Wed, Mar 13, 2024 at 11:23:56AM +0800, Tao Liu wrote: > Hi Aditya, > > On Tue, Mar 12, 2024 at 5:12 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > > > > Hi Tao, > > > > > <...> > > > > > > +crash_target *target = NULL; > > > + > > > void > > > crash_target_init (void) > > > { > > > int nr_cpus = crash_get_nr_cpus(); > > > - crash_target *target = new crash_target (); > > > + target = new crash_target (); > > > > > > /* Own the target until it is successfully pushed. */ > > > target_ops_up target_holder (target); > > > @@ -137,29 +139,35 @@ crash_target_init (void) > > > reinit_frame_cache (); > > > } > > > > > > -/* > > > - * Change gdb's thread context to the thread on given CPU > > > - **/ > > > extern "C" int > > > -gdb_change_cpu_context(unsigned int cpu) > > > +gdb_change_thread_context (ulong task) > > > { > > > - ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > > - inferior *inf = current_inferior (); > > > - thread_info *tp = find_thread_ptid (inf, ptid); > > > - > > > - if (tp == nullptr) > > > + int tried = 0; > > > + inferior* inf = current_inferior(); > > > + int cpu = crash_set_thread(task); > > > + if (cpu < 0) > > > return FALSE; > > > > > > - /* Making sure that crash's context is same */ > > > - set_cpu(cpu, FALSE); > > > - > > > - /* Switch to the thread */ > > > - switch_to_thread(tp); > > > - > > > - /* Fetch/Refresh thread's registers */ > > > - gdb_refresh_regcache(cpu); > > > + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > > > > > - return TRUE; > > > +retry: > > > + thread_info *tp = find_thread_ptid (inf, ptid); > > > + if (tp == nullptr && !tried) { > > > + thread_info *thread = add_thread_silent(target, > > > + ptid_t(CRASH_INFERIOR_PID, 0, cpu)); > > > > A minor comment. > > Is it possible to do it without needing 'target' as global ? I see > > there's 'inf->current_top_target()', but it's return type if target_ops, > > don't know if it can be used. > > > Thanks for your comments, however I guess it is not workable for the > code change. What we want is to add a thread to the target. I found > one function "target.c:new_thread", which can be used as > "new_thread(current_inferior(), ptid_t(CRASH_INFERIOR_PID, 0, cpu));" > to replace the "add_thread_silent()" code, except we need to make > "new_thread" to be non-static. > > I prefer to keep the code as current, because I don't see the side > effect of making target as global variable. What do you think? Then there doesn't seem to be an alternative, then I guess we will have to keep the global variable, seems okay to me. > > > > <...> > > > > > > /* > > > * Collect the irq_desc[] entry along with its associated handler and > > > diff --git a/task.c b/task.c > > > index a405b05..ef79f53 100644 > > > --- a/task.c > > > +++ b/task.c > > > @@ -715,7 +715,8 @@ task_init(void) > > > * crash_target::fetch_registers, so CPU 0's registers are shown as > > > * <unavailable> in gdb mode > > > * */ > > > - gdb_refresh_regcache(0); > > > + for (int i = 0; i < get_cpus_online(); i++) > > > + gdb_refresh_regcache(i); > > > > Does x86 require all regcache(s) to be refreshed at init time ? > > > > Till v4 of my ppc series, I also was doing refresh of all CPUs, but it's > > better to do it when required, and there was a review in v4, patch 5 > > from Lianbo, quoting it here: > > > > > In addition, I haven't tested how far it takes the time in the > > > multi-core environment, is it possible to implement a smaller > > > granularity refresh operation? For example: When we need switch to > > > another cpu, execute the refresh operation for corresponding regcache. > > > > CPU 0's refresh is required due to gdb having already tried to > > initialised registers before we initialised crash_target > > > > This might add a small noticeable delay on vmcores from big systems. > > > > Adds around 4.6 seconds for a ppc64 vmcore with 64 CPUs, in crash init > > time, while reading it on a fast 48 cpu Power8 system. > > This overhead is less for systems with less CPUs, eg. ~0.43 seconds, on > > a vmcore of 8 CPUs x86_64 system. > > Yes, thanks for the testing results. There do have performance > downgrades of refreshing all cpu caches. But this will give correct > results when user type "info threads" after showing "crash>": > > With all cpu cache refreshed: > crash> info threads > Id Target Id Frame > 1 CPU 0 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 2 CPU 1 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 3 CPU 2 <unavailable> in ?? () > 4 CPU 3 <unavailable> in ?? () > 5 CPU 4 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 6 CPU 5 <unavailable> in ?? () > 7 CPU 6 <unavailable> in ?? () > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 9 CPU 8 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 10 CPU 9 <unavailable> in ?? () > 11 CPU 10 <unavailable> in ?? () > 12 CPU 11 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 13 CPU 12 <unavailable> in ?? () > 14 CPU 13 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 15 CPU 14 <unavailable> in ?? () > 16 CPU 15 <unavailable> in ?? () > 17 CPU 16 <unavailable> in ?? () > 18 CPU 17 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 19 CPU 18 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 20 CPU 19 <unavailable> in ?? () > 21 CPU 20 <unavailable> in ?? () > 22 CPU 21 <unavailable> in ?? () > 23 CPU 22 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 24 CPU 23 <unavailable> in ?? () > crash> q > > With only cpu0 refreshed: > crash> info threads > Id Target Id Frame > 1 CPU 0 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 2 CPU 1 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 3 CPU 2 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 4 CPU 3 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 5 CPU 4 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 6 CPU 5 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 7 CPU 6 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 9 CPU 8 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 10 CPU 9 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 11 CPU 10 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 12 CPU 11 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 13 CPU 12 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 14 CPU 13 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 15 CPU 14 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 16 CPU 15 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 17 CPU 16 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 18 CPU 17 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 19 CPU 18 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 20 CPU 19 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 21 CPU 20 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 22 CPU 21 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 23 CPU 22 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 24 CPU 23 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > > As we can see, other cpus[1-6, 8-23] just take the reg cache of > cpu[7], which is incorrect. And if users go further like, "thread 20" > and "gdb bt", it will also give incorrect stack traces. > > The cpu cache will only get refreshed once user type "set <pid>", so > the cpu cache will be refreshed by the <pid> task's context. > > I doubt a user will understand all the details and constraints, so I'm > afraid the user will be confused by the faulty output. But I also have > no objection if the performance is the priority. Basically it is a > balance of pays and gains. In addition, since cmd "info" and "thread" > is a command provided by gdb, currently I don't know how to hack > those, so cpu cache can be refreshed when "info threads" or "thread > <num>" have been invoked. > > Do you have any thoughts? I also had faced that issue initially, ie. the other CPUs using up same regcache, if all are not refreshed. While iterating through all threads, gdb switches it's context temporarily, while crash's context remained same, thus causing gdb to get same registers for all threads other than 0. This was solved in patch #3 (synchronise cpu context changes between crash/gdb) in the ppc's 'Improve stack unwind on ppc64' series, by syncing gdb's context with crash. Can this change in thread.c in gdb-10.2.patch in patch #2 be reverted ? That will fix it. ``` ---- gdb-10.2/gdb/thread.c.orig -+++ gdb-10.2/gdb/thread.c -@@ -60,7 +60,7 @@ static thread_info *current_thread_; - - #ifdef CRASH_MERGE - /* Function to set cpu, defined by crash-utility */ --extern "C" void set_cpu (int); -+extern "C" void set_cpu(int cpu, int print_context); - #endif - - /* RAII type used to increase / decrease the refcount of each thread -@@ -1098,6 +1098,9 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, - uiout->table_body (); - } - -+#ifdef CRASH_MERGE -+ int current_cpu = current_thread ? current_thread->ptid.tid() : 0; -+#endif - for (inferior *inf : all_inferiors ()) - for (thread_info *tp : inf->threads ()) - { -@@ -1113,6 +1116,11 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, - - ui_out_emit_tuple tuple_emitter (uiout, NULL); - -+ /* Switch to the thread's CPU in crash */ -+#ifdef CRASH_MERGE -+ set_cpu(tp->ptid.tid(), FALSE); -+#endif -+ ``` 'info threads' keeps changing the gdb context temporarily, while it prints each thread's frame and other information. Correspondingly this changes ensured that crash's context also syncs according to change in gdb's context. Then, when user does 'info threads', it will refresh all regcache since it has to go through each thread. In that case that issue of the same frame being printed for all threads should not happen. That should remove the need to refresh all regcache at init time, and leave it to later when user needs it. Refreshing all regcache at init time is the sure shot way that all regcache are correct, but I guess everyone will anyways do a 'info threads', but we can delay the overhead to later. What do you think ? Thanks, Aditya Gupta > > Thanks, > Tao Liu > > > > > > > Thanks, > > Aditya Gupta > > > > > > > > tt->flags |= TASK_INIT_DONE; > > > } > > > @@ -5315,7 +5316,7 @@ set_context(ulong task, ulong pid, uint update_gdb_thread) > > > > > > /* change the selected thread in gdb, according to current context */ > > > if (update_gdb_thread) > > > - return gdb_change_cpu_context(tc->processor); > > > + return gdb_change_thread_context(tc->task); > > > else > > > return TRUE; > > > } else { > > > -- > > > 2.40.1 > > > > > > -- Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ Contribution Guidelines: https://github.com/crash-utility/crash/wiki

Hi Alexey, On Wed, Mar 13, 2024 at 12:42:22PM -0700, Alexey Makhalov wrote: > Hi folks, > > I'm working on an improvement in crash_target for gdb to backtrace and > debug (including local frame variables) all tasks, not just active > tasks/CPUs > I will send the patch shortly. Thanks for the interest in this. Just in case, with both patch series combined, it should be possible to backtrace and run info locals, etc, on arbitrary tasks also. Interested in your patch though :) Thanks, Aditya Gupta > > Thanks, > --Alexey > > On Wed, Mar 13, 2024 at 2:34 AM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > > > Hi Tao, > > On Wed, Mar 13, 2024 at 11:23:56AM +0800, Tao Liu wrote: > > > Hi Aditya, > > > > > > On Tue, Mar 12, 2024 at 5:12 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; > > wrote: > > > > > > > > Hi Tao, > > > > > > > > > <...> > > > > > > > > > > +crash_target *target = NULL; > > > > > + > > > > > void > > > > > crash_target_init (void) > > > > > { > > > > > int nr_cpus = crash_get_nr_cpus(); > > > > > - crash_target *target = new crash_target (); > > > > > + target = new crash_target (); > > > > > > > > > > /* Own the target until it is successfully pushed. */ > > > > > target_ops_up target_holder (target); > > > > > @@ -137,29 +139,35 @@ crash_target_init (void) > > > > > reinit_frame_cache (); > > > > > } > > > > > > > > > > -/* > > > > > - * Change gdb's thread context to the thread on given CPU > > > > > - **/ > > > > > extern "C" int > > > > > -gdb_change_cpu_context(unsigned int cpu) > > > > > +gdb_change_thread_context (ulong task) > > > > > { > > > > > - ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > > > > - inferior *inf = current_inferior (); > > > > > - thread_info *tp = find_thread_ptid (inf, ptid); > > > > > - > > > > > - if (tp == nullptr) > > > > > + int tried = 0; > > > > > + inferior* inf = current_inferior(); > > > > > + int cpu = crash_set_thread(task); > > > > > + if (cpu < 0) > > > > > return FALSE; > > > > > > > > > > - /* Making sure that crash's context is same */ > > > > > - set_cpu(cpu, FALSE); > > > > > - > > > > > - /* Switch to the thread */ > > > > > - switch_to_thread(tp); > > > > > - > > > > > - /* Fetch/Refresh thread's registers */ > > > > > - gdb_refresh_regcache(cpu); > > > > > + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > > > > > > > > > - return TRUE; > > > > > +retry: > > > > > + thread_info *tp = find_thread_ptid (inf, ptid); > > > > > + if (tp == nullptr && !tried) { > > > > > + thread_info *thread = add_thread_silent(target, > > > > > + ptid_t(CRASH_INFERIOR_PID, 0, cpu)); > > > > > > > > A minor comment. > > > > Is it possible to do it without needing 'target' as global ? I see > > > > there's 'inf->current_top_target()', but it's return type if > > target_ops, > > > > don't know if it can be used. > > > > > > > Thanks for your comments, however I guess it is not workable for the > > > code change. What we want is to add a thread to the target. I found > > > one function "target.c:new_thread", which can be used as > > > "new_thread(current_inferior(), ptid_t(CRASH_INFERIOR_PID, 0, cpu));" > > > to replace the "add_thread_silent()" code, except we need to make > > > "new_thread" to be non-static. > > > > > > I prefer to keep the code as current, because I don't see the side > > > effect of making target as global variable. What do you think? > > > > Then there doesn't seem to be an alternative, then I guess we will have > > to keep the global variable, seems okay to me. > > > > > > > > > > <...> > > > > > > > > > > /* > > > > > * Collect the irq_desc[] entry along with its associated handler > > and > > > > > diff --git a/task.c b/task.c > > > > > index a405b05..ef79f53 100644 > > > > > --- a/task.c > > > > > +++ b/task.c > > > > > @@ -715,7 +715,8 @@ task_init(void) > > > > > * crash_target::fetch_registers, so CPU 0's registers are > > shown as > > > > > * <unavailable> in gdb mode > > > > > * */ > > > > > - gdb_refresh_regcache(0); > > > > > + for (int i = 0; i < get_cpus_online(); i++) > > > > > + gdb_refresh_regcache(i); > > > > > > > > Does x86 require all regcache(s) to be refreshed at init time ? > > > > > > > > Till v4 of my ppc series, I also was doing refresh of all CPUs, but > > it's > > > > better to do it when required, and there was a review in v4, patch 5 > > > > from Lianbo, quoting it here: > > > > > > > > > In addition, I haven't tested how far it takes the time in the > > > > > multi-core environment, is it possible to implement a smaller > > > > > granularity refresh operation? For example: When we need switch to > > > > > another cpu, execute the refresh operation for corresponding > > regcache. > > > > > > > > CPU 0's refresh is required due to gdb having already tried to > > > > initialised registers before we initialised crash_target > > > > > > > > This might add a small noticeable delay on vmcores from big systems. > > > > > > > > Adds around 4.6 seconds for a ppc64 vmcore with 64 CPUs, in crash init > > > > time, while reading it on a fast 48 cpu Power8 system. > > > > This overhead is less for systems with less CPUs, eg. ~0.43 seconds, on > > > > a vmcore of 8 CPUs x86_64 system. > > > > > > Yes, thanks for the testing results. There do have performance > > > downgrades of refreshing all cpu caches. But this will give correct > > > results when user type "info threads" after showing "crash>": > > > > > > With all cpu cache refreshed: > > > crash> info threads > > > Id Target Id Frame > > > 1 CPU 0 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 2 CPU 1 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 3 CPU 2 <unavailable> in ?? () > > > 4 CPU 3 <unavailable> in ?? () > > > 5 CPU 4 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 6 CPU 5 <unavailable> in ?? () > > > 7 CPU 6 <unavailable> in ?? () > > > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 9 CPU 8 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 10 CPU 9 <unavailable> in ?? () > > > 11 CPU 10 <unavailable> in ?? () > > > 12 CPU 11 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 13 CPU 12 <unavailable> in ?? () > > > 14 CPU 13 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 15 CPU 14 <unavailable> in ?? () > > > 16 CPU 15 <unavailable> in ?? () > > > 17 CPU 16 <unavailable> in ?? () > > > 18 CPU 17 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 19 CPU 18 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 20 CPU 19 <unavailable> in ?? () > > > 21 CPU 20 <unavailable> in ?? () > > > 22 CPU 21 <unavailable> in ?? () > > > 23 CPU 22 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 24 CPU 23 <unavailable> in ?? () > > > crash> q > > > > > > With only cpu0 refreshed: > > > crash> info threads > > > Id Target Id Frame > > > 1 CPU 0 native_safe_halt () at > > > arch/x86/include/asm/irqflags.h:54 > > > 2 CPU 1 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 3 CPU 2 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 4 CPU 3 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 5 CPU 4 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 6 CPU 5 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 7 CPU 6 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 9 CPU 8 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 10 CPU 9 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 11 CPU 10 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 12 CPU 11 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 13 CPU 12 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 14 CPU 13 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 15 CPU 14 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 16 CPU 15 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 17 CPU 16 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 18 CPU 17 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 19 CPU 18 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 20 CPU 19 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 21 CPU 20 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 22 CPU 21 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 23 CPU 22 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > 24 CPU 23 blk_mq_rq_timed_out (req=0xffff880fdb246000, > > > reserved=reserved@entry=false) at block/blk-mq.c:640 > > > > > > As we can see, other cpus[1-6, 8-23] just take the reg cache of > > > cpu[7], which is incorrect. And if users go further like, "thread 20" > > > and "gdb bt", it will also give incorrect stack traces. > > > > > > The cpu cache will only get refreshed once user type "set <pid>", so > > > the cpu cache will be refreshed by the <pid> task's context. > > > > > > I doubt a user will understand all the details and constraints, so I'm > > > afraid the user will be confused by the faulty output. But I also have > > > no objection if the performance is the priority. Basically it is a > > > balance of pays and gains. In addition, since cmd "info" and "thread" > > > is a command provided by gdb, currently I don't know how to hack > > > those, so cpu cache can be refreshed when "info threads" or "thread > > > <num>" have been invoked. > > > > > > Do you have any thoughts? > > > > I also had faced that issue initially, ie. the other CPUs using up same > > regcache, if all are not refreshed. > > While iterating through all threads, gdb switches it's context > > temporarily, while crash's context remained same, thus causing gdb to > > get same registers for all threads other than 0. > > > > This was solved in patch #3 (synchronise cpu context changes between > > crash/gdb) > > in the ppc's 'Improve stack unwind on ppc64' series, by syncing gdb's > > context with crash. > > > > Can this change in thread.c in gdb-10.2.patch in patch #2 be reverted ? > > That will fix it. > > > > ``` > > ---- gdb-10.2/gdb/thread.c.orig > > -+++ gdb-10.2/gdb/thread.c > > -@@ -60,7 +60,7 @@ static thread_info *current_thread_; > > - > > - #ifdef CRASH_MERGE > > - /* Function to set cpu, defined by crash-utility */ > > --extern "C" void set_cpu (int); > > -+extern "C" void set_cpu(int cpu, int print_context); > > - #endif > > - > > - /* RAII type used to increase / decrease the refcount of each thread > > -@@ -1098,6 +1098,9 @@ print_thread_info_1 (struct ui_out *uiout, const > > char *requested_threads, > > - uiout->table_body (); > > - } > > - > > -+#ifdef CRASH_MERGE > > -+ int current_cpu = current_thread ? current_thread->ptid.tid() : 0; > > -+#endif > > - for (inferior *inf : all_inferiors ()) > > - for (thread_info *tp : inf->threads ()) > > - { > > -@@ -1113,6 +1116,11 @@ print_thread_info_1 (struct ui_out *uiout, const > > char *requested_threads, > > - > > - ui_out_emit_tuple tuple_emitter (uiout, NULL); > > - > > -+ /* Switch to the thread's CPU in crash */ > > -+#ifdef CRASH_MERGE > > -+ set_cpu(tp->ptid.tid(), FALSE); > > -+#endif > > -+ > > ``` > > > > 'info threads' keeps changing the gdb context temporarily, while it > > prints each thread's frame and other information. > > Correspondingly this changes ensured that crash's context also syncs > > according to change in gdb's context. > > > > Then, when user does 'info threads', it will refresh all regcache since > > it has to go through each thread. > > > > In that case that issue of the same frame being printed for all threads > > should not happen. > > > > That should remove the need to refresh all regcache at init time, and > > leave it to later when user needs it. Refreshing all regcache at init > > time is the sure shot way that all regcache are correct, but I guess > > everyone will anyways do a 'info threads', but we can delay the overhead > > to later. What do you think ? > > > > Thanks, > > Aditya Gupta > > > > > > > > Thanks, > > > Tao Liu > > > > > > > > > > > > > > > > > Thanks, > > > > Aditya Gupta > > > > > > > > > > > > > > tt->flags |= TASK_INIT_DONE; > > > > > } > > > > > @@ -5315,7 +5316,7 @@ set_context(ulong task, ulong pid, uint > > update_gdb_thread) > > > > > > > > > > /* change the selected thread in gdb, according to > > current context */ > > > > > if (update_gdb_thread) > > > > > - return gdb_change_cpu_context(tc->processor); > > > > > + return gdb_change_thread_context(tc->task); > > > > > else > > > > > return TRUE; > > > > > } else { > > > > > -- > > > > > 2.40.1 > > > > > > > > > > > > > > -- > > Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io > > To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io > > https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ > > Contribution Guidelines: https://github.com/crash-utility/crash/wiki > >

Hi Alexey, On Fri, Mar 15, 2024 at 6:55 AM Alexey Makhalov <alexey.makhalov(a)broadcom.com&gt; wrote: > > Please find a patch in "crash_target: Support for GDB debugging of all tasks" mail thread. Use the latest version 3 of the patch. Thanks, --Alexey > > On Wed, Mar 13, 2024 at 10:41 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: >> >> Hi Alexey, >> >> On Wed, Mar 13, 2024 at 12:42:22PM -0700, Alexey Makhalov wrote: >> > Hi folks, >> > >> > I'm working on an improvement in crash_target for gdb to backtrace and >> > debug (including local frame variables) all tasks, not just active >> > tasks/CPUs >> > I will send the patch shortly. >> >> Thanks for the interest in this. Just in case, with both patch series >> combined, it should be possible to backtrace and run info locals, etc, >> on arbitrary tasks also. Interested in your patch though :) >> Yeah, with Aditya's "Improve stack unwind on ppc64" and my "x86_64 gdb stack unwinding support" patch series, arbitrary tasks' stack unwinding/local variables can be viewed by gdb. So there might be some work we have repeated, and I'd like to take reference of your patch and work together for this feature. In addition, I didn't find the "crash_target: Support for GDB debugging of all tasks" patch, could you please just share a link to it?

Thanks, Tao Liu >> Thanks, >> Aditya Gupta >> >> > >> > Thanks, >> > --Alexey >> > >> > On Wed, Mar 13, 2024 at 2:34 AM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: >> > >> > > Hi Tao, >> > > On Wed, Mar 13, 2024 at 11:23:56AM +0800, Tao Liu wrote: >> > > > Hi Aditya, >> > > > >> > > > On Tue, Mar 12, 2024 at 5:12 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; >> > > wrote: >> > > > > >> > > > > Hi Tao, >> > > > > >> > > > > > <...> >> > > > > > >> > > > > > +crash_target *target = NULL; >> > > > > > + >> > > > > > void >> > > > > > crash_target_init (void) >> > > > > > { >> > > > > > int nr_cpus = crash_get_nr_cpus(); >> > > > > > - crash_target *target = new crash_target (); >> > > > > > + target = new crash_target (); >> > > > > > >> > > > > > /* Own the target until it is successfully pushed. */ >> > > > > > target_ops_up target_holder (target); >> > > > > > @@ -137,29 +139,35 @@ crash_target_init (void) >> > > > > > reinit_frame_cache (); >> > > > > > } >> > > > > > >> > > > > > -/* >> > > > > > - * Change gdb's thread context to the thread on given CPU >> > > > > > - **/ >> > > > > > extern "C" int >> > > > > > -gdb_change_cpu_context(unsigned int cpu) >> > > > > > +gdb_change_thread_context (ulong task) >> > > > > > { >> > > > > > - ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); >> > > > > > - inferior *inf = current_inferior (); >> > > > > > - thread_info *tp = find_thread_ptid (inf, ptid); >> > > > > > - >> > > > > > - if (tp == nullptr) >> > > > > > + int tried = 0; >> > > > > > + inferior* inf = current_inferior(); >> > > > > > + int cpu = crash_set_thread(task); >> > > > > > + if (cpu < 0) >> > > > > > return FALSE; >> > > > > > >> > > > > > - /* Making sure that crash's context is same */ >> > > > > > - set_cpu(cpu, FALSE); >> > > > > > - >> > > > > > - /* Switch to the thread */ >> > > > > > - switch_to_thread(tp); >> > > > > > - >> > > > > > - /* Fetch/Refresh thread's registers */ >> > > > > > - gdb_refresh_regcache(cpu); >> > > > > > + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); >> > > > > > >> > > > > > - return TRUE; >> > > > > > +retry: >> > > > > > + thread_info *tp = find_thread_ptid (inf, ptid); >> > > > > > + if (tp == nullptr && !tried) { >> > > > > > + thread_info *thread = add_thread_silent(target, >> > > > > > + ptid_t(CRASH_INFERIOR_PID, 0, cpu)); >> > > > > >> > > > > A minor comment. >> > > > > Is it possible to do it without needing 'target' as global ? I see >> > > > > there's 'inf->current_top_target()', but it's return type if >> > > target_ops, >> > > > > don't know if it can be used. >> > > > > >> > > > Thanks for your comments, however I guess it is not workable for the >> > > > code change. What we want is to add a thread to the target. I found >> > > > one function "target.c:new_thread", which can be used as >> > > > "new_thread(current_inferior(), ptid_t(CRASH_INFERIOR_PID, 0, cpu));" >> > > > to replace the "add_thread_silent()" code, except we need to make >> > > > "new_thread" to be non-static. >> > > > >> > > > I prefer to keep the code as current, because I don't see the side >> > > > effect of making target as global variable. What do you think? >> > > >> > > Then there doesn't seem to be an alternative, then I guess we will have >> > > to keep the global variable, seems okay to me. >> > > >> > > > >> > > > > > <...> >> > > > > > >> > > > > > /* >> > > > > > * Collect the irq_desc[] entry along with its associated handler >> > > and >> > > > > > diff --git a/task.c b/task.c >> > > > > > index a405b05..ef79f53 100644 >> > > > > > --- a/task.c >> > > > > > +++ b/task.c >> > > > > > @@ -715,7 +715,8 @@ task_init(void) >> > > > > > * crash_target::fetch_registers, so CPU 0's registers are >> > > shown as >> > > > > > * <unavailable> in gdb mode >> > > > > > * */ >> > > > > > - gdb_refresh_regcache(0); >> > > > > > + for (int i = 0; i < get_cpus_online(); i++) >> > > > > > + gdb_refresh_regcache(i); >> > > > > >> > > > > Does x86 require all regcache(s) to be refreshed at init time ? >> > > > > >> > > > > Till v4 of my ppc series, I also was doing refresh of all CPUs, but >> > > it's >> > > > > better to do it when required, and there was a review in v4, patch 5 >> > > > > from Lianbo, quoting it here: >> > > > > >> > > > > > In addition, I haven't tested how far it takes the time in the >> > > > > > multi-core environment, is it possible to implement a smaller >> > > > > > granularity refresh operation? For example: When we need switch to >> > > > > > another cpu, execute the refresh operation for corresponding >> > > regcache. >> > > > > >> > > > > CPU 0's refresh is required due to gdb having already tried to >> > > > > initialised registers before we initialised crash_target >> > > > > >> > > > > This might add a small noticeable delay on vmcores from big systems. >> > > > > >> > > > > Adds around 4.6 seconds for a ppc64 vmcore with 64 CPUs, in crash init >> > > > > time, while reading it on a fast 48 cpu Power8 system. >> > > > > This overhead is less for systems with less CPUs, eg. ~0.43 seconds, on >> > > > > a vmcore of 8 CPUs x86_64 system. >> > > > >> > > > Yes, thanks for the testing results. There do have performance >> > > > downgrades of refreshing all cpu caches. But this will give correct >> > > > results when user type "info threads" after showing "crash>": >> > > > >> > > > With all cpu cache refreshed: >> > > > crash> info threads >> > > > Id Target Id Frame >> > > > 1 CPU 0 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 2 CPU 1 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 3 CPU 2 <unavailable> in ?? () >> > > > 4 CPU 3 <unavailable> in ?? () >> > > > 5 CPU 4 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 6 CPU 5 <unavailable> in ?? () >> > > > 7 CPU 6 <unavailable> in ?? () >> > > > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 9 CPU 8 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 10 CPU 9 <unavailable> in ?? () >> > > > 11 CPU 10 <unavailable> in ?? () >> > > > 12 CPU 11 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 13 CPU 12 <unavailable> in ?? () >> > > > 14 CPU 13 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 15 CPU 14 <unavailable> in ?? () >> > > > 16 CPU 15 <unavailable> in ?? () >> > > > 17 CPU 16 <unavailable> in ?? () >> > > > 18 CPU 17 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 19 CPU 18 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 20 CPU 19 <unavailable> in ?? () >> > > > 21 CPU 20 <unavailable> in ?? () >> > > > 22 CPU 21 <unavailable> in ?? () >> > > > 23 CPU 22 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 24 CPU 23 <unavailable> in ?? () >> > > > crash> q >> > > > >> > > > With only cpu0 refreshed: >> > > > crash> info threads >> > > > Id Target Id Frame >> > > > 1 CPU 0 native_safe_halt () at >> > > > arch/x86/include/asm/irqflags.h:54 >> > > > 2 CPU 1 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 3 CPU 2 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 4 CPU 3 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 5 CPU 4 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 6 CPU 5 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 7 CPU 6 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 9 CPU 8 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 10 CPU 9 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 11 CPU 10 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 12 CPU 11 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 13 CPU 12 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 14 CPU 13 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 15 CPU 14 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 16 CPU 15 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 17 CPU 16 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 18 CPU 17 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 19 CPU 18 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 20 CPU 19 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 21 CPU 20 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 22 CPU 21 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 23 CPU 22 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > 24 CPU 23 blk_mq_rq_timed_out (req=0xffff880fdb246000, >> > > > reserved=reserved@entry=false) at block/blk-mq.c:640 >> > > > >> > > > As we can see, other cpus[1-6, 8-23] just take the reg cache of >> > > > cpu[7], which is incorrect. And if users go further like, "thread 20" >> > > > and "gdb bt", it will also give incorrect stack traces. >> > > > >> > > > The cpu cache will only get refreshed once user type "set <pid>", so >> > > > the cpu cache will be refreshed by the <pid> task's context. >> > > > >> > > > I doubt a user will understand all the details and constraints, so I'm >> > > > afraid the user will be confused by the faulty output. But I also have >> > > > no objection if the performance is the priority. Basically it is a >> > > > balance of pays and gains. In addition, since cmd "info" and "thread" >> > > > is a command provided by gdb, currently I don't know how to hack >> > > > those, so cpu cache can be refreshed when "info threads" or "thread >> > > > <num>" have been invoked. >> > > > >> > > > Do you have any thoughts? >> > > >> > > I also had faced that issue initially, ie. the other CPUs using up same >> > > regcache, if all are not refreshed. >> > > While iterating through all threads, gdb switches it's context >> > > temporarily, while crash's context remained same, thus causing gdb to >> > > get same registers for all threads other than 0. >> > > >> > > This was solved in patch #3 (synchronise cpu context changes between >> > > crash/gdb) >> > > in the ppc's 'Improve stack unwind on ppc64' series, by syncing gdb's >> > > context with crash. >> > > >> > > Can this change in thread.c in gdb-10.2.patch in patch #2 be reverted ? >> > > That will fix it. >> > > >> > > ``` >> > > ---- gdb-10.2/gdb/thread.c.orig >> > > -+++ gdb-10.2/gdb/thread.c >> > > -@@ -60,7 +60,7 @@ static thread_info *current_thread_; >> > > - >> > > - #ifdef CRASH_MERGE >> > > - /* Function to set cpu, defined by crash-utility */ >> > > --extern "C" void set_cpu (int); >> > > -+extern "C" void set_cpu(int cpu, int print_context); >> > > - #endif >> > > - >> > > - /* RAII type used to increase / decrease the refcount of each thread >> > > -@@ -1098,6 +1098,9 @@ print_thread_info_1 (struct ui_out *uiout, const >> > > char *requested_threads, >> > > - uiout->table_body (); >> > > - } >> > > - >> > > -+#ifdef CRASH_MERGE >> > > -+ int current_cpu = current_thread ? current_thread->ptid.tid() : 0; >> > > -+#endif >> > > - for (inferior *inf : all_inferiors ()) >> > > - for (thread_info *tp : inf->threads ()) >> > > - { >> > > -@@ -1113,6 +1116,11 @@ print_thread_info_1 (struct ui_out *uiout, const >> > > char *requested_threads, >> > > - >> > > - ui_out_emit_tuple tuple_emitter (uiout, NULL); >> > > - >> > > -+ /* Switch to the thread's CPU in crash */ >> > > -+#ifdef CRASH_MERGE >> > > -+ set_cpu(tp->ptid.tid(), FALSE); >> > > -+#endif >> > > -+ >> > > ``` >> > > >> > > 'info threads' keeps changing the gdb context temporarily, while it >> > > prints each thread's frame and other information. >> > > Correspondingly this changes ensured that crash's context also syncs >> > > according to change in gdb's context. >> > > >> > > Then, when user does 'info threads', it will refresh all regcache since >> > > it has to go through each thread. >> > > >> > > In that case that issue of the same frame being printed for all threads >> > > should not happen. >> > > >> > > That should remove the need to refresh all regcache at init time, and >> > > leave it to later when user needs it. Refreshing all regcache at init >> > > time is the sure shot way that all regcache are correct, but I guess >> > > everyone will anyways do a 'info threads', but we can delay the overhead >> > > to later. What do you think ? >> > > >> > > Thanks, >> > > Aditya Gupta >> > > >> > > > >> > > > Thanks, >> > > > Tao Liu >> > > > >> > > > >> > > > >> > > > > >> > > > > Thanks, >> > > > > Aditya Gupta >> > > > > >> > > > > > >> > > > > > tt->flags |= TASK_INIT_DONE; >> > > > > > } >> > > > > > @@ -5315,7 +5316,7 @@ set_context(ulong task, ulong pid, uint >> > > update_gdb_thread) >> > > > > > >> > > > > > /* change the selected thread in gdb, according to >> > > current context */ >> > > > > > if (update_gdb_thread) >> > > > > > - return gdb_change_cpu_context(tc->processor); >> > > > > > + return gdb_change_thread_context(tc->task); >> > > > > > else >> > > > > > return TRUE; >> > > > > > } else { >> > > > > > -- >> > > > > > 2.40.1 >> > > > > > >> > > > > >> > > > >> > > -- >> > > Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io >> > > To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io >> > > https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ >> > > Contribution Guidelines: https://github.com/crash-utility/crash/wiki >> > >

Hi Tao, On Wed, Mar 13, 2024 at 11:23:56AM +0800, Tao Liu wrote: > Hi Aditya, > > On Tue, Mar 12, 2024 at 5:12 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > > > > Hi Tao, > > > > > <...> > > > > > > +crash_target *target = NULL; > > > + > > > void > > > crash_target_init (void) > > > { > > > int nr_cpus = crash_get_nr_cpus(); > > > - crash_target *target = new crash_target (); > > > + target = new crash_target (); > > > > > > /* Own the target until it is successfully pushed. */ > > > target_ops_up target_holder (target); > > > @@ -137,29 +139,35 @@ crash_target_init (void) > > > reinit_frame_cache (); > > > } > > > > > > -/* > > > - * Change gdb's thread context to the thread on given CPU > > > - **/ > > > extern "C" int > > > -gdb_change_cpu_context(unsigned int cpu) > > > +gdb_change_thread_context (ulong task) > > > { > > > - ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > > - inferior *inf = current_inferior (); > > > - thread_info *tp = find_thread_ptid (inf, ptid); > > > - > > > - if (tp == nullptr) > > > + int tried = 0; > > > + inferior* inf = current_inferior(); > > > + int cpu = crash_set_thread(task); > > > + if (cpu < 0) > > > return FALSE; > > > > > > - /* Making sure that crash's context is same */ > > > - set_cpu(cpu, FALSE); > > > - > > > - /* Switch to the thread */ > > > - switch_to_thread(tp); > > > - > > > - /* Fetch/Refresh thread's registers */ > > > - gdb_refresh_regcache(cpu); > > > + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); > > > > > > - return TRUE; > > > +retry: > > > + thread_info *tp = find_thread_ptid (inf, ptid); > > > + if (tp == nullptr && !tried) { > > > + thread_info *thread = add_thread_silent(target, > > > + ptid_t(CRASH_INFERIOR_PID, 0, cpu)); > > > > A minor comment. > > Is it possible to do it without needing 'target' as global ? I see > > there's 'inf->current_top_target()', but it's return type if target_ops, > > don't know if it can be used. > > > Thanks for your comments, however I guess it is not workable for the > code change. What we want is to add a thread to the target. I found > one function "target.c:new_thread", which can be used as > "new_thread(current_inferior(), ptid_t(CRASH_INFERIOR_PID, 0, cpu));" > to replace the "add_thread_silent()" code, except we need to make > "new_thread" to be non-static. > > I prefer to keep the code as current, because I don't see the side > effect of making target as global variable. What do you think? Then there doesn't seem to be an alternative, then I guess we will have to keep the global variable, seems okay to me. > > > > <...> > > > > > > /* > > > * Collect the irq_desc[] entry along with its associated handler and > > > diff --git a/task.c b/task.c > > > index a405b05..ef79f53 100644 > > > --- a/task.c > > > +++ b/task.c > > > @@ -715,7 +715,8 @@ task_init(void) > > > * crash_target::fetch_registers, so CPU 0's registers are shown as > > > * <unavailable> in gdb mode > > > * */ > > > - gdb_refresh_regcache(0); > > > + for (int i = 0; i < get_cpus_online(); i++) > > > + gdb_refresh_regcache(i); > > > > Does x86 require all regcache(s) to be refreshed at init time ? > > > > Till v4 of my ppc series, I also was doing refresh of all CPUs, but it's > > better to do it when required, and there was a review in v4, patch 5 > > from Lianbo, quoting it here: > > > > > In addition, I haven't tested how far it takes the time in the > > > multi-core environment, is it possible to implement a smaller > > > granularity refresh operation? For example: When we need switch to > > > another cpu, execute the refresh operation for corresponding regcache. > > > > CPU 0's refresh is required due to gdb having already tried to > > initialised registers before we initialised crash_target > > > > This might add a small noticeable delay on vmcores from big systems. > > > > Adds around 4.6 seconds for a ppc64 vmcore with 64 CPUs, in crash init > > time, while reading it on a fast 48 cpu Power8 system. > > This overhead is less for systems with less CPUs, eg. ~0.43 seconds, on > > a vmcore of 8 CPUs x86_64 system. > > Yes, thanks for the testing results. There do have performance > downgrades of refreshing all cpu caches. But this will give correct > results when user type "info threads" after showing "crash>": > > With all cpu cache refreshed: > crash> info threads > Id Target Id Frame > 1 CPU 0 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 2 CPU 1 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 3 CPU 2 <unavailable> in ?? () > 4 CPU 3 <unavailable> in ?? () > 5 CPU 4 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 6 CPU 5 <unavailable> in ?? () > 7 CPU 6 <unavailable> in ?? () > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 9 CPU 8 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 10 CPU 9 <unavailable> in ?? () > 11 CPU 10 <unavailable> in ?? () > 12 CPU 11 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 13 CPU 12 <unavailable> in ?? () > 14 CPU 13 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 15 CPU 14 <unavailable> in ?? () > 16 CPU 15 <unavailable> in ?? () > 17 CPU 16 <unavailable> in ?? () > 18 CPU 17 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 19 CPU 18 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 20 CPU 19 <unavailable> in ?? () > 21 CPU 20 <unavailable> in ?? () > 22 CPU 21 <unavailable> in ?? () > 23 CPU 22 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 24 CPU 23 <unavailable> in ?? () > crash> q > > With only cpu0 refreshed: > crash> info threads > Id Target Id Frame > 1 CPU 0 native_safe_halt () at > arch/x86/include/asm/irqflags.h:54 > 2 CPU 1 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 3 CPU 2 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 4 CPU 3 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 5 CPU 4 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 6 CPU 5 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 7 CPU 6 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 9 CPU 8 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 10 CPU 9 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 11 CPU 10 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 12 CPU 11 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 13 CPU 12 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 14 CPU 13 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 15 CPU 14 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 16 CPU 15 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 17 CPU 16 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 18 CPU 17 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 19 CPU 18 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 20 CPU 19 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 21 CPU 20 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 22 CPU 21 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 23 CPU 22 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > 24 CPU 23 blk_mq_rq_timed_out (req=0xffff880fdb246000, > reserved=reserved@entry=false) at block/blk-mq.c:640 > > As we can see, other cpus[1-6, 8-23] just take the reg cache of > cpu[7], which is incorrect. And if users go further like, "thread 20" > and "gdb bt", it will also give incorrect stack traces. > > The cpu cache will only get refreshed once user type "set <pid>", so > the cpu cache will be refreshed by the <pid> task's context. > > I doubt a user will understand all the details and constraints, so I'm > afraid the user will be confused by the faulty output. But I also have > no objection if the performance is the priority. Basically it is a > balance of pays and gains. In addition, since cmd "info" and "thread" > is a command provided by gdb, currently I don't know how to hack > those, so cpu cache can be refreshed when "info threads" or "thread > <num>" have been invoked. > > Do you have any thoughts? I also had faced that issue initially, ie. the other CPUs using up same regcache, if all are not refreshed. While iterating through all threads, gdb switches it's context temporarily, while crash's context remained same, thus causing gdb to get same registers for all threads other than 0. This was solved in patch #3 (synchronise cpu context changes between crash/gdb) in the ppc's 'Improve stack unwind on ppc64' series, by syncing gdb's context with crash. Can this change in thread.c in gdb-10.2.patch in patch #2 be reverted ? That will fix it.

``` ---- gdb-10.2/gdb/thread.c.orig -+++ gdb-10.2/gdb/thread.c -@@ -60,7 +60,7 @@ static thread_info *current_thread_; - - #ifdef CRASH_MERGE - /* Function to set cpu, defined by crash-utility */ --extern "C" void set_cpu (int); -+extern "C" void set_cpu(int cpu, int print_context); - #endif - - /* RAII type used to increase / decrease the refcount of each thread -@@ -1098,6 +1098,9 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, - uiout->table_body (); - } - -+#ifdef CRASH_MERGE -+ int current_cpu = current_thread ? current_thread->ptid.tid() : 0; -+#endif - for (inferior *inf : all_inferiors ()) - for (thread_info *tp : inf->threads ()) - { -@@ -1113,6 +1116,11 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads, - - ui_out_emit_tuple tuple_emitter (uiout, NULL); - -+ /* Switch to the thread's CPU in crash */ -+#ifdef CRASH_MERGE -+ set_cpu(tp->ptid.tid(), FALSE); -+#endif -+ ``` 'info threads' keeps changing the gdb context temporarily, while it prints each thread's frame and other information. Correspondingly this changes ensured that crash's context also syncs according to change in gdb's context. Then, when user does 'info threads', it will refresh all regcache since it has to go through each thread. In that case that issue of the same frame being printed for all threads should not happen. That should remove the need to refresh all regcache at init time, and leave it to later when user needs it. Refreshing all regcache at init time is the sure shot way that all regcache are correct, but I guess everyone will anyways do a 'info threads', but we can delay the overhead to later. What do you think ? Thanks, Aditya Gupta > > Thanks, > Tao Liu > > > > > > > Thanks, > > Aditya Gupta > > > > > > > > tt->flags |= TASK_INIT_DONE; > > > } > > > @@ -5315,7 +5316,7 @@ set_context(ulong task, ulong pid, uint update_gdb_thread) > > > > > > /* change the selected thread in gdb, according to current context */ > > > if (update_gdb_thread) > > > - return gdb_change_cpu_context(tc->processor); > > > + return gdb_change_thread_context(tc->task); > > > else > > > return TRUE; > > > } else { > > > -- > > > 2.40.1 > > > > > >

Hi Tao, On Fri, Mar 15, 2024 at 08:33:31PM +0800, Tao Liu wrote: > Hi Aditya, > > > > As we can see, other cpus[1-6, 8-23] just take the reg cache of > > > cpu[7], which is incorrect. And if users go further like, "thread 20" > > > and "gdb bt", it will also give incorrect stack traces. > > > > > > The cpu cache will only get refreshed once user type "set <pid>", so > > > the cpu cache will be refreshed by the <pid> task's context. > > > > > > I doubt a user will understand all the details and constraints, so I'm > > > afraid the user will be confused by the faulty output. But I also have > > > no objection if the performance is the priority. Basically it is a > > > balance of pays and gains. In addition, since cmd "info" and "thread" > > > is a command provided by gdb, currently I don't know how to hack > > > those, so cpu cache can be refreshed when "info threads" or "thread > > > <num>" have been invoked. > > > > > > Do you have any thoughts? > > > > I also had faced that issue initially, ie. the other CPUs using up same > > regcache, if all are not refreshed. > > While iterating through all threads, gdb switches it's context > > temporarily, while crash's context remained same, thus causing gdb to > > get same registers for all threads other than 0. > > > > This was solved in patch #3 (synchronise cpu context changes between crash/gdb) > > in the ppc's 'Improve stack unwind on ppc64' series, by syncing gdb's > > context with crash. > > > > Can this change in thread.c in gdb-10.2.patch in patch #2 be reverted ? > > That will fix it. > > Could you share your patch, based on your v10 and my v1 patch series, > so I can get a clue how to do this? Sure tao, i will attach it to the end of this mail. Basically what I did is to revert changes to gdb-10.2.patch in this patch. I pushed it along with testing with only regcache_refresh for CPU 0 instead of all CPUs, to: https://github.com/adi-g15-ibm/crash/tree/tmp-test-branch-10928

> > I tried but was unsuccessful. Since I have changed your #3 patch a bit > in my v1 patch series, such as gdb_change_cpu_context() -> > gdb_change_thread_context(), I doubt that's the reason for failing. > > What I did is keeping "set_cpu" in thread.c:thread_command() as the > gdb-10.2.patch describes in your #3 patch. But only one thread gets > refreshed when I invoke "thread X", and no regcache refreshed when > invoke "info threads". If i understand clearly, "thread X" causing refresh for one thread/CPU is expected, as we want only registers for "X" to be refreshed. But 'info threads' not refreshing any regcache should be solved by the restoring changes to gdb-10.2.patch to do the 'set_cpu' in the thread_command. Thanks Tao, - Aditya Gupta commit d1ad22747de0b6c9846ecc8ea746ee9a38c7dade Author: Tao Liu <ltao(a)redhat.com&gt; Date: Wed Feb 14 10:44:54 2024 +0800 change thread context Previously we can only view the stack unwinding for the tasks which are running on each CPUs. This patch will enable the ability to view arbitrary tasks stack unwinding. After crash get initialized, "info threads" will output like the following: crash> info threads Id Target Id Frame 1 CPU 0 native_safe_halt () at arch/x86/include/asm/irqflags.h:54 ... * 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, reserved=reserved@entry=false) at block/blk-mq.c:640 ... 13 CPU 12 <unavailable> in ?? () 14 CPU 13 native_safe_halt () at arch/x86/include/asm/irqflags.h:54 ... crash> ps PID PPID CPU TASK ST %MEM VSZ RSS COMM > 0 0 0 ffffffff819f9480 RU 0.0 0 0 [swapper/0] > 0 0 1 ffff880169411fa0 RU 0.0 0 0 [swapper/1] ... 0 0 23 ffff8801694e0000 RU 0.0 0 0 [swapper/23] 1 0 13 ffff880169b30000 IN 0.0 193052 4180 systemd "info threads" show the tasks which are currently running on each CPU. If we'd like to view systemd, which are not running, task's stack unwinding, we do the following: crash> set 1 or crash> set ffff880169b30000 Then the register cache of systemd will be swapped into CPU 13: crash> info threads crash> info threads Id Target Id Frame 1 CPU 0 native_safe_halt () at arch/x86/include/asm/irqflags.h:54 ... 8 CPU 7 blk_mq_rq_timed_out (req=0xffff880fdb246000, reserved=reserved@entry=false) at block/blk-mq.c:640 ... 13 CPU 12 <unavailable> in ?? () * 14 CPU 13 0xffffffff816a8f65 in context_switch (rq=0x0, next=0x0, prev=0xffff880169b30000) at kernel/sched/core.c:2527 ... And we can view the stack unwinding of systemd: crash> bt PID: 1 TASK: ffff880169b30000 CPU: 13 COMMAND: "systemd" #0 [ffff880169b3bd58] __schedule at ffffffff816a8f65 #1 [ffff880169b3bdc0] schedule at ffffffff816a94e9 #2 [ffff880169b3bdd0] schedule_hrtimeout_range_clock at ffffffff816a86fd #3 [ffff880169b3be68] schedule_hrtimeout_range at ffffffff816a8733 #4 [ffff880169b3be78] ep_poll at ffffffff8124bb7e #5 [ffff880169b3bf30] sys_epoll_wait at ffffffff8124d00d #6 [ffff880169b3bf80] system_call_fastpath at ffffffff816b5009 RIP: 00007f0449407923 RSP: 00007ffc35a3c378 RFLAGS: 00010246 RAX: 00000000000000e8 RBX: ffffffff816b5009 RCX: 0000000000000071 RDX: 000000000000001d RSI: 00007ffc35a3d5a0 RDI: 0000000000000004 RBP: 00007ffc35a3d810 R8: 0000000000000000 R9: 0000000000000000 R10: 00000000ffffffff R11: 0000000000000293 R12: 0000563ca2ebe980 R13: 0000000000000003 R14: ffffffffffffffff R15: 0000000000000001 ORIG_RAX: 00000000000000e8 CS: 0033 SS: 002b crash> gdb bt #0 0xffffffff816a8f65 in context_switch (rq=0x0, next=0x0, prev=0xffff880169b30000) at kernel/sched/core.c:2527 #1 __schedule () at kernel/sched/core.c:3540 #2 0xffffffff816a94e9 in schedule () at kernel/sched/core.c:3577 #3 0xffffffff816a86fd in schedule_hrtimeout_range_clock (expires=expires@entry=0x0, delta=delta@entry=0, mode=mode@entry=HRTIMER_MODE_ABS, clock=clock@entry=1) at kernel/hrtimer.c:1724 #4 0xffffffff816a8733 in schedule_hrtimeout_range (expires=expires@entry=0x0, delta=delta@entry=0, mode=mode@entry=HRTIMER_MODE_ABS) at kernel/hrtimer.c:1778 #5 0xffffffff8124bb7e in ep_poll (ep=0xffff880fd861f8c0, events=events@entry=0x7ffc35a3d5a0, maxevents=maxevents@entry=29, timeout=timeout@entry=-1) at fs/eventpoll.c:1669 #6 0xffffffff8124d00d in SYSC_epoll_wait (timeout=<optimized out>, maxevents=29, events=<optimized out>, epfd=<optimized out>) at fs/eventpoll.c:2043 #7 SyS_epoll_wait (epfd=<optimized out>, events=140721208415648, maxevents=29, timeout=4294967295) at fs/eventpoll.c:2008 #8 <signal handler called> #9 0x00007f0449407923 in ?? () Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com&gt; diff --git a/crash_target.c b/crash_target.c index d06383f594aa..1df1e9d34a45 100644 --- a/crash_target.c +++ b/crash_target.c @@ -29,10 +29,10 @@ extern "C" int gdb_readmem_callback(unsigned long, void *, int, int); extern "C" int crash_get_nr_cpus(void); extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname, int regsize, void *val); -extern "C" int gdb_change_cpu_context (unsigned int cpu); extern "C" void gdb_refresh_regcache(unsigned int cpu); extern "C" int set_cpu(int cpu, int print_context); - +extern "C" int crash_set_thread(ulong); +extern "C" int gdb_change_thread_context (ulong task); /* The crash target. */ @@ -110,11 +110,13 @@ crash_target::xfer_partial (enum target_object object, const char *annex, #define CRASH_INFERIOR_PID 1 +crash_target *target = NULL; + void crash_target_init (void) { int nr_cpus = crash_get_nr_cpus(); - crash_target *target = new crash_target (); + target = new crash_target (); /* Own the target until it is successfully pushed. */ target_ops_up target_holder (target); @@ -137,27 +139,33 @@ crash_target_init (void) reinit_frame_cache (); } -/* - * Change gdb's thread context to the thread on given CPU - **/ extern "C" int -gdb_change_cpu_context(unsigned int cpu) +gdb_change_thread_context (ulong task) { + int tried = 0; + inferior* inf = current_inferior (); + int cpu = crash_set_thread(task); + if (cpu < 0) + return FALSE; + ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); - inferior *inf = current_inferior (); + +retry: thread_info *tp = find_thread_ptid (inf, ptid); + if (tp == nullptr && !tried) { + thread_info *thread = add_thread_silent(target, ptid_t(CRASH_INFERIOR_PID, 0, cpu)); + tried++; + if (thread) { + goto retry; + } + } - if (tp == nullptr) + if (tp == nullptr && tried) return FALSE; - /* Making sure that crash's context is same */ - set_cpu(cpu, FALSE); - - /* Switch to the thread */ + target_fetch_registers(get_thread_regcache(tp), -1); switch_to_thread(tp); - - /* Fetch/Refresh thread's registers */ - gdb_refresh_regcache(cpu); + reinit_frame_cache (); return TRUE; } diff --git a/defs.h b/defs.h index 49b606979d9e..d5cef621b465 100644 --- a/defs.h +++ b/defs.h @@ -8192,7 +8192,6 @@ enum ppc64_regnum { }; /* crash_target.c */ -extern int gdb_change_cpu_context (unsigned int cpu); extern void gdb_refresh_regcache (unsigned int cpu); #endif /* !GDB_COMMON */ diff --git a/kernel.c b/kernel.c index ea5b5cb32914..50832ed906e5 100644 --- a/kernel.c +++ b/kernel.c @@ -6544,6 +6544,29 @@ set_cpu(int cpu, int print_context) show_context(CURRENT_CONTEXT()); } +int +crash_set_thread(ulong task) +{ + bool found = FALSE; + struct task_context *tc = FIRST_CONTEXT(); + + for (int i = 0; i < RUNNING_TASKS(); i++, tc++) { + if (tc->task == task) { + found = TRUE; + break; + } + } + + if (!found) + return -1; + + if (CURRENT_TASK() == tc->task) + return tc->processor; + + set_context(tc->task, NO_PID); + return tc->processor; +} + /* * Collect the irq_desc[] entry along with its associated handler and diff --git a/task.c b/task.c index a405b05a47d1..ef79f533f11a 100644 --- a/task.c +++ b/task.c @@ -715,7 +715,8 @@ task_init(void) * crash_target::fetch_registers, so CPU 0's registers are shown as * <unavailable> in gdb mode * */ - gdb_refresh_regcache(0); + for (int i = 0; i < get_cpus_online(); i++) + gdb_refresh_regcache(i); tt->flags |= TASK_INIT_DONE; } @@ -5315,7 +5316,7 @@ set_context(ulong task, ulong pid, uint update_gdb_thread) /* change the selected thread in gdb, according to current context */ if (update_gdb_thread) - return gdb_change_cpu_context(tc->processor); + return gdb_change_thread_context(tc->task); else return TRUE; } else {

<...> diff --git a/ppc64.c b/ppc64.c index f86d951..a87e621 100644 --- a/ppc64.c +++ b/ppc64.c @@ -2516,11 +2516,6 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, ulong ip, sp; bool ret = FALSE; - if (LIVE()) { - /* doesn't support reading registers in live dump */ - return FALSE; - } -