| Message ID | 1512397331-15238-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | linux-user: Fix locking order in fork_start() | expand |
On 04/12/2017 15:22, Peter Maydell wrote: > Our locking order is that the tb lock should be taken > inside the mmap_lock, but fork_start() grabs locks the > other way around. This means that if a heavily multithreaded > guest process (such as Java) calls fork() it can deadlock, > with the thread that called fork() stuck in fork_start() > with the tb lock and waiting for the mmap lock, but some > other thread in tb_find() with the mmap lock and waiting > for the tb lock. The cpu_list_lock() should also always be > taken last, not first. > > Fix this by making fork_start() grab the locks in the > right order. The order in which we drop locks doesn't > matter, so we leave fork_end() the way it is. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-stable@nongnu.org > --- > linux-user/main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index 6286661..146ee3e 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env) > /* Make sure everything is in a consistent state for calling fork(). */ > void fork_start(void) > { > - cpu_list_lock(); > - qemu_mutex_lock(&tb_ctx.tb_lock); > mmap_fork_start(); > + qemu_mutex_lock(&tb_ctx.tb_lock); > + cpu_list_lock(); > } > > void fork_end(int child) > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Peter Maydell <peter.maydell@linaro.org> writes: > Our locking order is that the tb lock should be taken > inside the mmap_lock, but fork_start() grabs locks the > other way around. This means that if a heavily multithreaded > guest process (such as Java) calls fork() it can deadlock, > with the thread that called fork() stuck in fork_start() > with the tb lock and waiting for the mmap lock, but some > other thread in tb_find() with the mmap lock and waiting > for the tb lock. The cpu_list_lock() should also always be > taken last, not first. > > Fix this by making fork_start() grab the locks in the > right order. The order in which we drop locks doesn't > matter, so we leave fork_end() the way it is. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-stable@nongnu.org > --- > linux-user/main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index 6286661..146ee3e 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env) > /* Make sure everything is in a consistent state for calling fork(). */ > void fork_start(void) > { > - cpu_list_lock(); > - qemu_mutex_lock(&tb_ctx.tb_lock); > mmap_fork_start(); > + qemu_mutex_lock(&tb_ctx.tb_lock); > + cpu_list_lock(); > } > > void fork_end(int child) Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Le 04/12/2017 à 15:22, Peter Maydell a écrit : > Our locking order is that the tb lock should be taken > inside the mmap_lock, but fork_start() grabs locks the > other way around. This means that if a heavily multithreaded > guest process (such as Java) calls fork() it can deadlock, > with the thread that called fork() stuck in fork_start() > with the tb lock and waiting for the mmap lock, but some > other thread in tb_find() with the mmap lock and waiting > for the tb lock. The cpu_list_lock() should also always be > taken last, not first. > > Fix this by making fork_start() grab the locks in the > right order. The order in which we drop locks doesn't > matter, so we leave fork_end() the way it is. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-stable@nongnu.org > --- > linux-user/main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index 6286661..146ee3e 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env) > /* Make sure everything is in a consistent state for calling fork(). */ > void fork_start(void) > { > - cpu_list_lock(); > - qemu_mutex_lock(&tb_ctx.tb_lock); > mmap_fork_start(); > + qemu_mutex_lock(&tb_ctx.tb_lock); > + cpu_list_lock(); > } > > void fork_end(int child) > Applied to my linux-user branch. Thanks, Laurent
diff --git a/linux-user/main.c b/linux-user/main.c index 6286661..146ee3e 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -128,9 +128,9 @@ int cpu_get_pic_interrupt(CPUX86State *env) /* Make sure everything is in a consistent state for calling fork(). */ void fork_start(void) { - cpu_list_lock(); - qemu_mutex_lock(&tb_ctx.tb_lock); mmap_fork_start(); + qemu_mutex_lock(&tb_ctx.tb_lock); + cpu_list_lock(); } void fork_end(int child)
Our locking order is that the tb lock should be taken inside the mmap_lock, but fork_start() grabs locks the other way around. This means that if a heavily multithreaded guest process (such as Java) calls fork() it can deadlock, with the thread that called fork() stuck in fork_start() with the tb lock and waiting for the mmap lock, but some other thread in tb_find() with the mmap lock and waiting for the tb lock. The cpu_list_lock() should also always be taken last, not first. Fix this by making fork_start() grab the locks in the right order. The order in which we drop locks doesn't matter, so we leave fork_end() the way it is. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Cc: qemu-stable@nongnu.org --- linux-user/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.7.4