Re: [RFC v3 3/8] Add basic Linux kernel support

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

Hi Omair,

and now the third mail.

On Wed, 3 May 2017 19:12:36 +0500
Omair Javaid <omair.javaid@linaro.org> wrote:

> On 24 April 2017 at 20:24, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
> > On Thu, Apr 20 2017, Omair Javaid wrote:
> >  
> >> Hi Philipp and Andreas,
> >>
> >> I have some further comments on this patch specifically about copying
> >> task_struct->pid into ptid->lwp and using task_struct address as tid.
> >>
> >> I see that we are overriding lwp, tid which any target beneath might
> >> be using differently.
> >>
> >> So suggestion about storing task_struct->pid or task_struct address is
> >> to use private_thread_info in binutils-gdb/gdb/gdbthread.h for this
> >> information.  
> >
> > The current version of the patch series is mainly focused on dump
> > targets.  Remote targets require some additional changes.  We've
> > discussed the use of private_thread_info before, and the last I've seen
> > is that it is not suitable either, because remote.c uses it already:
> >
> >   https://sourceware.org/ml/gdb-patches/2017-02/msg00543.html
> >
> > In my view, the private_thread_info field really is a hack, and we are
> > now facing its limits.  It provides some space for a single thread layer
> > to store information into, but not for multiple such layers.  In the
> > case of the Linux kernel we at least have two different thread layers:
> > the CPU layer (each "thread" is a CPU), and the kernel task layer on top
> > of that.
> >
> > I think we need to allow a target to maintain its *own* thread list.
> > The CPU "thread list" would be maintained by the target beneath
> > (remote/dump), and the kernel task list would be maintained by the LK
> > target.  The ptid namespaces could be completely separate.  
> 
> Hi Philip and Andreas,
> 
> Further more on this topic, remote stub assigns a common pid to all
> CPU threads and uses LWP as CPU number.
> While tid is left zero. I think we ll have to rework the old to new
> ptid mapping mechanism a little bit in order to adjust all types of
> targets beneath.

As mentioned in the mail before, the mapping between old and new ptid is a
hack.  Feel free to change it.  Although I think that the only proper solution
is to allow every target to manage its own thread_list.

> In your implementation of lk_init_ptid_map() you are testing pid from
> task_struct against lwp of set by target target beneath. In case of
> remote this will never be equal to pid as it is marked as the cpu
> number.

For dumps it depends on what was written in the dump.  Some architectures
write there the pid some the cpu (e.g. s390).  Thats why I made it possible
for the architecture to overwrite the default behavior.

I think the cleanest solution is to classify lk_private and make the hooks
virtual class methods (as Yao suggested).  What do you think?
 
> Also in your implementation of lk_update_running_tasks lwp is being
> updated with pid read from task_struct and tid is the task struct
> address.
> We are doing this not only for linux thread layer tasks but also for
> CPU tasks in lk_update_running_tasks. This causes some sync issues
> while on live targets as every time we halt a new thread is reported
> by remote.
> 
> I think linux thread layer should only update tasks it has created
> itself i-e tasks created by parsing task_struct. We can devise a
> mechanism to map CPU tasks to curr task_struct and leave CPU tasks as
> they were created by target beneath with same ptids.
> 
> Whats your take on this?

I'm against this.  When we only update the threads we created ourself there
will be multiple problems.  For example you will see running tasks twice with
"info thread", once its task_struct version and once its remote version.
Furthermore there will be no mapping between both versions.  Thus the
task_struct version will show the outdated state when the task was last
"unscheduled".  While the remote version shows the tasks actual state but has
no information about kernel internals, e.g. the linux pid.  This would be
extremely confusing!

In addition there is a chance that you have multiple threads with the same
ptid, e.g. cpu1 colliding with the init process with pid = 1.  GDB is not able
to handle two threads with the same ptid.  This could (most likely) be
prevented by using the task_struct address as tid.  But there is nothing
preventing the target beneath to do the same.

Long story short.  GDBs current thread implementation is a hack with
limitations we are now reaching.  The only clean solution is for every target
to have its own thread_list.

Philipp

> >> I also have reservation about use of old_ptid naming in struct
> >> lk_private and struct lk_ptid_map.
> >>
> >> old_ptid naming is a little confusing kindly choose a distinguishable
> >> name for old_ptid varibles in both lk_private and lk_ptid_map.
> >>
> >> Further Here's an implementation of bitmap_weight function from linux
> >> kernel. Kindly see if your implementation can be improved and moved to
> >> a generic area in gdb.
> >>
> >>  10 int __bitmap_weight(const unsigned long *bitmap, int bits)
> >>  11 {
> >>  12         int k, w = 0, lim = bits/BITS_PER_LONG;
> >>  13
> >>  14         for (k = 0; k < lim; k++)
> >>  15                 w += hweight_long(bitmap[k]);
> >>  16
> >>  17         if (bits % BITS_PER_LONG)
> >>  18                 w += hweight_long(bitmap[k] &
> >> BITMAP_LAST_WORD_MASK(bits)); 19
> >>  20         return w;
> >>  21 }  
> >
> > The __bitmap_weight function is specific to Linux, so I'm not sure we
> > want to move it to a generic area.  For big-endian targets the function
> > depends on the width of Linux' "unsigned long" type, because
> > BITMAP_LAST_WORD_MASK builds a mask for the *least significant* bits
> > instead of the *lowest-addressed* ones.
> >
> > It's probably true that the performance of lk_bitmap_hweight could be
> > improved.  For instance, with some care a function like popcount_hwi()
> > in GCC's hwint.h could be exploited, even if the target's word width and
> > byte order may not match the GDB client's.  This would not make the
> > function simpler, though.
> >
> > --
> > Andreas
> >  
>