From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 4FKgDtEW0mAtYAAAWB0awg (envelope-from ) for ; Tue, 22 Jun 2021 12:58:57 -0400 Received: by simark.ca (Postfix, from userid 112) id 389361F1F2; Tue, 22 Jun 2021 12:58:57 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id A30461E54D for ; Tue, 22 Jun 2021 12:58:56 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6381C383D032 for ; Tue, 22 Jun 2021 16:58:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6381C383D032 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1624381136; bh=uamuOu+ix1+W9fml0pgcm4ovyLz9uzZa+vJS1ZqL8/U=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=sQYF4ReP7xXLmYQCfogj0ptHaZnBw2u7kjQoyPA8W7bjGnveQL+3S0rR+936WY6Df 3VsThXuPfD44PptsrCam8bJ00dPJ17daq71neOewhY+Gls4VL2PAKlhl0Vc4q8IgNs wGwv0vHqQyqz6iYcvLDJjtSadxSFKg66Y8CZRqPg= Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 653133857001 for ; Tue, 22 Jun 2021 16:57:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 653133857001 X-ASG-Debug-ID: 1624381027-0c856e67e2169e560001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id tG0E1x5hOqZMMvF6 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 22 Jun 2021 12:57:07 -0400 (EDT) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.localdomain (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) by smtp.ebox.ca (Postfix) with ESMTP id B5886441B21; Tue, 22 Jun 2021 12:57:06 -0400 (EDT) X-Barracuda-RBL-IP: 192.222.157.6 X-Barracuda-Effective-Source-IP: 192-222-157-6.qc.cable.ebox.net[192.222.157.6] X-Barracuda-Apparent-Source-IP: 192.222.157.6 To: gdb-patches@sourceware.org Subject: [PATCH 07/11] gdb: maintain per-process-target list of resumed threads with pending wait status Date: Tue, 22 Jun 2021 12:57:00 -0400 X-ASG-Orig-Subj: [PATCH 07/11] gdb: maintain per-process-target list of resumed threads with pending wait status Message-Id: <20210622165704.2404007-8-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20210622165704.2404007-1-simon.marchi@polymtl.ca> References: <20210622165704.2404007-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1624381027 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Barracuda-BRTS-Status: 1 X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 12410 X-Barracuda-Spam-Score: 0.50 X-Barracuda-Spam-Status: No, SCORE=0.50 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_RULE7568M X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.90826 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_RULE7568M Custom Rule 7568M X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Looking up threads that are both resumed and have a pending wait status to report is something that we do quite often in the fast path and is expensive if there are many threads, since it currently requires walking whole thread lists. The first instance is in maybe_set_commit_resumed_all_targets. This is called after handling each event in fetch_inferior_event, to see if we should ask targets to commit their resumed threads or not. If at least one thread is resumed but has a pending wait status, we don't ask the targets to commit their resumed threads, because we want to consume and handle the pending wait status first. The second instance is in random_pending_event_thread, where we want to select a random thread among all those that are resumed and have a pending wait status. This is called every time we try to consume events, to see if there are any pending events that we we want to consume, before asking the targets for more events. To allow optimizing these cases, maintain a per-process-target list of threads that are resumed and have a pending wait status. In maybe_set_commit_resumed_all_targets, we'll be able to check in O(1) if there are any such threads simply by checking whether the list is empty. In random_pending_event_thread, we'll be able to use that list, which will be quicker than iterating the list of threads, especially when there are no resumed with pending wait status threads. About implementation details: using the new setters on class thread_info, it's relatively easy to maintain that list. Any time the "resumed" or "pending wait status" property is changed, we check whether that should cause the thread to be added or removed from the list. In set_thread_exited, we try to remove the thread from the list, because keeping an exited thread in that list would make no sense (especially if the thread is freed). My first implementation assumed that a process stratum target was always present when set_thread_exited is called. That's however, not the case: in some cases, targets unpush themselves from an inferior and then call "exit_inferior", which exits all the threads. If the target is unpushed before set_thread_exited is called on the threads, it means we could mistakenly leave some threads in the list. I tried to see how hard it would be to make it such that targets have to exit all threads before unpushing themselves from the inferior (that would seem logical to me, we don't want threads belonging to an inferior that has no process target). That seem quite difficult and not worth the time. Instead, I changed inferior::unpush_target to remove an threads of that inferior from the list. As of this patch, the list is not used, this is done in the subsequent patches. gdb/ChangeLog: * process-stratum-target.h (class process_stratum_target) : New. : New. * process-stratum-target.c (process_stratum_target::maybe_add_resumed_with_pending_wait_status, process_stratum_target::maybe_remove_resumed_with_pending_wait_status): New. * gdbthread.h (class thread_info) : Remove definition. : New. (thread_info_resumed_with_pending_wait_status_node, thread_info_resumed_with_pending_wait_status_list): New. (set_thread_exited): Call maybe_remove_resumed_with_pending_wait_status. (thread_info::set_resumed): New definition. (thread_info::set_pending_waitstatus): Call maybe_add_resumed_with_pending_wait_status. (thread_info::clear_pending_waitstatus): Call maybe_remove_resumed_with_pending_wait_status. * inferior.h (class inferior) : Remove definition. * inferior.c (inferior::unpush_target): New definition. Change-Id: Iad8f93db2d13984dd5aa5867db940ed1169dbb67 --- gdb/gdbthread.h | 14 ++++++++++++-- gdb/inferior.c | 19 +++++++++++++++++++ gdb/inferior.h | 3 +-- gdb/process-stratum-target.c | 34 ++++++++++++++++++++++++++++++++++ gdb/process-stratum-target.h | 16 ++++++++++++++++ gdb/thread.c | 33 +++++++++++++++++++++++++++++++++ 6 files changed, 115 insertions(+), 4 deletions(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 5ea08a13ee5f..47d7f40eaa08 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -296,8 +296,7 @@ class thread_info : public refcounted_object, bool resumed () const { return m_resumed; } - void set_resumed (bool resumed) - { m_resumed = resumed; } + void set_resumed (bool resumed); /* Frontend view of the thread state. Note that the THREAD_RUNNING/ THREAD_STOPPED states are different from EXECUTING. When the @@ -470,6 +469,10 @@ class thread_info : public refcounted_object, linked. */ intrusive_list_node step_over_list_node; + /* Node for list of threads that are resumed and have a pending wait + status. */ + intrusive_list_node resumed_with_pending_wait_status_node; + /* Displaced-step state for this thread. */ displaced_step_thread_state displaced_step_state; @@ -488,6 +491,13 @@ class thread_info : public refcounted_object, thread_suspend_state m_suspend; }; +using thread_info_resumed_with_pending_wait_status_node + = intrusive_member_node; +using thread_info_resumed_with_pending_wait_status_list + = intrusive_list; + /* A gdb::ref_ptr pointer to a thread_info. */ using thread_info_ref diff --git a/gdb/inferior.c b/gdb/inferior.c index f1b0bdde554b..e07a8f88422a 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -89,6 +89,25 @@ inferior::inferior (int pid_) m_target_stack.push (get_dummy_target ()); } +/* See inferior.h. */ + +int +inferior::unpush_target (struct target_ops *t) +{ + /* If unpushing the process stratum target while threads exists, ensure that + we don't leave any threads of this inferior in the target's "resumed with + pending wait status" list. */ + if (t->stratum () == process_stratum) + { + process_stratum_target *proc_target = as_process_stratum_target (t); + + for (thread_info *thread : this->non_exited_threads ()) + proc_target->maybe_remove_resumed_with_pending_wait_status (thread); + } + + return m_target_stack.unpush (t); +} + void inferior::set_tty (const char *terminal_name) { diff --git a/gdb/inferior.h b/gdb/inferior.h index 830dec3ebbaa..2bfe29afed3f 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -362,8 +362,7 @@ class inferior : public refcounted_object, } /* Unpush T from this inferior's target stack. */ - int unpush_target (struct target_ops *t) - { return m_target_stack.unpush (t); } + int unpush_target (struct target_ops *t); /* Returns true if T is pushed in this inferior's target stack. */ bool target_is_pushed (target_ops *t) diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c index c851090a7f2e..44e138273b2f 100644 --- a/gdb/process-stratum-target.c +++ b/gdb/process-stratum-target.c @@ -106,6 +106,40 @@ process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid, /* See process-stratum-target.h. */ +void +process_stratum_target::maybe_add_resumed_with_pending_wait_status + (thread_info *thread) +{ + gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ()); + + if (thread->resumed () && thread->has_pending_waitstatus ()) + { + infrun_debug_printf ("adding to resumed threads with event list: %s", + target_pid_to_str (thread->ptid).c_str ()); + m_resumed_with_pending_wait_status.push_back (*thread); + } +} + +/* See process-stratum-target.h. */ + +void +process_stratum_target::maybe_remove_resumed_with_pending_wait_status + (thread_info *thread) +{ + if (thread->resumed () && thread->has_pending_waitstatus ()) + { + infrun_debug_printf ("removing from resumed threads with event list: %s", + target_pid_to_str (thread->ptid).c_str ()); + gdb_assert (thread->resumed_with_pending_wait_status_node.is_linked ()); + auto it = m_resumed_with_pending_wait_status.iterator_to (*thread); + m_resumed_with_pending_wait_status.erase (it); + } + else + gdb_assert (!thread->resumed_with_pending_wait_status_node.is_linked ()); +} + +/* See process-stratum-target.h. */ + std::set all_non_exited_process_targets () { diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h index 31a97753db9c..d79efbee8f2c 100644 --- a/gdb/process-stratum-target.h +++ b/gdb/process-stratum-target.h @@ -22,6 +22,8 @@ #include "target.h" #include +#include "gdbsupport/intrusive_list.h" +#include "gdbthread.h" /* Abstract base class inherited by all process_stratum targets. */ @@ -78,6 +80,14 @@ class process_stratum_target : public target_ops may have spawned new threads we haven't heard of yet. */ bool threads_executing = false; + /* If THREAD is resumed and has a pending wait status, add it to the + target's "resumed with pending wait status" list. */ + void maybe_add_resumed_with_pending_wait_status (thread_info *thread); + + /* If THREAD is resumed and has a pending wait status, remove it from the + target's "resumed with pending wait status" list. */ + void maybe_remove_resumed_with_pending_wait_status (thread_info *thread); + /* The connection number. Visible in "info connections". */ int connection_number = 0; @@ -112,6 +122,12 @@ class process_stratum_target : public target_ops coalesce multiple resumption requests in a single vCont packet. */ bool commit_resumed_state = false; + +private: + /* List of threads managed by this target which simultaneously are resumed + and have a pending wait status. */ + thread_info_resumed_with_pending_wait_status_list + m_resumed_with_pending_wait_status; }; /* Downcast TARGET to process_stratum_target. */ diff --git a/gdb/thread.c b/gdb/thread.c index 289d33c74c3b..26974e1b8cbc 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -188,6 +188,10 @@ set_thread_exited (thread_info *tp, bool silent) if (tp->state != THREAD_EXITED) { + process_stratum_target *proc_target = tp->inf->process_target (); + if (proc_target != nullptr) + proc_target->maybe_remove_resumed_with_pending_wait_status (tp); + gdb::observers::thread_exit.notify (tp, silent); /* Tag it as exited. */ @@ -295,6 +299,29 @@ thread_info::deletable () const /* See gdbthread.h. */ +void +thread_info::set_resumed (bool resumed) +{ + if (resumed == m_resumed) + return; + + process_stratum_target *proc_target = this->inf->process_target (); + + /* If we transition from resumed to not resumed, we might need to remove + the thread from the resumed threads with pending statuses list. */ + if (!resumed) + proc_target->maybe_remove_resumed_with_pending_wait_status (this); + + m_resumed = resumed; + + /* If we transition from not resumed to resumed, we might need to add + the thread to the resumed threads with pending statuses list. */ + if (resumed) + proc_target->maybe_add_resumed_with_pending_wait_status (this); +} + +/* See gdbthread.h. */ + void thread_info::set_pending_waitstatus (const target_waitstatus &ws) { @@ -302,6 +329,9 @@ thread_info::set_pending_waitstatus (const target_waitstatus &ws) m_suspend.waitstatus = ws; m_suspend.waitstatus_pending_p = 1; + + process_stratum_target *proc_target = this->inf->process_target (); + proc_target->maybe_add_resumed_with_pending_wait_status (this); } /* See gdbthread.h. */ @@ -311,6 +341,9 @@ thread_info::clear_pending_waitstatus () { gdb_assert (this->has_pending_waitstatus ()); + process_stratum_target *proc_target = this->inf->process_target (); + proc_target->maybe_remove_resumed_with_pending_wait_status (this); + m_suspend.waitstatus_pending_p = 0; } -- 2.32.0