Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
On Mon, 2024-02-19 at 14:05 +0100, Ilya Leoshkevich wrote:
> On Sat, 2024-02-17 at 10:21 -1000, Richard Henderson wrote:
> > On 2/16/24 03:05, Ilya Leoshkevich wrote:
> > > The upcoming follow-fork-mode child support will require
> > > disabling
> > > gdbstub in the parent process, which may have multiple threads
> > > (which
> > > are represented as CPUs).
> > >
> > > Loop over all CPUs in order to remove breakpoints and disable
> > > single-step. Move the respective code into a separate function.
> > >
> > > Signed-off-by: Ilya Leoshkevich
> > > ---
> > > gdbstub/user.c | 19 +++
> > > 1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > > index 14918d1a217..e17f7ece908 100644
> > > --- a/gdbstub/user.c
> > > +++ b/gdbstub/user.c
> > > @@ -356,16 +356,27 @@ int gdbserver_start(const char
> > > *port_or_path)
> > > return -1;
> > > }
> > >
> > > +static void disable_gdbstub(void)
> > > +{
> > > + CPUState *cpu;
> > > +
> > > + close(gdbserver_user_state.fd);
> > > + gdbserver_user_state.fd = -1;
> > > + CPU_FOREACH(cpu) {
> > > + cpu_breakpoint_remove_all(cpu, BP_GDB);
> > > + /* no cpu_watchpoint_remove_all for user-mode */
> > > + cpu_single_step(cpu, 0);
> > > + tb_flush(cpu);
> >
> > You only need to flush once. The cpu argument is used to determine
> > if we can perform the
> > flush immediately or need to queue it.
>
> I thought we needed to flush jump caches on all CPUs, but I see now
> that do_tb_flush() already does this, so this loop is unnecessarily
> quadratic.
>
> Btw, shouldn't do_tb_flush() have cpu as a local variable, and not as
> a parameter?
Never mind, the dummy parameter is needed for usage with
async_safe_run_on_cpu().
[...]
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
On Sat, 2024-02-17 at 10:21 -1000, Richard Henderson wrote:
> On 2/16/24 03:05, Ilya Leoshkevich wrote:
> > The upcoming follow-fork-mode child support will require disabling
> > gdbstub in the parent process, which may have multiple threads
> > (which
> > are represented as CPUs).
> >
> > Loop over all CPUs in order to remove breakpoints and disable
> > single-step. Move the respective code into a separate function.
> >
> > Signed-off-by: Ilya Leoshkevich
> > ---
> > gdbstub/user.c | 19 +++
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 14918d1a217..e17f7ece908 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -356,16 +356,27 @@ int gdbserver_start(const char *port_or_path)
> > return -1;
> > }
> >
> > +static void disable_gdbstub(void)
> > +{
> > + CPUState *cpu;
> > +
> > + close(gdbserver_user_state.fd);
> > + gdbserver_user_state.fd = -1;
> > + CPU_FOREACH(cpu) {
> > + cpu_breakpoint_remove_all(cpu, BP_GDB);
> > + /* no cpu_watchpoint_remove_all for user-mode */
> > + cpu_single_step(cpu, 0);
> > + tb_flush(cpu);
>
> You only need to flush once. The cpu argument is used to determine
> if we can perform the
> flush immediately or need to queue it.
I thought we needed to flush jump caches on all CPUs, but I see now
that do_tb_flush() already does this, so this loop is unnecessarily
quadratic.
Btw, shouldn't do_tb_flush() have cpu as a local variable, and not as
a parameter?
>
> Otherwise,
> Reviewed-by: Richard Henderson
>
>
> r~
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
On 2/16/24 03:05, Ilya Leoshkevich wrote:
The upcoming follow-fork-mode child support will require disabling
gdbstub in the parent process, which may have multiple threads (which
are represented as CPUs).
Loop over all CPUs in order to remove breakpoints and disable
single-step. Move the respective code into a separate function.
Signed-off-by: Ilya Leoshkevich
---
gdbstub/user.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 14918d1a217..e17f7ece908 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -356,16 +356,27 @@ int gdbserver_start(const char *port_or_path)
return -1;
}
+static void disable_gdbstub(void)
+{
+CPUState *cpu;
+
+close(gdbserver_user_state.fd);
+gdbserver_user_state.fd = -1;
+CPU_FOREACH(cpu) {
+cpu_breakpoint_remove_all(cpu, BP_GDB);
+/* no cpu_watchpoint_remove_all for user-mode */
+cpu_single_step(cpu, 0);
+tb_flush(cpu);
You only need to flush once. The cpu argument is used to determine if we can perform the
flush immediately or need to queue it.
Otherwise,
Reviewed-by: Richard Henderson
r~
Re: [PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
Ilya Leoshkevich writes: > The upcoming follow-fork-mode child support will require disabling > gdbstub in the parent process, which may have multiple threads (which > are represented as CPUs). > > Loop over all CPUs in order to remove breakpoints and disable > single-step. Move the respective code into a separate function. > > Signed-off-by: Ilya Leoshkevich Reviewed-by: Alex Bennée -- Alex Bennée Virtualisation Tech Lead @ Linaro
[PATCH v3 01/11] gdbstub: Support disablement in a multi-threaded process
The upcoming follow-fork-mode child support will require disabling
gdbstub in the parent process, which may have multiple threads (which
are represented as CPUs).
Loop over all CPUs in order to remove breakpoints and disable
single-step. Move the respective code into a separate function.
Signed-off-by: Ilya Leoshkevich
---
gdbstub/user.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 14918d1a217..e17f7ece908 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -356,16 +356,27 @@ int gdbserver_start(const char *port_or_path)
return -1;
}
+static void disable_gdbstub(void)
+{
+CPUState *cpu;
+
+close(gdbserver_user_state.fd);
+gdbserver_user_state.fd = -1;
+CPU_FOREACH(cpu) {
+cpu_breakpoint_remove_all(cpu, BP_GDB);
+/* no cpu_watchpoint_remove_all for user-mode */
+cpu_single_step(cpu, 0);
+tb_flush(cpu);
+}
+}
+
/* Disable gdb stub for child processes. */
void gdbserver_fork(CPUState *cpu)
{
if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
return;
}
-close(gdbserver_user_state.fd);
-gdbserver_user_state.fd = -1;
-cpu_breakpoint_remove_all(cpu, BP_GDB);
-/* no cpu_watchpoint_remove_all for user-mode */
+disable_gdbstub();
}
/*
--
2.43.0
