From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88976 invoked by alias); 5 Feb 2019 14:15:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 88967 invoked by uid 89); 5 Feb 2019 14:15:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=Furthermore, forgotten X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 Feb 2019 14:15:41 +0000 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x15EFN6A086529 for ; Tue, 5 Feb 2019 09:15:38 -0500 Received: from e06smtp07.uk.ibm.com (e06smtp07.uk.ibm.com [195.75.94.103]) by mx0b-001b2d01.pphosted.com with ESMTP id 2qfax2uunc-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 05 Feb 2019 09:15:37 -0500 Received: from localhost by e06smtp07.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 5 Feb 2019 14:15:36 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195) by e06smtp07.uk.ibm.com (192.168.101.137) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Tue, 5 Feb 2019 14:15:33 -0000 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id x15EFWKC48169194 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 5 Feb 2019 14:15:32 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 943B711C04A; Tue, 5 Feb 2019 14:15:32 +0000 (GMT) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 51F0C11C052; Tue, 5 Feb 2019 14:15:32 +0000 (GMT) Received: from laptop-ibm (unknown [9.152.212.204]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Tue, 5 Feb 2019 14:15:32 +0000 (GMT) Date: Tue, 05 Feb 2019 14:15:00 -0000 From: Philipp Rudo To: Omair Javaid Cc: GDB Patches , Pedro Alves , Andreas Arnez , Peter Griffin , Ulrich Weigand , Kieran Bingham Subject: Re: [RFC PATCH 4/6] Linux kernel debug infrastructure and target Linux kernel In-Reply-To: References: <1548738199-9403-1-git-send-email-omair.javaid@linaro.org> <1548738199-9403-5-git-send-email-omair.javaid@linaro.org> <20190131171417.7329cf5e@laptop-ibm> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit x-cbid: 19020514-0028-0000-0000-000003447746 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19020514-0029-0000-0000-000024028081 Message-Id: <20190205151531.0df1f11d@laptop-ibm> X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00021.txt.bz2 Hi Omair, On Mon, 4 Feb 2019 14:35:26 +0500 Omair Javaid wrote: > Hi Philipp, > > Thanks for your review. My comments inline. > > On Thu, 31 Jan 2019 at 21:14, Philipp Rudo wrote: > > > > Hi Omair, > > > > On Tue, 29 Jan 2019 10:03:17 +0500 > > Omair Javaid 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 > > >