Re: [PATCH 1/4] cpus: Make {start,end}_exclusive() recursive

[Ilya Leoshkevich]

On Wed, 2023-02-01 at 01:46 +0100, Ilya Leoshkevich wrote:
> Currently dying to one of the core_dump_signal()s deadlocks, because
> dump_core_and_abort() calls start_exclusive() two times: first via
> stop_all_tasks(), and then via preexit_cleanup() ->
> qemu_plugin_user_exit().
> 
> There are a number of ways to solve this: resume after dumping core;
> check cpu_in_exclusive_context() in qemu_plugin_user_exit(); or make
> {start,end}_exclusive() recursive. Pick the last option, since it's
> the most straightforward one.
> 
> Fixes: da91c1920242 ("linux-user: Clean up when exiting due to a
> signal")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Hi,

I noticed that fork()ed CPUs start with in_exclusive_context set (in
this patch it is renamed to exclusive_context_count, but the point
stands). That was not important before, since only pending_cpus decided
what happens in start_exclusive()/end_exclusive(). Now that
exclusive_context_count is also important, we need something like:

--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -161,13 +161,15 @@ void fork_end(int child)
         }
         qemu_init_cpu_list();
         gdbserver_fork(thread_cpu);
-        /* qemu_init_cpu_list() takes care of reinitializing the
-         * exclusive state, so we don't need to end_exclusive() here.
-         */
     } else {
         cpu_list_unlock();
-        end_exclusive();
     }
+    /*
+     * qemu_init_cpu_list() reinitialized the child exclusive state,
but we
+     * also need to keep current_cpu consistent, so call
end_exclusive() for
+     * both child and parent.
+     */
+    end_exclusive();
 }
 
 __thread CPUState *thread_cpu;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1f8c10f8ef9..70fad4bed01 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6776,6 +6776,7 @@ static int do_fork(CPUArchState *env, unsigned
int flags, abi_ulong newsp,
             cpu_clone_regs_parent(env, flags);
             fork_end(0);
         }
+        g_assert(!cpu_in_exclusive_context(cpu));
     }
     return ret;
 }

I can include this in v2, if the overall recursive lock approach is
considered appropriate.

Best regards,
Ilya