Re: [RFC PATCH 4/6] Linux kernel debug infrastructure and target Linux kernel

[Philipp Rudo <prudo at linux dot ibm dot com>]

Hi Omair,

On Mon, 4 Feb 2019 14:35:26 +0500
Omair Javaid <omair.javaid@linaro.org> wrote:

> Hi Philipp,
> 
> Thanks for your review. My comments inline.
> 
> On Thu, 31 Jan 2019 at 21:14, Philipp Rudo <prudo@linux.ibm.com> wrote:
> >
> > Hi Omair,
> >
> > On Tue, 29 Jan 2019 10:03:17 +0500
> > Omair Javaid <omair.javaid@linaro.org> wrote:
> >
> > [...]
> >  
> > > diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> > > index 434ee3b..5e88623 100644
> > > --- a/gdb/gdbarch.c
> > > +++ b/gdb/gdbarch.c
> > > @@ -3,7 +3,7 @@
> > >
> > >  /* Dynamic architecture support for GDB, the GNU debugger.
> > >
> > > -   Copyright (C) 1998-2019 Free Software Foundation, Inc.
> > > +   Copyright (C) 1998-2018 Free Software Foundation, Inc.  
> >
> > that doesn't look right. Same for gdbarch.h.  
> 
> This probably required fixing gdbarch.sh to generate the correct
> years. I didnt change it because i didnt expect this patchset to go
> immediately.

True. There's still 2018 in gdbarch.sh:copyright. Looks like all files
with MULTIPLE_COPYRIGHT_HEADERS in copyright.py were forgotten to be
updated this year...
 
> > [...]
> >  
> > > +size_t
> > > +lk_bitmap::hweight () const
> > > +{
> > > +  size_t ret = 0;
> > > +//  for (auto bit : *this)
> > > +//    ret++;
> > > +  return ret;
> > > +}  
> >
> > Is this commented out for a reason?  
> 
> This function is not being used anywhere and should be removed. I ll
> do that in follow up patchset.

I originally added it to have a simple interface for e.g. the number of
online cpus. But yes, when nobody is using it simply remove it. If
needed it can still be added later.

> 
> >
> > [...]
> >  
> > > diff --git a/gdb/lk-low.c b/gdb/lk-low.c
> > > new file mode 100644
> > > index 0000000..1931964
> > > --- /dev/null
> > > +++ b/gdb/lk-low.c  
> >
> > [...]
> >  
> > > +/* See lk-low.h.  */
> > > +
> > > +bool
> > > +linux_kernel_ops::update_tasks ()
> > > +{
> > > +  lk_task_ptid_list now_running_ptids;
> > > +  lk_task_ptid_map new_task_struct_ptids;
> > > +  auto last_cpu_task_struct_addr = cpu_curr_task_struct_addr;
> > > +  int inf_pid = current_inferior ()->pid;
> > > +
> > > +  /* Clear cpu_task_struct_addr and cpu_ptid_pair cache that we created
> > > +     on last stop.  */
> > > +  cpu_curr_task_struct_addr.clear();
> > > +  cpu_ptid_pair.clear();
> > > +  tid_task_struct.clear();
> > > +  lk_thread_count = 1;
> > > +
> > > +  /* Iterate over all threads and register target beneath threads.  */
> > > +  for (thread_info *tp : all_threads_safe ())
> > > +    {
> > > +      /* Check if this task represents a CPU.  */
> > > +      if (tp->ptid.tid () == 0)  
> >
> > why not  
> 
> This function is basically run on each stop where we update running
> and sleeping kernel tasks.
> I think I ll add more information in comments of this function to make
> more sense.

That would be good. For me it was very confusing. I understand what
should be done. But I don't understand how it is done.

> >
> > if (tp->ptid.tid () != 0)
> >         continue;
> >
> > Then you wouldn't need the extra indent on the whole for-loop.
> >  
> > > +        {
> > > +          //TODO: Can we have a target beneath thread with lwp != cpu ???  
> >
> > Yes, you can. For core files the lwp is whatever the pr_pid field in
> > the elf_prstatus is. So in this case it depends on who created the dump.
> > For s390 kernel dumps the pr_pid is a cpu id. But in theory it could
> > also be the pid of the task that was running when the dump was created.  
> 
> Is there a way to map task to their specific CPUs. One way to use
> gdb's CPU numbers and other in case lwp is PID is to match PIDs.
> But in absence of both is there a way like may be reading current PC
> cached somewhere in kernel data structures?

I don't think there is a suitable generic way to get the map. That's 
why I made the beneath_thread_to_cpu method in my patches virtual so
every architecture can implement their own way to initialize the map.
That allowed me for s390 to initialize the map via the lowcore (a s390
specific per-cpu structure). But as said this is s390 specific and does
not work on other architectures.

Using the PC won't help. The problem here is that the kernel structures
where it is stored (note also arch specific) are only updated when the
task is scheduled. So for running tasks the PC you find in memory and
the one you find in the regcache will be out of sync.

My best guess for a 'generic' solution would be to check if the SP
points to the kernel stack of a task. But stack handling is arch
specific as well. Furthermore, you usually have multiple kernel stacks
to check and most likely you'd need proper stack unwinding for it to
work. So all in all quite complicated with lot's of arch specific code.

> >
> > [...]
> >  
> > > diff --git a/gdb/lk-low.h b/gdb/lk-low.h
> > > new file mode 100644
> > > index 0000000..103d49f
> > > --- /dev/null
> > > +++ b/gdb/lk-low.h
> > > @@ -0,0 +1,354 @@  
> >
> > [...]
> >  
> > > +  /* Holds Linux kernel target tids as key and
> > > +     corresponding task struct address as value.  */
> > > +  lk_tid_task_map tid_task_struct;
> > > +
> > > +  /* Maps cpu number to linux kernel and target beneath ptids.  */
> > > +  lk_task_ptid_pair_map cpu_ptid_pair;
> > > +
> > > +  /* Maps task_struct addresses to ptids.  */
> > > +  lk_task_ptid_map task_struct_ptids;
> > > +
> > > +  /* Holds cpu to current running task struct address mappings.  */
> > > +  lk_cpu_task_struct_map cpu_curr_task_struct_addr;
> > > +
> > > +  /* Holds cpu to current idle task struct address mappings.  */
> > > +  lk_cpu_task_struct_map cpu_idle_task_struct_addr;  
> >
> > Why are you tracking the idle task separately? Why can't they be treated
> > like 'normal' tasks?
> >
> > For me it looks like you only need this to handle the idle task
> > slightly different in update_tasks. What exactly are you doing different
> > with it?  
> Yes you are right So when we traverse task structs we create a per-cpu
> idle task map and helps us figure out if this cpu is running idle or
> something else in update tasks.

Well sure it is interesting to know if a cpu is idle or runs some 
workload. But why do you need a special map to keep the idle tasks?
Wouldn't be a check task_struct->pid == 0 enough?

Furthermore, I don't understand why you skip the idle tasks for the
sleeping tasks, i.e. when you traverse the task_structs. For me it
would make more sense to keep them.

> Here another kernel question. Can one idle task be scheduled to
> multiple CPUs. For 4 cores we have 4 separate idle task structs. Are
> they fixed per core or any core can use any of the four idle task
> structs.

AFAIK the idle task is created during boot and is fixed to one core.
I'm not sure how it works with cpu-hotplug, but I'd say that is a
corner case  we don't have to support from the beginning.

Thanks
Philipp

> >  
> > > +
> > > +  /* Update task_struct_ptids map by walking the task_structs starting from
> > > +     init_task.  */
> > > +  bool update_task_struct_ptids ();  
> >
> > There is no definition for this function.  
> Ack.
> >
> > Thanks
> > Philipp
> >  
>