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

[Omair Javaid <omair dot javaid at linaro dot org>]

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.

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.

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 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
>