From: Razvan Becheriu Date: Wed, 15 Apr 2020 12:25:07 +0000 (+0300) Subject: [#893] addressed review comments X-Git-Tag: Kea-1.7.7~57 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=349a6aead248cd41889a820d4d13cdcc69be336d;p=thirdparty%2Fkea.git [#893] addressed review comments --- diff --git a/doc/examples/kea4/all-keys.json b/doc/examples/kea4/all-keys.json index 38f394436c..be507dc80c 100644 --- a/doc/examples/kea4/all-keys.json +++ b/doc/examples/kea4/all-keys.json @@ -515,13 +515,13 @@ // information) with each lease. "store-extended-info": true, - // Statistics keep some samples per observation point. - // There are two default values: maximum count and maximum age. - // Set the maximum count to zero disables it. - "statistic-default-sample-count": 0, + // Statistics keep some samples per observation point. + // There are two default values: maximum count and maximum age. + // Set the maximum count to zero disables it. + "statistic-default-sample-count": 0, - // When the maximum count is 0 the maximum age (in seconds) applies. - "statistic-default-sample-age": 60, + // When the maximum count is 0 the maximum age (in seconds) applies. + "statistic-default-sample-age": 60, // Multi-threading parameters. "multi-threading": { @@ -537,9 +537,8 @@ // When multi-threading is enabled, Kea will read packets from the // interface and append a working item to the thread pool. This - // option configures the maximum number of items that can be queued - // for each processing thread. The value must be a positive integer - // (0 means unlimited). + // option configures the maximum number of items that can be queued. + // The value must be a positive integer (0 means unlimited). "packet-queue-size": 0 }, diff --git a/doc/examples/kea6/all-keys.json b/doc/examples/kea6/all-keys.json index a4d6ac35f3..3b68070d7d 100644 --- a/doc/examples/kea6/all-keys.json +++ b/doc/examples/kea6/all-keys.json @@ -456,13 +456,13 @@ // information) with each lease. "store-extended-info": true, - // Statistics keep some samples per observation point. - // There are two default values: maximum count and maximum age. - // Set the maximum count to zero disables it. - "statistic-default-sample-count": 0, + // Statistics keep some samples per observation point. + // There are two default values: maximum count and maximum age. + // Set the maximum count to zero disables it. + "statistic-default-sample-count": 0, - // When the maximum count is 0 the maximum age (in seconds) applies. - "statistic-default-sample-age": 60, + // When the maximum count is 0 the maximum age (in seconds) applies. + "statistic-default-sample-age": 60, // Multi-threading parameters. "multi-threading": { @@ -478,9 +478,8 @@ // When multi-threading is enabled, Kea will read packets from the // interface and append a working item to the thread pool. This - // option configures the maximum number of items that can be queued - // for each processing thread. The value must be a positive integer - // (0 means unlimited). + // option configures the maximum number of items that can be queued. + // The value must be a positive integer (0 means unlimited). "packet-queue-size": 0 }, diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index 8dd405b935..8d06aa97c0 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -3751,11 +3751,11 @@ represented by: - ``thread-pool-size`` - specify the number of threads to process packets in parallel. Supported values are: 0 (auto detect), any positive number sets - number of threads explicitly. + thread count explicitly. -- ``packet-queue-size`` - specify the size of the queue used by each thread to - process packets. Supported values are: 0 (unlimited), any positive number - sets size explicitly. +- ``packet-queue-size`` - specify the size of the queue used by the thread + pool to process packets. Supported values are: 0 (unlimited), any positive + number sets queue size explicitly. An example configuration that sets these parameter looks as follows: diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index 69d34db768..b8d3b7a6d7 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -3263,11 +3263,11 @@ represented by: - ``thread-pool-size`` - specify the number of threads to process packets in parallel. Supported values are: 0 (auto detect), any positive number sets - number of threads explicitly. + thread count explicitly. -- ``packet-queue-size`` - specify the size of the queue used by each thread to - process packets. Supported values are: 0 (unlimited), any positive number - sets size explicitly. +- ``packet-queue-size`` - specify the size of the queue used by the thread + pool to process packets. Supported values are: 0 (unlimited), any positive + number sets queue size explicitly. An example configuration that sets these parameter looks as follows: diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 6ddd9a2873..96fe448410 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -193,7 +193,7 @@ ControlledDhcpv4Srv::loadConfigFile(const std::string& file_name) { LOG_WARN(dhcp4_logger, DHCP4_MULTI_THREADING_INFO) .arg(MultiThreadingMgr::instance().getMode() ? "yes" : "no") .arg(MultiThreadingMgr::instance().getThreadPoolSize()) - .arg(MultiThreadingMgr::instance().getThreadQueueSize()); + .arg(MultiThreadingMgr::instance().getPacketQueueSize()); return (result); } @@ -879,8 +879,7 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { // Configure multi threading try { - CfgMultiThreading::apply(Dhcpv4Srv::srv_thread_count_, - CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()); + CfgMultiThreading::apply(CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()); if (MultiThreadingMgr::instance().getMode()) { LOG_FATAL(dhcp4_logger, DHCP4_MULTI_THREADING_WARNING); } diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 7e2a8b1c48..2faee32d7b 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -588,8 +588,6 @@ void Dhcpv4Exchange::evaluateClasses(const Pkt4Ptr& pkt, bool depend_on_known) { const std::string Dhcpv4Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_"); -int Dhcpv4Srv::srv_thread_count_ = -1; - Dhcpv4Srv::Dhcpv4Srv(uint16_t server_port, uint16_t client_port, const bool use_bcast, const bool direct_response_desired) : io_service_(new IOService()), server_port_(server_port), @@ -942,10 +940,10 @@ Dhcpv4Srv::run_one() { // Do not read more packets from socket if there are enough packets to // be processed in the dhcp thread pool queue // max_queue_size = 0 means no limit - const int max_queue_size = MultiThreadingMgr::instance().getThreadQueueSize(); + const int max_queue_size = MultiThreadingMgr::instance().getPacketQueueSize(); const int thread_count = MultiThreadingMgr::instance().getThreadPoolSize(); size_t pkt_queue_size = MultiThreadingMgr::instance().getThreadPool().count(); - if (thread_count && max_queue_size && (pkt_queue_size >= thread_count * max_queue_size)) { + if (thread_count && max_queue_size && (pkt_queue_size >= max_queue_size)) { read_pkt = false; } diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index b35cf8936f..bc26386a0f 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -1061,11 +1061,6 @@ protected: CBControlDHCPv4Ptr cb_control_; public: - /// @brief command line parameter thread count - /// when parameter is not specified, the default value -1 is used - /// which means disabled (single-threaded), 0 means auto-detect, other - /// values set thread count explicitly. - static int srv_thread_count_; /// Class methods for DHCPv4-over-DHCPv6 handler diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index a503b9cfa1..a234eee0d8 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -403,15 +403,15 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set, continue; } - if (config_pair.first == "dhcp-queue-control") { - DHCPQueueControlParser parser; - srv_cfg->setDHCPQueueControl(parser.parse(config_pair.second)); + if (config_pair.first == "multi-threading") { + MultiThreadingConfigParser parser; + parser.parse(*srv_cfg, config_pair.second); continue; } - if (config_pair.first == "multi-threading") { - MultiThreadingConfigParser parser; - srv_cfg->setDHCPMultiThreading(parser.parse(config_pair.second)); + if (config_pair.first == "dhcp-queue-control") { + DHCPQueueControlParser parser; + srv_cfg->setDHCPQueueControl(parser.parse(config_pair.second)); continue; } diff --git a/src/bin/dhcp4/main.cc b/src/bin/dhcp4/main.cc index 111a4bdab6..98fee8ff34 100644 --- a/src/bin/dhcp4/main.cc +++ b/src/bin/dhcp4/main.cc @@ -48,7 +48,7 @@ usage() { cerr << "Kea DHCPv4 server, version " << VERSION << endl; cerr << endl; cerr << "Usage: " << DHCP4_NAME - << " -[v|V|W] [-d] [-{c|t} cfgfile] [-p number] [-P number] [-N number]" << endl; + << " -[v|V|W] [-d] [-{c|t} cfgfile] [-p number] [-P number]" << endl; cerr << " -v: print version number and exit" << endl; cerr << " -V: print extended version and exit" << endl; cerr << " -W: display the configuration report and exit" << endl; @@ -59,8 +59,6 @@ usage() { << "(useful for testing only)" << endl; cerr << " -P number: specify non-standard client port number 1-65535 " << "(useful for testing only)" << endl; - cerr << " -N number: enable multi-threading and set thread count 0-65535 " - << "(0 means auto detect)" << endl; exit(EXIT_FAILURE); } } // namespace @@ -136,23 +134,6 @@ main(int argc, char* argv[]) { } break; - case 'N': // number of threads - try { - thread_count = boost::lexical_cast(optarg); - } catch (const boost::bad_lexical_cast &) { - cerr << "Failed to parse thread count number: [" << optarg - << "], 0-65535 allowed." << endl; - usage(); - } - if (thread_count < 0 || thread_count > 65535) { - cerr << "Failed to parse thread count number: [" << optarg - << "], 0-65535 allowed." << endl; - usage(); - } else { - Dhcpv4Srv::srv_thread_count_ = thread_count; - } - break; - default: usage(); } diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index b56514b455..bbc3f856b4 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -7159,16 +7159,49 @@ TEST_F(Dhcp4ParserTest, statsDefaultLimits) { EXPECT_EQ("00:00:05", util::durationToText(stats_mgr.getMaxSampleAgeDefault(), 0)); } - -// This test checks that adding multi threadding settings works. + +// This test checks that using default multi threading settings works. +TEST_F(Dhcp4ParserTest, multiThreadingDefaultSettings) { + std::string config = "{ " + genIfaceConfig() + "," + + "\"subnet4\": [ ]" + "}"; + + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP4(config)); + extractConfig(config); + + ConstElementPtr status; + ASSERT_NO_THROW(status = configureDhcp4Server(*srv_, json)); + checkResult(status, 0); + + ConstElementPtr cfg = CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading(); + ASSERT_TRUE(cfg); + + std::string content_json = + "{" + " \"enable-multi-threading\": false,\n" + " \"thread-pool-size\": 0,\n" + " \"packet-queue-size\": 0\n" + "}"; + ConstElementPtr param; + ASSERT_NO_THROW(param = Element::fromJSON(content_json)) + << "invalid context_json, test is broken"; + ASSERT_TRUE(cfg->equals(*param)) + << "expected: " << *(param) << std::endl + << " actual: " << *(cfg) << std::endl; +} + +// This test checks that adding multi threading settings works. TEST_F(Dhcp4ParserTest, multiThreadingSettings) { + std::string content_json = + "{" + " \"enable-multi-threading\": true,\n" + " \"thread-pool-size\": 48,\n" + " \"packet-queue-size\": 1024\n" + "}"; std::string config = "{ " + genIfaceConfig() + "," + "\"subnet4\": [ ], " - "\"multi-threading\": { " - " \"enable-multi-threading\": false," - " \"thread-pool-size\": 0," - " \"packet-queue-size\": 0 }" - "}"; + "\"multi-threading\": " + content_json; ConstElementPtr json; ASSERT_NO_THROW(json = parseDHCP4(config)); @@ -7178,8 +7211,15 @@ TEST_F(Dhcp4ParserTest, multiThreadingSettings) { ASSERT_NO_THROW(status = configureDhcp4Server(*srv_, json)); checkResult(status, 0); - ASSERT_TRUE(CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()); - ASSERT_EQ(CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()->size(), 3); + ConstElementPtr cfg = CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading(); + ASSERT_TRUE(cfg); + + ConstElementPtr param; + ASSERT_NO_THROW(param = Element::fromJSON(content_json)) + << "invalid context_json, test is broken"; + ASSERT_TRUE(cfg->equals(*param)) + << "expected: " << *(param) << std::endl + << " actual: " << *(cfg) << std::endl; } } // namespace diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index 1d875b60c6..eca0012180 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -164,7 +164,7 @@ ControlledDhcpv6Srv::loadConfigFile(const std::string& file_name) { LOG_WARN(dhcp6_logger, DHCP6_MULTI_THREADING_INFO) .arg(MultiThreadingMgr::instance().getMode() ? "yes" : "no") .arg(MultiThreadingMgr::instance().getThreadPoolSize()) - .arg(MultiThreadingMgr::instance().getThreadQueueSize()); + .arg(MultiThreadingMgr::instance().getPacketQueueSize()); return (result); } @@ -203,7 +203,6 @@ void ControlledDhcpv6Srv::cleanup() { ConstElementPtr ControlledDhcpv6Srv::commandShutdownHandler(const string&, ConstElementPtr args) { - if (!ControlledDhcpv6Srv::getInstance()) { LOG_WARN(dhcp6_logger, DHCP6_NOT_RUNNING); return(createAnswer(CONTROL_RESULT_ERROR, "Shutdown failure.")); @@ -901,8 +900,7 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) { // Configure multi threading try { - CfgMultiThreading::apply(Dhcpv6Srv::srv_thread_count_, - CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()); + CfgMultiThreading::apply(CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()); if (MultiThreadingMgr::instance().getMode()) { LOG_FATAL(dhcp6_logger, DHCP6_MULTI_THREADING_WARNING); } diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 538b8e5927..c68f791006 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -204,8 +204,6 @@ namespace dhcp { const std::string Dhcpv6Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_"); -int Dhcpv6Srv::srv_thread_count_ = -1; - Dhcpv6Srv::Dhcpv6Srv(uint16_t server_port, uint16_t client_port) : io_service_(new IOService()), server_port_(server_port), client_port_(client_port), serverid_(), shutdown_(true), @@ -528,10 +526,10 @@ void Dhcpv6Srv::run_one() { // Do not read more packets from socket if there are enough packets to // be processed in the dhcp thread pool queue // max_queue_size = 0 means no limit - const int max_queue_size = MultiThreadingMgr::instance().getThreadQueueSize(); + const int max_queue_size = MultiThreadingMgr::instance().getPacketQueueSize(); const int thread_count = MultiThreadingMgr::instance().getThreadPoolSize(); size_t pkt_queue_size = MultiThreadingMgr::instance().getThreadPool().count(); - if (thread_count && max_queue_size && (pkt_queue_size >= thread_count * max_queue_size)) { + if (thread_count && max_queue_size && (pkt_queue_size >= max_queue_size)) { read_pkt = false; } diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 9c0cbc53f6..b5106cdea8 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -1061,11 +1061,6 @@ protected: uint16_t client_port_; public: - /// @brief command line parameter thread count - /// when parameter is not specified, the default value -1 is used - /// which means disabled (single-threaded), 0 means auto-detect, other - /// values set thread count explicitly. - static int srv_thread_count_; /// @note used by DHCPv4-over-DHCPv6 so must be public and static diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 9802965d06..41cae9920b 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -518,15 +518,15 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set, continue; } - if (config_pair.first == "dhcp-queue-control") { - DHCPQueueControlParser parser; - srv_config->setDHCPQueueControl(parser.parse(config_pair.second)); + if (config_pair.first == "multi-threading") { + MultiThreadingConfigParser parser; + parser.parse(*srv_config, config_pair.second); continue; } - if (config_pair.first == "multi-threading") { - MultiThreadingConfigParser parser; - srv_config->setDHCPMultiThreading(parser.parse(config_pair.second)); + if (config_pair.first == "dhcp-queue-control") { + DHCPQueueControlParser parser; + srv_config->setDHCPQueueControl(parser.parse(config_pair.second)); continue; } diff --git a/src/bin/dhcp6/main.cc b/src/bin/dhcp6/main.cc index e6f8cdb6c8..552b4fa44b 100644 --- a/src/bin/dhcp6/main.cc +++ b/src/bin/dhcp6/main.cc @@ -48,7 +48,7 @@ usage() { cerr << "Kea DHCPv6 server, version " << VERSION << endl; cerr << endl; cerr << "Usage: " << DHCP6_NAME - << " -[v|V|W] [-d] [-{c|t} cfgfile] [-p number] [-P number] [-N number]" << endl; + << " -[v|V|W] [-d] [-{c|t} cfgfile] [-p number] [-P number]" << endl; cerr << " -v: print version number and exit." << endl; cerr << " -V: print extended version and exit" << endl; cerr << " -W: display the configuration report and exit" << endl; @@ -59,8 +59,6 @@ usage() { << "(useful for testing only)" << endl; cerr << " -P number: specify non-standard client port number 1-65535 " << "(useful for testing only)" << endl; - cerr << " -N number: enable multi-threading and set thread count 0-65535 " - << "(0 means auto detect)" << endl; exit(EXIT_FAILURE); } } // namespace @@ -136,23 +134,6 @@ main(int argc, char* argv[]) { } break; - case 'N': // number of threads - try { - thread_count = boost::lexical_cast(optarg); - } catch (const boost::bad_lexical_cast &) { - cerr << "Failed to parse thread count number: [" << optarg - << "], 0-65535 allowed." << endl; - usage(); - } - if (thread_count < 0 || thread_count > 65535) { - cerr << "Failed to parse thread count number: [" << optarg - << "], 0-65535 allowed." << endl; - usage(); - } else { - Dhcpv6Srv::srv_thread_count_ = thread_count; - } - break; - default: usage(); } diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 4e2343093e..217be4b5ec 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -7728,15 +7728,48 @@ TEST_F(Dhcp6ParserTest, statsDefaultLimits) { util::durationToText(stats_mgr.getMaxSampleAgeDefault(), 0)); } -// This test checks that adding multi threadding settings works. +// This test checks that using default multi threading settings works. +TEST_F(Dhcp6ParserTest, multiThreadingDefaultSettings) { + std::string config = "{ " + genIfaceConfig() + "," + + "\"subnet6\": [ ]" + "}"; + + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP6(config)); + extractConfig(config); + + ConstElementPtr status; + ASSERT_NO_THROW(status = configureDhcp6Server(srv_, json)); + checkResult(status, 0); + + ConstElementPtr cfg = CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading(); + ASSERT_TRUE(cfg); + + std::string content_json = + "{" + " \"enable-multi-threading\": false,\n" + " \"thread-pool-size\": 0,\n" + " \"packet-queue-size\": 0\n" + "}"; + ConstElementPtr param; + ASSERT_NO_THROW(param = Element::fromJSON(content_json)) + << "invalid context_json, test is broken"; + ASSERT_TRUE(cfg->equals(*param)) + << "expected: " << *(param) << std::endl + << " actual: " << *(cfg) << std::endl; +} + +// This test checks that adding multi threading settings works. TEST_F(Dhcp6ParserTest, multiThreadingSettings) { + std::string content_json = + "{" + " \"enable-multi-threading\": true,\n" + " \"thread-pool-size\": 48,\n" + " \"packet-queue-size\": 1024\n" + "}"; std::string config = "{ " + genIfaceConfig() + "," + "\"subnet6\": [ ], " - "\"multi-threading\": { " - " \"enable-multi-threading\": false," - " \"thread-pool-size\": 0," - " \"packet-queue-size\": 0 }" - "}"; + "\"multi-threading\": " + content_json; ConstElementPtr json; ASSERT_NO_THROW(json = parseDHCP6(config)); @@ -7746,8 +7779,15 @@ TEST_F(Dhcp6ParserTest, multiThreadingSettings) { ASSERT_NO_THROW(status = configureDhcp6Server(srv_, json)); checkResult(status, 0); - ASSERT_TRUE(CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()); - ASSERT_EQ(CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading()->size(), 3); + ConstElementPtr cfg = CfgMgr::instance().getStagingCfg()->getDHCPMultiThreading(); + ASSERT_TRUE(cfg); + + ConstElementPtr param; + ASSERT_NO_THROW(param = Element::fromJSON(content_json)) + << "invalid context_json, test is broken"; + ASSERT_TRUE(cfg->equals(*param)) + << "expected: " << *(param) << std::endl + << " actual: " << *(cfg) << std::endl; } } // namespace diff --git a/src/lib/dhcpsrv/cfg_multi_threading.cc b/src/lib/dhcpsrv/cfg_multi_threading.cc index 9a13c61885..a9a3c7296f 100644 --- a/src/lib/dhcpsrv/cfg_multi_threading.cc +++ b/src/lib/dhcpsrv/cfg_multi_threading.cc @@ -18,17 +18,11 @@ namespace isc { namespace dhcp { void -CfgMultiThreading::apply(int32_t overwrite, ConstElementPtr value) { - // command line parameters overwrite file and database configuration +CfgMultiThreading::apply(ConstElementPtr value) { bool enabled = false; uint32_t thread_pool_size = 0; uint32_t thread_queue_size = 0; - if (overwrite >= 0) { - enabled = true; - } - if (enabled) { - thread_pool_size = overwrite; - } else { + if (value) { if (value->get("enable-multi-threading")) { enabled = SimpleParser::getBoolean(value, "enable-multi-threading"); } diff --git a/src/lib/dhcpsrv/cfg_multi_threading.h b/src/lib/dhcpsrv/cfg_multi_threading.h index dfae9013da..6e1647252b 100644 --- a/src/lib/dhcpsrv/cfg_multi_threading.h +++ b/src/lib/dhcpsrv/cfg_multi_threading.h @@ -18,9 +18,8 @@ public: /// @brief apply multi threading configuration /// - /// @param overwrite The value used to overwrite configuration /// @param value The multi-threading configuration - static void apply(int32_t overwrite, data::ConstElementPtr value); + static void apply(data::ConstElementPtr value); }; } // namespace dhcp diff --git a/src/lib/dhcpsrv/parsers/multi_threading_config_parser.cc b/src/lib/dhcpsrv/parsers/multi_threading_config_parser.cc index e50fe2fd4b..1869a03dc6 100644 --- a/src/lib/dhcpsrv/parsers/multi_threading_config_parser.cc +++ b/src/lib/dhcpsrv/parsers/multi_threading_config_parser.cc @@ -5,6 +5,7 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include +#include #include #include @@ -13,10 +14,11 @@ using namespace isc::data; namespace isc { namespace dhcp { -ElementPtr -MultiThreadingConfigParser::parse(const ConstElementPtr& value) { +void +MultiThreadingConfigParser::parse(SrvConfig& srv_cfg, + const ConstElementPtr& value) { if (!value) { - return (ElementPtr()); + return; } if (value->getType() != Element::map) { isc_throw(DhcpConfigError, "multi-threading is supposed to be a map"); @@ -59,7 +61,7 @@ MultiThreadingConfigParser::parse(const ConstElementPtr& value) { } } - return (data::copy(value)); + srv_cfg.setDHCPMultiThreading(value); } } // namespace dhcp diff --git a/src/lib/dhcpsrv/parsers/multi_threading_config_parser.h b/src/lib/dhcpsrv/parsers/multi_threading_config_parser.h index a2002be74a..436c5714fa 100644 --- a/src/lib/dhcpsrv/parsers/multi_threading_config_parser.h +++ b/src/lib/dhcpsrv/parsers/multi_threading_config_parser.h @@ -19,8 +19,9 @@ public: /// @brief parses JSON structure /// - /// @param value a JSON map that contains multi-threading parameter. - data::ElementPtr parse(const isc::data::ConstElementPtr& value); + /// @param srv_cfg parsed value will be stored here + /// @param value a JSON map that contains multi-threading parameters. + void parse(SrvConfig& srv_cfg, const isc::data::ConstElementPtr& value); }; } // namespace dhcp diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.cc b/src/lib/dhcpsrv/parsers/simple_parser4.cc index 0539265b41..4a6a3e03a1 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser4.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser4.cc @@ -354,7 +354,7 @@ const SimpleDefaults SimpleParser4::DHCP_QUEUE_CONTROL4_DEFAULTS = { const SimpleDefaults SimpleParser4::DHCP_MULTI_THREADING4_DEFAULTS = { { "enable-multi-threading", Element::boolean, "false" }, { "thread-pool-size", Element::integer, "0" }, - { "packet-queue-size", Element::integer, "4" } + { "packet-queue-size", Element::integer, "64" } }; /// @brief This defines default values for sanity checking for DHCPv4. diff --git a/src/lib/dhcpsrv/parsers/simple_parser6.cc b/src/lib/dhcpsrv/parsers/simple_parser6.cc index 669a492462..e01bbf1a5f 100644 --- a/src/lib/dhcpsrv/parsers/simple_parser6.cc +++ b/src/lib/dhcpsrv/parsers/simple_parser6.cc @@ -366,7 +366,7 @@ const SimpleDefaults SimpleParser6::DHCP_QUEUE_CONTROL6_DEFAULTS = { const SimpleDefaults SimpleParser6::DHCP_MULTI_THREADING6_DEFAULTS = { { "enable-multi-threading", Element::boolean, "false" }, { "thread-pool-size", Element::integer, "0" }, - { "packet-queue-size", Element::integer, "4" } + { "packet-queue-size", Element::integer, "64" } }; /// @brief This defines default values for sanity checking for DHCPv6. diff --git a/src/lib/dhcpsrv/tests/cfg_multi_threading_unittest.cc b/src/lib/dhcpsrv/tests/cfg_multi_threading_unittest.cc index 56541183a3..a9e8c3599c 100644 --- a/src/lib/dhcpsrv/tests/cfg_multi_threading_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_multi_threading_unittest.cc @@ -47,19 +47,20 @@ CfgMultiThreadingTest::TearDown() { TEST_F(CfgMultiThreadingTest, apply) { EXPECT_FALSE(MultiThreadingMgr::instance().getMode()); EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); - ElementPtr param = Element::createMap(); - param->set("enable-multi-threading", Element::create(true)); - param->set("thread-pool-size", Element::create(4)); - param->set("packet-queue-size", Element::create(64)); - CfgMultiThreading::apply(-1, param); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); + std::string content_json = + "{" + " \"enable-multi-threading\": true,\n" + " \"thread-pool-size\": 4,\n" + " \"packet-queue-size\": 64\n" + "}"; + ConstElementPtr param; + ASSERT_NO_THROW(param = Element::fromJSON(content_json)) + << "invalid context_json, test is broken"; + CfgMultiThreading::apply(param); EXPECT_TRUE(MultiThreadingMgr::instance().getMode()); EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 4); - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 64); - CfgMultiThreading::apply(8, param); - EXPECT_TRUE(MultiThreadingMgr::instance().getMode()); - EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 8); - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 64); } } // namespace diff --git a/src/lib/dhcpsrv/tests/multi_threading_config_parser_unittest.cc b/src/lib/dhcpsrv/tests/multi_threading_config_parser_unittest.cc index 82d9b0a7fd..ffc5657922 100644 --- a/src/lib/dhcpsrv/tests/multi_threading_config_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/multi_threading_config_parser_unittest.cc @@ -68,6 +68,7 @@ TEST_F(MultiThreadingConfigParserTest, validContent) { for (auto scenario : scenarios) { SCOPED_TRACE(scenario.description_); { + SrvConfig srv_config; // Construct the config JSON ASSERT_NO_THROW(config_elems = Element::fromJSON(scenario.json_)) << "invalid JSON, test is broken"; @@ -75,17 +76,15 @@ TEST_F(MultiThreadingConfigParserTest, validContent) { // Parsing config should succeed. MultiThreadingConfigParser parser; try { - multi_threading_config = parser.parse(config_elems); + parser.parse(srv_config, config_elems); } catch (const std::exception& ex) { ADD_FAILURE() << "parser threw an exception: " << ex.what(); } + multi_threading_config = srv_config.getDHCPMultiThreading(); // Verify the resultant configuration. ASSERT_TRUE(multi_threading_config); - // The parser should have created a duplicate of the - // configuration elements. - ASSERT_TRUE(multi_threading_config.get() != config_elems.get()); EXPECT_TRUE(multi_threading_config->equals(*config_elems)); } } @@ -150,13 +149,14 @@ TEST_F(MultiThreadingConfigParserTest, invalidContent) { for (auto scenario : scenarios) { SCOPED_TRACE(scenario.description_); { + SrvConfig srv_config; // Construct the config JSON ASSERT_NO_THROW(config_elems = Element::fromJSON(scenario.json_)) << "invalid JSON, test is broken"; // Parsing config into a queue control should succeed. MultiThreadingConfigParser parser; - EXPECT_THROW(parser.parse(config_elems), DhcpConfigError); + EXPECT_THROW(parser.parse(srv_config, config_elems), DhcpConfigError); } } } diff --git a/src/lib/util/multi_threading_mgr.cc b/src/lib/util/multi_threading_mgr.cc index 3362d55f13..0d7135c92a 100644 --- a/src/lib/util/multi_threading_mgr.cc +++ b/src/lib/util/multi_threading_mgr.cc @@ -11,7 +11,7 @@ namespace util { MultiThreadingMgr::MultiThreadingMgr() : enabled_(false), critical_section_count_(0), thread_pool_size_(0), - thread_queue_size_(0) { + packet_queue_size_(0) { } MultiThreadingMgr::~MultiThreadingMgr() { @@ -69,13 +69,13 @@ MultiThreadingMgr::setThreadPoolSize(uint32_t size) { } uint32_t -MultiThreadingMgr::getThreadQueueSize() const { - return (thread_queue_size_); +MultiThreadingMgr::getPacketQueueSize() const { + return (packet_queue_size_); } void -MultiThreadingMgr::setThreadQueueSize(uint32_t size) { - thread_queue_size_ = size; +MultiThreadingMgr::setPacketQueueSize(uint32_t size) { + packet_queue_size_ = size; } uint32_t @@ -103,7 +103,7 @@ MultiThreadingMgr::apply(bool enabled, uint32_t thread_count, uint32_t queue_siz thread_pool_.stop(); } setThreadPoolSize(thread_count); - setThreadQueueSize(queue_size); + setPacketQueueSize(queue_size); setMode(true); if (!isInCriticalSection()) { thread_pool_.start(thread_count); @@ -112,7 +112,7 @@ MultiThreadingMgr::apply(bool enabled, uint32_t thread_count, uint32_t queue_siz thread_pool_.reset(); setMode(false); setThreadPoolSize(thread_count); - setThreadQueueSize(queue_size); + setPacketQueueSize(queue_size); } } diff --git a/src/lib/util/multi_threading_mgr.h b/src/lib/util/multi_threading_mgr.h index 48e0f9b528..fb3dc9c014 100644 --- a/src/lib/util/multi_threading_mgr.h +++ b/src/lib/util/multi_threading_mgr.h @@ -89,28 +89,28 @@ public: /// @brief Get the dhcp thread pool. /// - /// @return The dhcp thread pool of this binary instance. + /// @return The dhcp thread pool. ThreadPool>& getThreadPool(); /// @brief Get the configured dhcp thread pool size. /// - /// @return The dhcp thread pool size of this binary instance. + /// @return The dhcp thread pool size. uint32_t getThreadPoolSize() const; /// @brief Set the configured dhcp thread pool size. /// - /// @param size The dhcp thread pool size of this binary instance. + /// @param size The dhcp thread pool size. void setThreadPoolSize(uint32_t size); - /// @brief Get the configured dhcp thread queue size. + /// @brief Get the configured dhcp packet queue size. /// - /// @return The dhcp thread queue size of this binary instance. - uint32_t getThreadQueueSize() const; + /// @return The dhcp packet queue size. + uint32_t getPacketQueueSize() const; - /// @brief Set the configured dhcp thread queue size. + /// @brief Set the configured dhcp packet queue size. /// - /// @param size The dhcp thread queue size of this binary instance. - void setThreadQueueSize(uint32_t size); + /// @param size The dhcp packet queue size. + void setPacketQueueSize(uint32_t size); /// @brief The system current supported hardware concurrency thread count. /// @@ -166,8 +166,8 @@ private: /// @brief The configured size of the dhcp thread pool. uint32_t thread_pool_size_; - /// @brief The configured size of the dhcp thread queue. - uint32_t thread_queue_size_; + /// @brief The configured size of the dhcp packet queue. + uint32_t packet_queue_size_; /// @brief Packet processing thread pool. ThreadPool> thread_pool_; diff --git a/src/lib/util/tests/multi_threading_mgr_unittest.cc b/src/lib/util/tests/multi_threading_mgr_unittest.cc index 27fe185fe2..fde292cc92 100644 --- a/src/lib/util/tests/multi_threading_mgr_unittest.cc +++ b/src/lib/util/tests/multi_threading_mgr_unittest.cc @@ -46,18 +46,18 @@ TEST(MultiThreadingMgrTest, threadPoolSize) { EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); } -/// @brief Verifies that the thread queue size setter works. -TEST(MultiThreadingMgrTest, threadQueueSize) { +/// @brief Verifies that the packet queue size setter works. +TEST(MultiThreadingMgrTest, packetQueueSize) { // default queue size is 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); // set queue size to 16 - EXPECT_NO_THROW(MultiThreadingMgr::instance().setThreadQueueSize(16)); + EXPECT_NO_THROW(MultiThreadingMgr::instance().setPacketQueueSize(16)); // queue size should be 16 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 16); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 16); // set queue size to 0 - EXPECT_NO_THROW(MultiThreadingMgr::instance().setThreadQueueSize(0)); + EXPECT_NO_THROW(MultiThreadingMgr::instance().setPacketQueueSize(0)); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); } /// @brief Verifies that determining supported thread count works. @@ -89,7 +89,7 @@ TEST(MultiThreadingMgrTest, applyConfig) { // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // thread pool should be started EXPECT_EQ(thread_pool.size(), 16); // disable MT @@ -99,7 +99,7 @@ TEST(MultiThreadingMgrTest, applyConfig) { // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); // thread pool should be stopped EXPECT_EQ(thread_pool.size(), 0); // enable MT with auto scaling @@ -147,7 +147,7 @@ TEST(MultiThreadingMgrTest, criticalSectionFlag) { // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // thread pool should be stopped EXPECT_EQ(thread_pool.size(), 0); // exit critical section @@ -165,7 +165,7 @@ TEST(MultiThreadingMgrTest, criticalSectionFlag) { // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); // thread pool should be stopped EXPECT_EQ(thread_pool.size(), 0); } @@ -183,7 +183,7 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // use scope to test constructor and destructor { MultiThreadingCriticalSection cs; @@ -192,7 +192,7 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // use scope to test constructor and destructor { MultiThreadingCriticalSection inner_cs; @@ -201,21 +201,21 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); } // thread pool should be stopped EXPECT_EQ(thread_pool.size(), 0); // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); } // thread count should match EXPECT_EQ(thread_pool.size(), 16); // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // use scope to test constructor and destructor { MultiThreadingCriticalSection cs; @@ -224,7 +224,7 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 16 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 16); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // apply multi-threading configuration with 64 threads and queue size 4 MultiThreadingMgr::instance().apply(true, 64, 4); // thread pool should be stopped @@ -232,14 +232,14 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 64 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 64); // queue size should be 4 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 4); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 4); } // thread count should match EXPECT_EQ(thread_pool.size(), 64); // thread count should be 64 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 64); // queue size should be 4 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 4); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 4); // use scope to test constructor and destructor { MultiThreadingCriticalSection cs; @@ -248,7 +248,7 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 64 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 64); // queue size should be 4 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 4); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 4); // apply multi-threading configuration with 0 threads MultiThreadingMgr::instance().apply(false, 64, 256); // thread pool should be stopped @@ -256,14 +256,14 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); } // thread count should match EXPECT_EQ(thread_pool.size(), 0); // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); // use scope to test constructor and destructor { MultiThreadingCriticalSection cs; @@ -272,7 +272,7 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); // use scope to test constructor and destructor { MultiThreadingCriticalSection inner_cs; @@ -281,21 +281,21 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); } // thread pool should be stopped EXPECT_EQ(thread_pool.size(), 0); // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); } // thread count should match EXPECT_EQ(thread_pool.size(), 0); // thread count should be 0 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 0); // queue size should be 0 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 0); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 0); // use scope to test constructor and destructor { MultiThreadingCriticalSection cs; @@ -308,14 +308,14 @@ TEST(MultiThreadingMgrTest, criticalSection) { // thread count should be 64 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 64); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); } // thread count should match EXPECT_EQ(thread_pool.size(), 64); // thread count should be 64 EXPECT_EQ(MultiThreadingMgr::instance().getThreadPoolSize(), 64); // queue size should be 256 - EXPECT_EQ(MultiThreadingMgr::instance().getThreadQueueSize(), 256); + EXPECT_EQ(MultiThreadingMgr::instance().getPacketQueueSize(), 256); // apply multi-threading configuration with 0 threads MultiThreadingMgr::instance().apply(false, 0, 0); }