[RFC PATCH 4/6] Linux kernel debug infrastructure and target Linux kernel
Omair Javaid
omair.javaid@linaro.org
Fri Feb 8 08:53:00 GMT 2019
On Tue, 5 Feb 2019 at 19:15, Philipp Rudo <prudo@linux.ibm.com> wrote:
>
> 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.
There are two reasons I am using idle tasks:
1) in case a cpu is not running idle we need to know that cpu's idle
task struct address to avoid creating a new gdb thread when a task
starts running on a cpu i simply replace the gdb thread with task
struct address of the idle task.
2) to identify cpu number as all cpu's have their unique idle tasks.
Although i eventually didnt use it because i was not sure if this can
serve as the unique id.
>
> > 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
> > >
> >
>
More information about the Gdb-patches
mailing list