(RFC) remove the GThread coroutine implementation

PATCH Applied Daniel P. Berrange 5 participants 8 messages Apr 28, 2017 – May 3, 2017

Share & tools

Daniel P. Berrange

author

2017-04-28 12:25 UTC #0 lore raw

PATCH 1/1 (RFC) remove the GThread coroutine implementation

At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.

I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......

Daniel P. Berrange (1):
coroutine: remove GThread implementation

 .travis.yml              |   5 +-
 configure                |  19 ++---
 util/coroutine-gthread.c | 198 -----------------------------------------------
 3 files changed, 6 insertions(+), 216 deletions(-)
 delete mode 100644 util/coroutine-gthread.c

Signature · 1 line

2.9.3

Daniel P. Berrange

author

2017-04-28 12:25 UTC #1 lore raw

PATCH 1/1 coroutine: remove GThread implementation

The GThread implementation is not functional enough to actually run QEMU reliably. While it was potentially useful for debugging, we have a scripts/qemugdb/coroutine.py to enable tracing of ucontext coroutines in GDB, so that removes the only reason for GThread to exist.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 .travis.yml              |   5 +-
 configure                |  19 ++---
 util/coroutine-gthread.c | 198 -----------------------------------------------
 3 files changed, 6 insertions(+), 216 deletions(-)
 delete mode 100644 util/coroutine-gthread.c

.travis.yml +1 -4

index 9008a79..27a2d9c 100644
@@ -86,9 +86,6 @@ matrix:
     - env: CONFIG="--enable-trace-backends=ust"
            TEST_CMD=""
       compiler: gcc
-    - env: CONFIG="--with-coroutine=gthread"
-           TEST_CMD=""
-      compiler: gcc
     - env: CONFIG=""
       os: osx
       compiler: clang

@@ -191,7 +188,7 @@ matrix:
       compiler: none
       env:
         - COMPILER_NAME=gcc CXX=g++-5 CC=gcc-5
-        - CONFIG="--cc=gcc-5 --cxx=g++-5 --disable-pie --disable-linux-user --with-coroutine=gthread"
+        - CONFIG="--cc=gcc-5 --cxx=g++-5 --disable-pie --disable-linux-user"
         - TEST_CMD=""
       before_script:
         - ./configure ${CONFIG} --extra-cflags="-g3 -O0 -fsanitize=thread -fuse-ld=gold" || cat config.log

configure +5 ~~-14~~

index c35acf1..9da5c34 100755
@@ -1334,7 +1334,7 @@ Advanced options (experts only):
   --oss-lib                path to OSS library
   --cpu=CPU                Build for host CPU [$cpu]
   --with-coroutine=BACKEND coroutine backend. Supported options:
-                           gthread, ucontext, sigaltstack, windows
+                           ucontext, sigaltstack, windows
   --enable-gcov            enable test coverage analysis with gcov
   --gcov=GCOV              use specified gcov [$gcov_tool]
   --disable-blobs          disable installing provided firmware blobs

@@ -4418,10 +4418,8 @@ fi
 # check and set a backend for coroutine
 
 # We prefer ucontext, but it's not always possible. The fallback
-# is sigcontext. gthread is not selectable except explicitly, because
-# it is not functional enough to run QEMU proper. (It is occasionally
-# useful for debugging purposes.)  On Windows the only valid backend
-# is the Windows-specific one.
+# is sigcontext. On Windows the only valid backend is the Windows
+# specific one.
 
 ucontext_works=no
 if test "$darwin" != "yes"; then

@@ -4460,7 +4458,7 @@ else
       feature_not_found "ucontext"
     fi
     ;;
-  gthread|sigaltstack)
+  sigaltstack)
     if test "$mingw32" = "yes"; then
       error_exit "only the 'windows' coroutine backend is valid for Windows"
     fi

@@ -4472,14 +4470,7 @@ else
 fi
 
 if test "$coroutine_pool" = ""; then
-  if test "$coroutine" = "gthread"; then
-    coroutine_pool=no
-  else
-    coroutine_pool=yes
-  fi
-fi
-if test "$coroutine" = "gthread" -a "$coroutine_pool" = "yes"; then
-  error_exit "'gthread' coroutine backend does not support pool (use --disable-coroutine-pool)"
+  coroutine_pool=yes
 fi
 
 if test "$debug_stack_usage" = "yes"; then

util/coroutine-gthread.c deleted +0 ~~-198~~

198 lines not on this page · raw message

Signature · 1 line

2.9.3

Stefan Hajnoczi

Reviewed-by on v1 1/1

2017-04-28 15:39 UTC #2 lore raw

In reply to Daniel P. Berrange

Quoted 8 lines · Daniel P. Berrange

At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.
I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......

Quoted 8 lines

Daniel P. Berrange (1):
coroutine: remove GThread implementation

 .travis.yml              |   5 +-
 configure                |  19 ++---
 util/coroutine-gthread.c | 198 -----------------------------------------------
 3 files changed, 6 insertions(+), 216 deletions(-)
 delete mode 100644 util/coroutine-gthread.c

Happy to merge this but waiting in case anyone wants to discuss it.

Reviewed-by Stefan Hajnoczi <[email protected]>

Richard Henderson

2017-04-29 09:28 UTC #3 lore raw

In reply to Daniel P. Berrange

Quoted 7 lines · Daniel P. Berrange

At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.
I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......

The last time this was mentioned, the reason that we were keeping it was to make clang's thread-sanitizer module happy. Whether we can still find relevant bugs with that, I don't know.

Alex Bennée

2017-04-30 06:20 UTC #4 lore raw

Quoted 5 lines · Richard Henderson

Quoted 4 lines · Daniel P. Berrange
At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.

Quoted 7 lines

Quoted 3 lines
I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......
The last time this was mentioned, the reason that we were keeping it was to make clang's thread-sanitizer module happy. Whether we can still find relevant bugs with that, I don't know.

It's been a while since I last did a ThreadSanitizer run. The correct fix is teaching the sanitizer about set context so we can use our normal build - however this has been at the bottom of a pile for such a long time.

--
Alex Bennée

Stefan Hajnoczi

2017-05-02 14:09 UTC #5 lore raw

Quoted 12 lines · Alex Bennée

Quoted 9 lines · Richard Henderson
Quoted 7 lines · Daniel P. Berrange
At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.
I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......

Quoted 8 lines

Quoted 3 lines
The last time this was mentioned, the reason that we were keeping it was to make clang's thread-sanitizer module happy. Whether we can still find relevant bugs with that, I don't know.
It's been a while since I last did a ThreadSanitizer run. The correct fix is teaching the sanitizer about set context so we can use our normal build - however this has been at the bottom of a pile for such a long time.

Any objections to merging this patch?

Stefan

Alex Bennée

Acked-by

2017-05-02 14:35 UTC #6 lore raw

Quoted 17 lines · Stefan Hajnoczi

Quoted 16 lines · Alex Bennée
Quoted 12 lines · Richard Henderson
Quoted 7 lines · Daniel P. Berrange
At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.
I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......
The last time this was mentioned, the reason that we were keeping it was to make clang's thread-sanitizer module happy. Whether we can still find relevant bugs with that, I don't know.

Quoted 6 lines

Quoted 4 lines
It's been a while since I last did a ThreadSanitizer run. The correct fix is teaching the sanitizer about set context so we can use our normal build - however this has been at the bottom of a pile for such a long time.
Any objections to merging this patch?

I shall not stand in its way ;-)

Acked-by Alex Bennée <[email protected]>

--
Alex Bennée

Stefan Hajnoczi

Applied

2017-05-03 14:03 UTC #7 lore raw

In reply to Daniel P. Berrange

Quoted 8 lines · Daniel P. Berrange

At the 2016 summit it was suggested that we delete the GThread coroutine impl since it is not fully functional, and you can debug the ucontext impl with our GDB helper script.
I don't recall the subject being raised again since the summit so here's a proposal to delete the GThread impl, as a way to trigger input from anyone who thinks we need to keep it......

Quoted 12 lines

Daniel P. Berrange (1):
coroutine: remove GThread implementation

 .travis.yml              |   5 +-
 configure                |  19 ++---
 util/coroutine-gthread.c | 198 -----------------------------------------------
 3 files changed, 6 insertions(+), 216 deletions(-)
 delete mode 100644 util/coroutine-gthread.c

Signature · 1 line

2.9.3

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan