coroutine: Avoid ucontext usage on i386 Linux host
PATCH 1/1 coroutine: Avoid ucontext usage on i386 Linux host
On i386, glibc only saves/restores the signal mask via sigprocmask, excluding RT signal. A Linux bug in the compat version of this syscall corrupts the RT signal state, which will cause lockups of QEMU's VCPU threads. Therefore, fall back to gthread coroutines on this host platform.
---
I'm not sure where to fall back to. The existing code uses gthread, likely because it is the safer harbor. So I picked it as well.
This is also stable material.
configure | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-)
configure +9 -4
@@ -2777,17 +2777,22 @@ fi # windows autodetected by make if test "$coroutine" = "" -o "$coroutine" = "ucontext"; then if test "$darwin" != "yes"; then - cat > $TMPC << EOF + if test "$linux" = "yes" -a "$cpu" = "i386"; then + # RT signal mask corruption for 32-on-64 bit prevents ucontext usage + coroutine_backend=gthread + else + cat > $TMPC << EOF #include <ucontext.h> #ifdef __stub_makecontext #error Ignoring glibc stub makecontext which will always fail #endif int main(void) { makecontext(0, 0, 0); return 0; } EOF - if compile_prog "" "" ; then + if compile_prog "" "" ; then coroutine_backend=ucontext - else - coroutine_backend=gthread + else + coroutine_backend=gthread + fi fi else echo "Silently falling back into gthread backend under darwin"
Signature · 1 line
1.7.3.4
Quoted 4 lines · Jan Kiszka
On i386, glibc only saves/restores the signal mask via sigprocmask, excluding RT signal. A Linux bug in the compat version of this syscall corrupts the RT signal state, which will cause lockups of QEMU's VCPU threads.
This should obviously be fixed in kernel, for benefit of all (not only qemu), do you have any details here?
Quoted 5 lines
Signed-off-by Jan Kiszka <[email protected]>---
I'm not sure where to fall back to. The existing code uses gthread, likely because it is the safer harbor. So I picked it as well.
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Thanks,
/mjt
Quoted 8 lines · Michael Tokarev
Quoted 4 lines · Jan Kiszka
On i386, glibc only saves/restores the signal mask via sigprocmask, excluding RT signal. A Linux bug in the compat version of this syscall corrupts the RT signal state, which will cause lockups of QEMU's VCPU threads.
This should obviously be fixed in kernel, for benefit of all (not only qemu), do you have any details here?
compat_sys_sigprocmask reads 32-bit sigmask from user space, i.e. excluding RT signal, but calls sys_sigprocmask that takes a 64-bit sigset. So the RT signals are unblocked. I'm testing a simple patch ATM, will post it to LKML once this works.
Quoted 7 lines
Quoted 6 lines
Signed-off-by Jan Kiszka <[email protected]>---
I'm not sure where to fall back to. The existing code uses gthread, likely because it is the safer harbor. So I picked it as well.
Quoted 7 lines
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Most conservative is definitely this patch, not switching to SIGUSR1, hoping that no other RT signal user shows up until current kernel are no longer in use.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 30 lines · Jan Kiszka
Quoted 8 lines · Michael Tokarev
Quoted 4 lines · Jan Kiszka
On i386, glibc only saves/restores the signal mask via sigprocmask, excluding RT signal. A Linux bug in the compat version of this syscall corrupts the RT signal state, which will cause lockups of QEMU's VCPU threads.
This should obviously be fixed in kernel, for benefit of all (not only qemu), do you have any details here?
compat_sys_sigprocmaskreads 32-bit sigmask from user space, i.e. excluding RT signal, but callssys_sigprocmaskthat takes a 64-bit sigset. So the RT signals are unblocked. I'm testing a simple patch ATM, will post it to LKML once this works.Quoted 7 lines
Quoted 6 lines
Signed-off-by Jan Kiszka<[email protected]>---
I'm not sure where to fall back to. The existing code uses gthread, likely because it is the safer harbor. So I picked it as well.
Quoted 7 lines
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Quoted 3 lines
Most conservative is definitely this patch, not switching to SIGUSR1, hoping that no other RT signal user shows up until current kernel are no longer in use.
Sorry, how is using a totally different code path more conservative than using a different signal number?
Why would we even use an RT signal in the future?
Regards,
Anthony Liguori
Quoted 3 lines
Jan
Quoted 31 lines · Anthony Liguori
Quoted 30 lines · Jan Kiszka
Quoted 8 lines · Michael Tokarev
Quoted 4 lines · Jan Kiszka
On i386, glibc only saves/restores the signal mask via sigprocmask, excluding RT signal. A Linux bug in the compat version of this syscall corrupts the RT signal state, which will cause lockups of QEMU's VCPU threads.
This should obviously be fixed in kernel, for benefit of all (not only qemu), do you have any details here?
compat_sys_sigprocmaskreads 32-bit sigmask from user space, i.e. excluding RT signal, but callssys_sigprocmaskthat takes a 64-bit sigset. So the RT signals are unblocked. I'm testing a simple patch ATM, will post it to LKML once this works.Quoted 14 lines
Quoted 6 lines
Signed-off-by Jan Kiszka<[email protected]>---
I'm not sure where to fall back to. The existing code uses gthread, likely because it is the safer harbor. So I picked it as well.
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Quoted 6 lines
Quoted 3 lines
Most conservative is definitely this patch, not switching to SIGUSR1, hoping that no other RT signal user shows up until current kernel are no longer in use.
Sorry, how is using a totally different code path more conservative than using a different signal number?
If the gthread version is not safe to use, why do we fall back to it?
Quoted 2 lines
Why would we even use an RT signal in the future?
As both SIGUSR1 and 2 are now in use?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Anthony Liguori
Quoted 8 lines · Jan Kiszka
Quoted 7 lines
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Most conservative is definitely this patch, not switching to SIGUSR1, hoping that no other RT signal user shows up until current kernel are no longer in use.
Sorry, how is using a totally different code path more conservative than using a different signal number?
If the gthread version is not safe to use, why do we fall back to it?
It's safe, but it's significantly slower.
Regards,
Anthony Liguori
Quoted 15 lines · Anthony Liguori
Quoted 14 lines · Jan Kiszka
Quoted 13 lines · Anthony Liguori
Quoted 11 lines · Jan Kiszka
Quoted 7 lines
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Most conservative is definitely this patch, not switching to SIGUSR1, hoping that no other RT signal user shows up until current kernel are no longer in use.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Sorry, how is using a totally different code path more conservative than using a different signal number?
If the gthread version is not safe to use, why do we fall back to it?
It's safe, but it's significantly slower.
OK. Then what about sigaltstack (once fixed)? Is it also slower? If not, can we converge over it? I would really hate staying with this time bomb of broken RT signals unless someone tells me we will kick out all these coroutines rather sooner than later.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 2 lines · Jan Kiszka
OK. Then what about sigaltstack (once fixed)? Is it also slower? If not, can we converge over it?
sigaltstack is going to be significantly faster than the gthread implementation (about the same speed as ucontext for coroutine switch, a bit slower for coroutine creation).
It suffers from the same problem of being a code path which nobody has seriously stress tested, though. And it does at least one thing which is "not permitted by the standards but works anyway".
Quoted 3 lines
I would really hate staying with this time bomb
of broken RT signals unless someone tells me we will kick out all these coroutines rather sooner than later.
I think that regardless of the long term choice it would be a pretty risky decision to switch coroutine implementation this close to release when we have the option of just using a different signal that avoids the bug.
Longer term (ie post 1.1) I'm strongly in favour of kicking out coroutines, because I think there clearly is no single solid portable implementation possible. C just isn't designed to allow them; better not to try to swim against the current.
-- PMM
Quoted 21 lines · Peter Maydell
Quoted 2 lines · Jan Kiszka
OK. Then what about sigaltstack (once fixed)? Is it also slower? If not, can we converge over it?
sigaltstack is going to be significantly faster than the gthread implementation (about the same speed as ucontext for coroutine switch, a bit slower for coroutine creation).
It suffers from the same problem of being a code path which nobody has seriously stress tested, though. And it does at least one thing which is "not permitted by the standards but works anyway".
Quoted 3 lines
I would really hate staying with this time bomb
of broken RT signals unless someone tells me we will kick out all these coroutines rather sooner than later.I think that regardless of the long term choice it would be a pretty risky decision to switch coroutine implementation this close to release when we have the option of just using a different signal that avoids the bug.
Quoted 4 lines
Longer term (ie post 1.1) I'm strongly in favour of kicking out coroutines, because I think there clearly is no single solid portable implementation possible. C just isn't designed to allow them; better not to try to swim against the current.
Unfortunately, voting for code to be different doesn't actually make it different.
If you're volunteering to rewrite the block layer to not require coroutines (either by using a state machine or by using re-entrant threads and fixing any locking issues associated with that) that's wonderful.
But we decided to not do synchronous I/O years ago and still haven't removed it all from the tree. Coroutines got us much closer to getting rid of synchronous I/O.
Regards,
Anthony Liguori
Quoted 3 lines
-- PMM
Quoted 8 lines · Anthony Liguori
Quoted 4 lines · Peter Maydell
Longer term (ie post 1.1) I'm strongly in favour of kicking out coroutines, because I think there clearly is no single solid portable implementation possible. C just isn't designed to allow them; better not to try to swim against the current.
Unfortunately, voting for code to be different doesn't actually make it different.
Yeah, I agree with this sentiment...
Quoted 7 lines
If you're volunteering to rewrite the block layer to not require coroutines (either by using a state machine or by using re-entrant threads and fixing any locking issues associated with that) that's wonderful.
But we decided to not do synchronous I/O years ago and still haven't removed it all from the tree. Coroutines got us much closer to getting rid of synchronous I/O.
...but I would at least like us to take the position that we don't introduce *more* users of coroutines.
-- PMM
Quoted 16 lines · Peter Maydell
Quoted 8 lines · Anthony Liguori
Quoted 4 lines · Peter Maydell
Longer term (ie post 1.1) I'm strongly in favour of kicking out coroutines, because I think there clearly is no single solid portable implementation possible. C just isn't designed to allow them; better not to try to swim against the current.
Unfortunately, voting for code to be different doesn't actually make it different.
Yeah, I agree with this sentiment...
Quoted 4 lines
If you're volunteering to rewrite the block layer to not require coroutines (either by using a state machine or by using re-entrant threads and fixing any locking issues associated with that) that's wonderful.
Quoted 6 lines
Quoted 3 lines
But we decided to not do synchronous I/O years ago and still haven't removed it all from the tree. Coroutines got us much closer to getting rid of synchronous I/O.
...but I would at least like us to take the position that we don't introduce *more* users of coroutines.
I think the long term plan has been:
1) replace synchronous I/O users with coroutines + async I/O
2) promote coroutines to threads by introducing fine grain locking.
I don't think avoiding coroutines helps us along this route nor does it help eliminate immediate users of coroutines.
I think our best strategy forward is to get rid of async I/O in the blocker layer and in devices. Then I think we should promote coroutines as much as possible.
Regards,
Anthony Liguori
Quoted 3 lines
-- PMM
Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Anthony Liguori
Quoted 14 lines · Jan Kiszka
Quoted 13 lines · Anthony Liguori
Quoted 11 lines · Jan Kiszka
Quoted 7 lines
Can't we resort to the SIGUSR1 workaround for the time being, while no RT signals are in actual use, and just have the time to let the kernel side to fix the things up before some actual RTsig user will emerge in qemu? I think it is a bit more conservative approach, especially having in mind the minority of users this issue affects (only 32/64 mixed environment). I'd favor for this variant, and it looks like I'm the "main" 32/64bit user of qemu in this world :)
Most conservative is definitely this patch, not switching to SIGUSR1, hoping that no other RT signal user shows up until current kernel are no longer in use.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Sorry, how is using a totally different code path more conservative than using a different signal number?
If the gthread version is not safe to use, why do we fall back to it?
It's safe, but it's significantly slower.
OK. Then what about sigaltstack (once fixed)? Is it also slower?
I don't know, performance testing would need to be done.
Quoted 4 lines
If not,
can we converge over it? I would really hate staying with this time bomb of broken RT signals unless someone tells me we will kick out all these coroutines rather sooner than later.
AFAICT, neither SIGUSR1 or SIGUSR2 are used today. We only use SIG_IPI today. We could easily #define SIG_IPI to SIGUSR1 unconditionally today. We used to use SIGUSR2 for posix-aio but that was ages ago. AFAICT, it's only used for sigaltstack now.
I don't see where this "time bomb" comes from. I think it's perfect reasonable that if we end up needing more signals (after exhausting SIGUSR1/SIGUSR2) we simply require a fixed kernel for 32bit on 64bit.
Regards,
Anthony Liguori
Quoted 3 lines
Jan
On 09.05.2012 23:21, Jan Kiszka wrote:
[]
configure fi · line 2783
Quoted 11 lines
configure +4
-1@@ -2777,17 +2777,22 @@ fi # windows autodetected by make if test "$coroutine" = "" -o "$coroutine" = "ucontext"; then if test "$darwin" != "yes"; then - cat > $TMPC << EOF + if test "$linux" = "yes" -a "$cpu" = "i386"; then + # RT signal mask corruption for 32-on-64 bit prevents ucontext usage + coroutine_backend=gthread + else
I'd say this is unacceptable. We're making 32bit x86 code to use different implementation of one of core subsystems, and 32bit code already receives much less testing because everyone is using 64bit code and many don't care about 32bits (this is general 32bit case, not a more exotic 32-on-64bit case). I think it is a sure way to have more subtle "works for me" bugs which happens only on 32bits..
Besides, gthread is quite a bit slower than ucontext (but at least it works even in the exotic case).
Thanks,
/mjt