From cf5f62361682877a08b246a0d04827c5c4cc30f5 Mon Sep 17 00:00:00 2001 From: rbrunner7 Date: Sun, 26 Nov 2017 15:26:17 +0100 Subject: [PATCH] Corrections in rate limiting / trottle code, especially in 'out' direction Deleted 3 out of 4 calls to method connection_basic::sleep_before_packet that were erroneous / superfluous, which enabled the elimination of a "fudge" factor of 2.1 in connection_basic::set_rate_up_limit; also ended the multiplying of limit values and numbers of bytes transferred by 1024 before handing them over to the global throttle objects --- .../epee/include/net/abstract_tcp_server2.inl | 9 ++++---- .../cryptonote_protocol_handler-base.cpp | 2 +- src/p2p/connection_basic.cpp | 13 ++++------- src/p2p/net_node.inl | 22 +++++++++---------- src/p2p/network_throttle-detail.cpp | 8 +------ src/p2p/network_throttle-detail.hpp | 4 +--- src/p2p/network_throttle.hpp | 4 ++-- src/rpc/core_rpc_server.cpp | 4 ++-- 8 files changed, 25 insertions(+), 41 deletions(-) diff --git a/contrib/epee/include/net/abstract_tcp_server2.inl b/contrib/epee/include/net/abstract_tcp_server2.inl index 00d03567c..04d884af2 100644 --- a/contrib/epee/include/net/abstract_tcp_server2.inl +++ b/contrib/epee/include/net/abstract_tcp_server2.inl @@ -286,7 +286,7 @@ PRAGMA_WARNING_DISABLE_VS(4355) { CRITICAL_REGION_LOCAL( epee::net_utils::network_throttle_manager::network_throttle_manager::m_lock_get_global_throttle_in ); - epee::net_utils::network_throttle_manager::network_throttle_manager::get_global_throttle_in().handle_trafic_exact(bytes_transferred * 1024); + epee::net_utils::network_throttle_manager::network_throttle_manager::get_global_throttle_in().handle_trafic_exact(bytes_transferred); } double delay=0; // will be calculated - how much we should sleep to obey speed limit etc @@ -297,7 +297,7 @@ PRAGMA_WARNING_DISABLE_VS(4355) { { //_scope_dbg1("CRITICAL_REGION_LOCAL"); CRITICAL_REGION_LOCAL( epee::net_utils::network_throttle_manager::m_lock_get_global_throttle_in ); - delay = epee::net_utils::network_throttle_manager::get_global_throttle_in().get_sleep_time_after_tick( bytes_transferred ); // decission from global throttle + delay = epee::net_utils::network_throttle_manager::get_global_throttle_in().get_sleep_time_after_tick( bytes_transferred ); } delay *= 0.5; @@ -482,9 +482,7 @@ PRAGMA_WARNING_DISABLE_VS(4355) //some data should be wrote to stream //request complete - if (speed_limit_is_enabled()) { - sleep_before_packet(cb, 1, 1); - } + // No sleeping here; sleeping is done once and for all in "handle_write" m_send_que_lock.lock(); // *** critical *** epee::misc_utils::auto_scope_leave_caller scope_exit_handler = epee::misc_utils::create_scope_leave_handler([&](){m_send_que_lock.unlock();}); @@ -607,6 +605,7 @@ PRAGMA_WARNING_DISABLE_VS(4355) } logger_handle_net_write(cb); + // The single sleeping that is needed for correctly handling "out" speed throttling if (speed_limit_is_enabled()) { sleep_before_packet(cb, 1, 1); } diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp b/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp index 3bda50c22..094e4fc95 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp +++ b/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp @@ -140,7 +140,7 @@ void cryptonote_protocol_handler_base::handler_response_blocks_now(size_t packet { CRITICAL_REGION_LOCAL( network_throttle_manager::m_lock_get_global_throttle_out ); - delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size ); // decission from global + delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size ); } diff --git a/src/p2p/connection_basic.cpp b/src/p2p/connection_basic.cpp index 8edd75b3e..06baa7893 100644 --- a/src/p2p/connection_basic.cpp +++ b/src/p2p/connection_basic.cpp @@ -173,14 +173,9 @@ connection_basic::~connection_basic() noexcept(false) { } void connection_basic::set_rate_up_limit(uint64_t limit) { - - // TODO remove __SCALING_FACTOR... - const double SCALING_FACTOR = 2.1; // to acheve the best performance - limit *= SCALING_FACTOR; { CRITICAL_REGION_LOCAL( network_throttle_manager::m_lock_get_global_throttle_out ); network_throttle_manager::get_global_throttle_out().set_target_speed(limit); - network_throttle_manager::get_global_throttle_out().set_real_target_speed(limit / SCALING_FACTOR); } save_limit_to_file(limit); } @@ -238,7 +233,7 @@ void connection_basic::sleep_before_packet(size_t packet_size, int phase, int q { CRITICAL_REGION_LOCAL( network_throttle_manager::m_lock_get_global_throttle_out ); - delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size ); // decission from global + delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size ); } delay *= 0.50; @@ -252,7 +247,7 @@ void connection_basic::sleep_before_packet(size_t packet_size, int phase, int q // XXX LATER XXX { CRITICAL_REGION_LOCAL( network_throttle_manager::m_lock_get_global_throttle_out ); - network_throttle_manager::get_global_throttle_out().handle_trafic_exact( packet_size * 700); // increase counter - global + network_throttle_manager::get_global_throttle_out().handle_trafic_exact( packet_size ); // increase counter - global } } @@ -262,13 +257,13 @@ void connection_basic::set_start_time() { } void connection_basic::do_send_handler_write(const void* ptr , size_t cb ) { - sleep_before_packet(cb,1,-1); + // No sleeping here; sleeping is done once and for all in connection::handle_write MTRACE("handler_write (direct) - before ASIO write, for packet="<::handle_write MTRACE("handler_write (after write, from queue="< arg_p2p_bind_ip = {"p2p-bind-ip", "Interface for p2p network protocol", "0.0.0.0"}; const command_line::arg_descriptor arg_p2p_bind_port = { "p2p-bind-port" @@ -1844,9 +1844,8 @@ namespace nodetool this->islimitup=false; } - limit *= 1024; epee::net_utils::connection >::set_rate_up_limit( limit ); - MINFO("Set limit-up to " << limit/1024 << " kB/s"); + MINFO("Set limit-up to " << limit << " kB/s"); return true; } @@ -1858,9 +1857,8 @@ namespace nodetool limit=default_limit_down; this->islimitdown=false; } - limit *= 1024; epee::net_utils::connection >::set_rate_down_limit( limit ); - MINFO("Set limit-down to " << limit/1024 << " kB/s"); + MINFO("Set limit-down to " << limit << " kB/s"); return true; } @@ -1872,21 +1870,21 @@ namespace nodetool if(limit == -1) { - limit_up = default_limit_up * 1024; - limit_down = default_limit_down * 1024; + limit_up = default_limit_up; + limit_down = default_limit_down; } else { - limit_up = limit * 1024; - limit_down = limit * 1024; + limit_up = limit; + limit_down = limit; } if(!this->islimitup) { epee::net_utils::connection >::set_rate_up_limit(limit_up); - MINFO("Set limit-up to " << limit_up/1024 << " kB/s"); + MINFO("Set limit-up to " << limit_up << " kB/s"); } if(!this->islimitdown) { epee::net_utils::connection >::set_rate_down_limit(limit_down); - MINFO("Set limit-down to " << limit_down/1024 << " kB/s"); + MINFO("Set limit-down to " << limit_down << " kB/s"); } return true; diff --git a/src/p2p/network_throttle-detail.cpp b/src/p2p/network_throttle-detail.cpp index 1df48ee26..651e01e6b 100644 --- a/src/p2p/network_throttle-detail.cpp +++ b/src/p2p/network_throttle-detail.cpp @@ -160,17 +160,11 @@ void network_throttle::set_target_speed( network_speed_kbps target ) { m_target_speed = target * 1024; MINFO("Setting LIMIT: " << target << " kbps"); - set_real_target_speed(target); -} - -void network_throttle::set_real_target_speed( network_speed_kbps real_target ) -{ - m_real_target_speed = real_target * 1024; } network_speed_kbps network_throttle::get_target_speed() { - return m_real_target_speed / 1024; + return m_target_speed / 1024; } void network_throttle::tick() diff --git a/src/p2p/network_throttle-detail.hpp b/src/p2p/network_throttle-detail.hpp index 27caa85d3..676d4341a 100644 --- a/src/p2p/network_throttle-detail.hpp +++ b/src/p2p/network_throttle-detail.hpp @@ -52,8 +52,7 @@ class network_throttle : public i_network_throttle { }; - network_speed_kbps m_target_speed; - network_speed_kbps m_real_target_speed; + network_speed_bps m_target_speed; size_t m_network_add_cost; // estimated add cost of headers size_t m_network_minimal_segment; // estimated minimal cost of sending 1 byte to round up to size_t m_network_max_segment; // recommended max size of 1 TCP transmission @@ -76,7 +75,6 @@ class network_throttle : public i_network_throttle { virtual ~network_throttle(); virtual void set_name(const std::string &name); virtual void set_target_speed( network_speed_kbps target ); - virtual void set_real_target_speed( network_speed_kbps real_target ); // only for throttle_out virtual network_speed_kbps get_target_speed(); // add information about events: diff --git a/src/p2p/network_throttle.hpp b/src/p2p/network_throttle.hpp index 9853df5e1..bf1f93859 100644 --- a/src/p2p/network_throttle.hpp +++ b/src/p2p/network_throttle.hpp @@ -80,7 +80,8 @@ namespace net_utils { // just typedefs to in code define the units used. TODO later it will be enforced that casts to other numericals are only explicit to avoid mistakes? use boost::chrono? -typedef double network_speed_kbps; +typedef double network_speed_kbps; // externally, for parameters and return values, all defined in kilobytes per second +typedef double network_speed_bps; // throttle-internally, bytes per second typedef double network_time_seconds; typedef double network_MB; @@ -137,7 +138,6 @@ class i_network_throttle { public: virtual void set_name(const std::string &name)=0; virtual void set_target_speed( network_speed_kbps target )=0; - virtual void set_real_target_speed(network_speed_kbps real_target)=0; virtual network_speed_kbps get_target_speed()=0; virtual void handle_trafic_exact(size_t packet_size) =0; // count the new traffic/packet; the size is exact considering all network costs diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp index e9a6a18aa..bcd1d4fc9 100644 --- a/src/rpc/core_rpc_server.cpp +++ b/src/rpc/core_rpc_server.cpp @@ -1547,7 +1547,7 @@ namespace cryptonote res.status = CORE_RPC_ERROR_CODE_WRONG_PARAM; return false; } - epee::net_utils::connection_basic::set_rate_down_limit(nodetool::default_limit_down * 1024); + epee::net_utils::connection_basic::set_rate_down_limit(nodetool::default_limit_down); } if (req.limit_up > 0) @@ -1561,7 +1561,7 @@ namespace cryptonote res.status = CORE_RPC_ERROR_CODE_WRONG_PARAM; return false; } - epee::net_utils::connection_basic::set_rate_up_limit(nodetool::default_limit_up * 1024); + epee::net_utils::connection_basic::set_rate_up_limit(nodetool::default_limit_up); } res.limit_down = epee::net_utils::connection_basic::get_rate_down_limit();