[PATCH v1] gdb: Enable early init of thread pool size

[PATCH v1] gdb: Enable early init of thread pool size

[Richard Bunt]

This commit enables the early initialization commands (92e4e97a9f5) to
modify the number of threads used by gdb's thread pool.

The motivation here is to prevent gdb from spawning a detrimental number
of threads on many-core systems under environments with restrictive
ulimits.

With gdb before this commit, the thread pool takes the following sizes:

1. Thread pool size is initialized to 0.

2. After the maintenance commands are defined, the thread pool size is
set to the number of system cores (if it has not already been set).

3. Using early initialization commands, the thread pool size can be
changed using "maint set worker-threads".

4. After the first prompt, the thread pool size can be changed as in the
previous step.

Therefore after step 2. gdb has potentially launched hundreds of threads
on a many-core system.

After this change, step 2 and 3 are reversed so there is an opportunity
to set the required number of threads without needing to default to the
number of system cores first.

There does exist a configure option (added in 261b07488b9) to disable
multithreading, but this does not allow for an already deployed gdb to
be configured.

In order to implement a test case, a bug fix was needed to enable
set/show commands to output information at early initialization time.
The early initiailzation commands are run before any interpreters are
available and printing resulted in a segmentation fault. The fix causes
gdb_stdout to be used directly. A side effect of this is that, any
output from set/show commands called at early intialization time will
not be formatted by an interpreter. In the same way the version string
resultant from the following command will not be decorated with MI
stream records.

    gdb -i=mi --version

GDB built with GCC 13.

No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
LLVM, NVHPC; Platforms: x86_64, aarch64.

The scenario that interests me the most involves preventing GDB from
spawning any worker threads at all. This was tested by counting the
number of clones observed by strace:

    strace -e clone,clone3 gdb/gdb -q \
    --early-init-eval-command="maint set worker-threads 0" \
    -ex q ./gdb/gdb |& grep --count clone

---
 gdb/cli/cli-setshow.c                      |  2 +-
 gdb/main.c                                 |  4 +++
 gdb/maint.c                                |  4 +--
 gdb/maint.h                                |  2 ++
 gdb/testsuite/gdb.base/early-init-file.exp | 40 ++++++++++++++++++++++
 gdbsupport/thread-pool.cc                  |  4 +++
 gdbsupport/thread-pool.h                   |  1 +
 7 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index c7bbac1666d..7bc919e7b20 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -661,7 +661,7 @@ do_show_command (const char *arg, int from_tty, struct cmd_list_element *c)
   /* FIXME: cagney/2005-02-10: There should be MI and CLI specific
      versions of code to print the value out.  */
 
-  if (uiout->is_mi_like_p ())
+  if (uiout && uiout->is_mi_like_p ())
     uiout->field_string ("value", val);
   else
     {
diff --git a/gdb/main.c b/gdb/main.c
index 2da39f89a90..6930c5e1549 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -1047,6 +1047,10 @@ captured_main_1 (struct captured_main_args *context)
   execute_cmdargs (&cmdarg_vec, CMDARG_EARLYINIT_FILE,
 		   CMDARG_EARLYINIT_COMMAND, &ret);
 
+  /* Set the thread pool size here, so the size can be influenced by the
+     early initialization commands.  */
+  update_thread_pool_size ();
+
   /* Initialize the extension languages.  */
   ext_lang_initialization ();
 
diff --git a/gdb/maint.c b/gdb/maint.c
index f91184c2005..b608161982f 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -845,7 +845,7 @@ maintenance_set_profile_cmd (const char *args, int from_tty,
 static int n_worker_threads = -1;
 
 /* Update the thread pool for the desired number of threads.  */
-static void
+void
 update_thread_pool_size ()
 {
 #if CXX_STD_THREAD
@@ -1474,6 +1474,4 @@ such as demangling symbol names."),
 					     maintenance_selftest_option_defs,
 					     &set_selftest_cmdlist,
 					     &show_selftest_cmdlist);
-
-  update_thread_pool_size ();
 }
diff --git a/gdb/maint.h b/gdb/maint.h
index 1741d132567..ede93882aa7 100644
--- a/gdb/maint.h
+++ b/gdb/maint.h
@@ -26,6 +26,8 @@ extern void set_per_command_time (int);
 
 extern void set_per_command_space (int);
 
+extern void update_thread_pool_size ();
+
 /* Records a run time and space usage to be used as a base for
    reporting elapsed time or change in space.  */
 
diff --git a/gdb/testsuite/gdb.base/early-init-file.exp b/gdb/testsuite/gdb.base/early-init-file.exp
index 6426da8dacf..996d8d38b04 100644
--- a/gdb/testsuite/gdb.base/early-init-file.exp
+++ b/gdb/testsuite/gdb.base/early-init-file.exp
@@ -99,6 +99,25 @@ proc check_gdb_startups_up_quietly { message } {
     }
 }
 
+# Restart GDB and check that the size of the thread pool has not been
+# adjusted to match the number of machine cores at early init time.
+proc check_gdb_maint_show { message } {
+    global gdb_prompt
+    global gdb_sanitizer_msg_re
+    gdb_exit
+    gdb_spawn
+    set setshowprefix "The number of worker threads GDB can use is"
+    set unset "$setshowprefix unlimited \\\(currently 0\\\)."
+    set final "$setshowprefix 1."
+    # Output when CXX_STD_THREAD is undefined.
+    set off "$setshowprefix 0."
+    gdb_test_multiple "" $message {
+	-re "^(${gdb_sanitizer_msg_re})?($unset|$off)\r\n($final|$off)\r\n$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
 with_ansi_styling_terminal {
 
     # Start GDB and confirm that the version string is styled.
@@ -164,4 +183,25 @@ with_ansi_styling_terminal {
 	check_gdb_startups_up_quietly \
 	    "check GDB starts quietly using XDG_CONFIG_HOME"
     }
+
+    # Create fake home directory for the thread pool size check.
+    set dirs [setup_home_directories "maint-show" \
+		  [multi_line_input \
+		       "set startup-quietly on" \
+		       "maint show worker-threads" \
+		       "maint set worker-threads 1" \
+		       "maint show worker-threads"]]
+
+    set home_dir [lindex $dirs 0]
+
+    # Now arrange to use the fake home directory startup file.
+    save_vars { INTERNAL_GDBFLAGS env(HOME) env(XDG_CONFIG_HOME) } {
+	set INTERNAL_GDBFLAGS [string map {"-nx" "" "-q" ""} $INTERNAL_GDBFLAGS]
+
+	# Now test GDB when using the HOME directory.
+	set env(HOME) $home_dir
+	unset -nocomplain env(XDG_CONFIG_HOME)
+	check_gdb_maint_show \
+	    "check early init of thread pool size"
+    }
 }
diff --git a/gdbsupport/thread-pool.cc b/gdbsupport/thread-pool.cc
index 1c871ed378f..c743e9896bf 100644
--- a/gdbsupport/thread-pool.cc
+++ b/gdbsupport/thread-pool.cc
@@ -154,6 +154,7 @@ thread_pool::set_thread_count (size_t num_threads)
 {
 #if CXX_STD_THREAD
   std::lock_guard<std::mutex> guard (m_tasks_mutex);
+  sized_at_least_once = true;
 
   /* If the new size is larger, start some new threads.  */
   if (m_thread_count < num_threads)
@@ -197,6 +198,9 @@ thread_pool::set_thread_count (size_t num_threads)
 void
 thread_pool::do_post_task (std::packaged_task<void ()> &&func)
 {
+  /* This assert is here to check that no tasks are posted to the pool between
+     its initialization and sizing.  */
+  gdb_assert (sized_at_least_once);
   std::packaged_task<void ()> t (std::move (func));
 
   if (m_thread_count != 0)
diff --git a/gdbsupport/thread-pool.h b/gdbsupport/thread-pool.h
index cb8696e1fa4..b4bdf5b5a76 100644
--- a/gdbsupport/thread-pool.h
+++ b/gdbsupport/thread-pool.h
@@ -204,6 +204,7 @@ class thread_pool
      between the main thread and the worker threads.  */
   std::condition_variable m_tasks_cv;
   std::mutex m_tasks_mutex;
+  bool sized_at_least_once = false;
 #endif /* CXX_STD_THREAD */
 };
 
-- 
2.32.0

Re: [PATCH v1] gdb: Enable early init of thread pool size

[Tom Tromey]

>>>>> "Richard" == Richard Bunt <richard.bunt@linaro.org> writes:

Richard> This commit enables the early initialization commands (92e4e97a9f5) to
Richard> modify the number of threads used by gdb's thread pool.

Thank you for the patch.

Richard> The motivation here is to prevent gdb from spawning a detrimental number
Richard> of threads on many-core systems under environments with restrictive
Richard> ulimits.

Is there a way to detect this situation & automatically adapt?  That
might be nice to do, if possible.  Or perhaps we should cap the number
of worker threads.  In my testing I couldn't get DWARF reading to scale
past about N=8 anyway, see

    https://sourceware.org/bugzilla/show_bug.cgi?id=29959

I have a couple nits about the patch, nothing very serious though.

Richard> diff --git a/gdb/maint.h b/gdb/maint.h
Richard> index 1741d132567..ede93882aa7 100644
Richard> --- a/gdb/maint.h
Richard> +++ b/gdb/maint.h
Richard> @@ -26,6 +26,8 @@ extern void set_per_command_time (int);
 
Richard>  extern void set_per_command_space (int);
 
Richard> +extern void update_thread_pool_size ();

Normally gdb puts an explanatory comment in the header and then a
reference to that comment by the implementation.

Richard> +  bool sized_at_least_once = false;

This should be called 'm_sized_at_least_once'.

Tom

Re: [PATCH v1] gdb: Enable early init of thread pool size

[Richard Bunt]

On 10/10/2023 18:15, Tom Tromey wrote:
>>>>>> "Richard" == Richard Bunt <richard.bunt@linaro.org> writes:
> 
> Richard> This commit enables the early initialization commands (92e4e97a9f5) to
> Richard> modify the number of threads used by gdb's thread pool.
> 
> Thank you for the patch.

Thanks for the review.

> 
> Richard> The motivation here is to prevent gdb from spawning a detrimental number
> Richard> of threads on many-core systems under environments with restrictive
> Richard> ulimits.
> 
> Is there a way to detect this situation & automatically adapt?  That
> might be nice to do, if possible.  Or perhaps we should cap the number
> of worker threads.  In my testing I couldn't get DWARF reading to scale
> past about N=8 anyway, see
> 

In my experience with this type of optimisation, the set of good N 
values need to be determined empirically from a given environment, 
platform and target program. Which led me to giving the user full 
control over the number of worker threads in use, for the entire 
lifetime of the thread pool. This way they can decide what's best for 
their situation. This patch is the product of this line of thought.

As far as default values go, I'd vote for the current default being the 
most widely applicable. Based on your data, an abundance of threads 
doesn't degrade performance, and there are worker threads available to 
service applications which have more CUs than GDB. Whereas using a soft 
limit, of say 8, might only be useful for the gdb debugging gdb case on 
that specific platform.

Specifically on the ulimit scenario. Assuming GDB had the ulimit data, I 
do not believe it would be able to make decision on thread pool size, 
without also knowing the resources the debug target was going to consume 
throughout its run.

I honestly cannot see a way to adapt to this situation or derive a 
better initial N.

>      https://sourceware.org/bugzilla/show_bug.cgi?id=29959
> 

Thank you for sharing those performance numbers. My stance for my use 
case until now was to eliminate all worker threads, so that they do not 
disrupt the target program. However, limiting to one or two threads 
might be good balance between performance and risking a ulimit breach.

> I have a couple nits about the patch, nothing very serious though.
> 

I will address the nits in a v2.

> Richard> diff --git a/gdb/maint.h b/gdb/maint.h
> Richard> index 1741d132567..ede93882aa7 100644
> Richard> --- a/gdb/maint.h
> Richard> +++ b/gdb/maint.h
> Richard> @@ -26,6 +26,8 @@ extern void set_per_command_time (int);
>   
> Richard>  extern void set_per_command_space (int);
>   
> Richard> +extern void update_thread_pool_size ();
> 
> Normally gdb puts an explanatory comment in the header and then a
> reference to that comment by the implementation.
> 
> Richard> +  bool sized_at_least_once = false;
> 
> This should be called 'm_sized_at_least_once'.
> 
> Tom

Many thanks,

Rich.