[Crash-utility] Re: [PATCH v7 3/5] synchronise cpu context changes between crash/gdb

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024/02/08 20:21, Aditya Gupta wrote:
> Hi Kazu,
> 
> On Thu, Feb 08, 2024 at 08:40:26AM +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
>> <...snip...>
>>
>>> Currently, both crash context and gdb's current thread context were
>>> pretty independent, and could be different, for example, crash commands
>>> might be working on thread 6 (CPU 5), but GDB passthroughs will be
>>> working on thread 2 (CPU 1).
>>>
>>> This was not a problem earlier since interaction of crash and gdb was
>>> not depending on current context for most part. But for gdb passthroughs
>>
>>> to work correctly, gdb needs register values from crash, which depend on
>>> current context in crash.
>>
>> Is this true?
>>
>> CURRENT_CONTEXT() is used in ppc64_get_cpu_reg, but get_cpu_reg returns
>> a register value from a cpu, so can't we use get_active_task(cpu) and
>> task_to_context()?  if usable, maybe some of the temporary set_cpu()s
>> can be removed.
> 
> Nice idea, true, 'get_active_task' can be used instead of me depending
> on CURRENT_CONTEXT.
> 
> But, seems everytime 'get_cpu_reg' will be called, 'get_active_task'
> will have to go through all tasks to get the active task. Can I add this
> if condition in 'get_active_task' to optimise it:

It looks like most (all?) dumpfiles should have tt->panic_threads, so 
the linear search may not be done with a dumpfile.

         if (has_cpu && (tt->flags & POPULATE_PANIC))
                 tt->panic_threads[tc->processor] = tc->task;

but yes, if there is a slow command, it's good to optimize.

Thanks,
Kazu

> 
>      --- a/task.c
>      +++ b/task.c
>      @@ -6308,6 +6308,11 @@ get_active_task(int cpu)
>              if (DUMPFILE() && (task = tt->panic_threads[cpu]))
>                      return task;
>      
>      +       /* return current task if already set to required active task */
>      +       tc = CURRENT_CONTEXT();
>      +       if ((tc->processor == cpu) && is_task_active(tc->task))
>      +               return tc->task;
>      +
>               tc = FIRST_CONTEXT();
>               for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
>                       if ((tc->processor == cpu) && is_task_active(tc->task))
> 
>>
>> The synchronization between crash and gdb is nice for usability, though.
> 
> Yes, I think it's better to not remove the set_cpu()s done in
> gdb_refresh_regcache(), so that there are no issues due to the gdb and
> crash contexts not being in sync.
> 
>>
>> (it's very helpful if you would rebase the patch set to the latest, when
>> you update this.)
> 
> Sure, i will.
> 
> Thanks,
> Aditya Gupta
> 
>>
>> Thanks,
>> Kazu
>>
>>>
>>> Synchronise 'thread' command in gdb with 'set -c' command in crash.
>>> 1. crash -> gdb synchronisation:
>>>      Everytime crash's context changes, a helper is called to switch to
>>>      the thread on that CPU in gdb. The function has been implemented in
>>>      crash_target.c, since gdb functions are accessible inside
>>>      'crash_target.c', and the thread ID to CPU ID mapping is also done
>>>      by the crash_target, during initially registering the threads with
>>>      gdb. With this implementation, GDB's default thread initially also
>>>      changes to the crashing thread, so a switch to crashing thread
>>>      manually isn't required anymore
>>>
>>> 2. gdb -> crash synchronisation:
>>>      gdb has been patched to call 'set_cpu' whenever user switches to any
>>>      thread.
>>>
>>> Signed-off-by: Aditya Gupta <adityag@xxxxxxxxxxxxx>
>>> ---
>>>    crash_target.c | 24 ++++++++++++++++++++++++
>>>    defs.h         |  3 +++
>>>    gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++
>>>    kernel.c       |  8 +++++++-
>>>    task.c         |  4 +++-
>>>    5 files changed, 67 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/crash_target.c b/crash_target.c
>>> index 455480679741..a9d7eea8a80e 100644
>>> --- a/crash_target.c
>>> +++ b/crash_target.c
>>> @@ -29,6 +29,8 @@ 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" int set_cpu (int cpu);
>>>    
>>>    
>>>    /* The crash target.  */
>>> @@ -133,3 +135,25 @@ crash_target_init (void)
>>>      /* Now, set up the frame cache. */
>>>      reinit_frame_cache ();
>>>    }
>>> +
>>> +/*
>>> + * Change gdb's thread context to the thread on given CPU
>>> + **/
>>> +extern "C" int
>>> +gdb_change_cpu_context(unsigned int cpu)
>>> +{
>>> +  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)
>>> +    return FALSE;
>>> +
>>> +  /* Making sure that crash's context is same */
>>> +  set_cpu(cpu);
>>> +
>>> +  /* Switch to the thread */
>>> +  switch_to_thread(tp);
>>> +  return TRUE;
>>> +}
>>> +
>>> diff --git a/defs.h b/defs.h
>>> index 615f3a37935a..7f7a12753658 100644
>>> --- a/defs.h
>>> +++ b/defs.h
>>> @@ -7976,4 +7976,7 @@ enum ppc64_regnum {
>>>    	PPC64_VRSAVE_REGNU = 139
>>>    };
>>>    
>>> +/* crash_target.c */
>>> +extern int gdb_change_cpu_context (unsigned int cpu);
>>> +
>>>    #endif /* !GDB_COMMON */
>>> diff --git a/gdb-10.2.patch b/gdb-10.2.patch
>>> index 2f7d585105aa..4698079ca343 100644
>>> --- a/gdb-10.2.patch
>>> +++ b/gdb-10.2.patch
>>> @@ -10,6 +10,7 @@
>>>    
>>>    tar xvzmf gdb-10.2.tar.gz \
>>>    	gdb-10.2/gdb/symtab.c \
>>> +	gdb-10.2/gdb/thread.c \
>>>    	gdb-10.2/gdb/printcmd.c \
>>>    	gdb-10.2/gdb/symfile.c \
>>>    	gdb-10.2/gdb/Makefile.in \
>>> @@ -3237,3 +3238,32 @@ exit 0
>>>     
>>>           for (compunit_symtab *cust : objfile->compunits ())
>>>             {
>>> +--- 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.  */
>>> diff --git a/kernel.c b/kernel.c
>>> index 52b7ba09f390..a8d60507dd95 100644
>>> --- a/kernel.c
>>> +++ b/kernel.c
>>> @@ -6504,7 +6504,13 @@ set_cpu(int cpu)
>>>    	if (hide_offline_cpu(cpu))
>>>    		error(FATAL, "invalid cpu number: cpu %d is OFFLINE\n", cpu);
>>>    
>>> -	if ((task = get_active_task(cpu)))
>>> +	task = get_active_task(cpu);
>>> +
>>> +	/* Check if context is already set to given cpu */
>>> +	if (task == CURRENT_TASK())
>>> +		return;
>>> +
>>> +	if (task)
>>>    		set_context(task, NO_PID);
>>>    	else
>>>    		error(FATAL, "cannot determine active task on cpu %ld\n", cpu);
>>> diff --git a/task.c b/task.c
>>> index ebdb5be3786f..3a190cafbacb 100644
>>> --- a/task.c
>>> +++ b/task.c
>>> @@ -5301,7 +5301,9 @@ set_context(ulong task, ulong pid)
>>>    
>>>    	if (found) {
>>>    		CURRENT_CONTEXT() = tc;
>>> -		return TRUE;
>>> +
>>> +		/* change the selected thread in gdb, according to current context */
>>> +		return gdb_change_cpu_context(tc->processor);
>>>    	} else {
>>>    		if (task)
>>>    			error(INFO, "cannot set context for task: %lx\n", task);
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux