From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93983 invoked by alias); 8 Feb 2019 08:53:36 -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 93975 invoked by uid 89); 8 Feb 2019 08:53:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=PIDs, pids, H*Ad:D*uk, H*Ad:D*co.uk X-HELO: mail-qt1-f196.google.com Received: from mail-qt1-f196.google.com (HELO mail-qt1-f196.google.com) (209.85.160.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Feb 2019 08:53:33 +0000 Received: by mail-qt1-f196.google.com with SMTP id l11so3122762qtp.0 for ; Fri, 08 Feb 2019 00:53:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=YLq8kmOWyvIMoxYdUfM4ciOVXFsVIvaBbd1ejZl8nOc=; b=OrICYobo8CKqgOtv3vYoLLp50ZBh8wtzsL4vVum9Sj6WgqlKvqJvvzPuEcGx5Ct8/v RdKnwGZHdDdAFJWy7VxbL0qxydKaKWLih8g9KdAO9cJcVOkssONkPTg8W0snit2Flkc/ ty1/Efl+jWe1PYU1WXeF72HLmTBCx2Fw02EGMZ9lYT8UlN3rdEyf5TVyCYmR6RBMw6n+ v6AwEoLvhW5lRkN2H68d5wlm1K03m4rOl3JFcdNq+P5Bh2Zt0qZu/mv6hojr6gl7dcgH wTiXV1u3mjht3TRLvkqQmIqPRAnNFI18Fw5h8w8TT6QDp73CDjBrjq1Kcp7z1geZ05QW o0Pg== MIME-Version: 1.0 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> <20190205151531.0df1f11d@laptop-ibm> In-Reply-To: <20190205151531.0df1f11d@laptop-ibm> From: Omair Javaid Date: Fri, 08 Feb 2019 08:53:00 -0000 Message-ID: Subject: Re: [RFC PATCH 4/6] Linux kernel debug infrastructure and target Linux kernel To: Philipp Rudo Cc: GDB Patches , Pedro Alves , Andreas Arnez , Peter Griffin , Ulrich Weigand , Kieran Bingham Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00076.txt.bz2 On Tue, 5 Feb 2019 at 19:15, Philipp Rudo wrote: > > 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. 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 > > > > > >