New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

thread pool #102

merged 16 commits into from Nov 13, 2017


None yet
4 participants

akoepsel commented Oct 4, 2017

  • Defines a thread pool containing a configurable amount of threads that keep running throughout the appliation's lifetime
  • Each thread is assigned to one of three tasks: mgmt, i/o or application handler
    • Each crofsock instance uses two I/O threads for RX/TX
    • Each crofconn instance uses one HND thread for running handler methods like handle_packet_in()
    • Each crofchan and crofbase instance uses one MGT thread for background tasks like garbage collection
  • The number of threads started by default is: MGT(1), HND(4), I/O(8)
  • Call rofl::cthread::pool_initialize(...) before instantiating a class deriving from rofl::crofbase to configure thread numbers according to your needs
  • Contrary to the existing scheme used up to v0.12, each thread may be used by multiple instances in parallel, so rofl::cthread's interface has slightly changed: you must specify a rofl::cthread_env* pointer as first argument to methods like add_timer(...), add_read_fd(...), etc.
  • Revised crofconn's finite state machine
  • Skip transmission of Openflow Echo-Request messages when the underlying crofsock instance indicates congestion
  • config/debug.m4 => added -fsanitize=thread to C(XX)FLAGS

@akoepsel akoepsel self-assigned this Oct 4, 2017

@akoepsel akoepsel requested review from toanju and DFritzsche Oct 4, 2017

akoepsel added some commits Oct 4, 2017

Fixing errors in build #330 for bisdn/rofl-common:
* crofconn: use std::atomic<crofconn_mode_t> mode => mode.load() in switch statement to cope with gcc 4.8.4
* crofsock: use TLS_method() only for openssl versions newer than 1.1.0f, use TLSv1_2_method() for older versions
Fixing errors in build #330 for bisdn/rofl-common:
* disabling linking of tsan in debug mode for now, as this fails even with correct flags (-fPIE -pie -shared)
Fixing errors in build #332 for bisdn/rofl-common:
* Remove -fsanitize=thread dependency from CFLAGS in test/rofl/common/crofchantest

besides the minor comments in the beginning I stopped reviewing after running into the asan errors. These should be fixed first.

Show outdated Hide outdated test/rofl/common/crofsock/crofsocktest.cpp
@@ -1 +1 @@

This comment has been minimized.


toanju Oct 12, 2017


I think we should update only when we release


toanju Oct 12, 2017


I think we should update only when we release

Show outdated Hide outdated config/debug.m4

a few comments so far

Show outdated Hide outdated src/rofl/common/crofchan.h
Show outdated Hide outdated src/rofl/common/crofchan.h

akoepsel and others added some commits Nov 1, 2017

* added configure options for google address and thread sanitizers
* moved global OpenSSL initialization and termination to class cthread
* removed state STATE_CLOSED from crofsock
* revised openssl deinitialization for tests with google's address sanitizer
* disabled test crofsocktest::test_tls(), when ASAN is enabled
* seeking a volunteer for doing the bleak work of writing openssl
  version specific deinitialization code for the various openssl
  versions used by various Linux distributions
* make use of --enable-asan in .travis.yml
* add flag -g to config/google_sanitizers.m4 for libasan
* make format
* crofsock: added openssl calls SSL_read/SSL_write to methods send_fr…
…om_queue() and recv_message()

* crofsock: introduced crwlock for openssl using two threads for TX and RX

@akoepsel akoepsel merged commit 0f67ee9 into bisdn:master Nov 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed

@akoepsel akoepsel deleted the akoepsel:thread_pool branch Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment