From 76e4ef19366f2b445f829c3279da7a1eacada8a5 Mon Sep 17 00:00:00 2001 From: ashoksingh Date: Mon, 16 Jan 2017 10:20:50 +0530 Subject: [PATCH] Fix flow sampling threshold value from overflowing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. When a flow-sample is getting dropped, reset its diff statistics. The lost statistics will be accounted by normalizing of stats of flows that get exported. This reset will reduce the number flows with high statistics and prevent threshold from overflowing. 2. Change the flow threshold value from u32 to u64 field 3. Also, don’t let u64 threshold value to overflow. If any increment of threshold causes its value to overflow, don’t increment/change the value of threshold Closes-Bug: #1655276 (cherry picked from commit a69c95788f382ab4dae0a630fa51f572cb2f7a7e) Change-Id: If9a50828bdbddcbeb67bd89073bd24b80a00db99 --- .../vrouter/flow_stats/flow_export_info.cc | 9 +++---- .../vrouter/flow_stats/flow_export_info.h | 10 ------- .../vrouter/flow_stats/flow_stats.sandesh | 4 +-- .../flow_stats/flow_stats_collector.cc | 27 ++++++------------- .../vrouter/flow_stats/flow_stats_collector.h | 2 +- .../vrouter/flow_stats/flow_stats_manager.cc | 6 ++++- .../vrouter/flow_stats/flow_stats_manager.h | 6 ++--- 7 files changed, 22 insertions(+), 42 deletions(-) diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_export_info.cc b/src/vnsw/agent/vrouter/flow_stats/flow_export_info.cc index e2f687f75b1..2243c4bf0a1 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_export_info.cc +++ b/src/vnsw/agent/vrouter/flow_stats/flow_export_info.cc @@ -3,16 +3,14 @@ FlowExportInfo::FlowExportInfo() : flow_(), setup_time_(0), teardown_time_(0), last_modified_time_(0), - bytes_(0), packets_(0), prev_diff_bytes_(0), prev_diff_packets_(0), - underlay_source_port_(0), changed_(false), + bytes_(0), packets_(0), underlay_source_port_(0), changed_(false), tcp_flags_(0), delete_enqueue_time_(0), evict_enqueue_time_(0), exported_atleast_once_(false) { } FlowExportInfo::FlowExportInfo(const FlowEntryPtr &fe) : flow_(fe), setup_time_(0), teardown_time_(0), last_modified_time_(0), - bytes_(0), packets_(0), prev_diff_bytes_(0), prev_diff_packets_(0), - underlay_source_port_(0), changed_(true), + bytes_(0), packets_(0), underlay_source_port_(0), changed_(true), tcp_flags_(0), delete_enqueue_time_(0), evict_enqueue_time_(0), exported_atleast_once_(false) { } @@ -20,8 +18,7 @@ FlowExportInfo::FlowExportInfo(const FlowEntryPtr &fe) : FlowExportInfo::FlowExportInfo(const FlowEntryPtr &fe, uint64_t setup_time) : flow_(fe), setup_time_(setup_time), teardown_time_(0), last_modified_time_(setup_time), - bytes_(0), packets_(0), prev_diff_bytes_(0), prev_diff_packets_(0), - underlay_source_port_(0), changed_(true), + bytes_(0), packets_(0), underlay_source_port_(0), changed_(true), tcp_flags_(0), delete_enqueue_time_(0), evict_enqueue_time_(0), exported_atleast_once_(false) { } diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_export_info.h b/src/vnsw/agent/vrouter/flow_stats/flow_export_info.h index 349b7bc86a2..e2e321501dd 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_export_info.h +++ b/src/vnsw/agent/vrouter/flow_stats/flow_export_info.h @@ -27,10 +27,6 @@ class FlowExportInfo { void set_bytes(uint64_t value) { bytes_ = value; } uint64_t packets() const { return packets_; } void set_packets(uint64_t value) { packets_ = value; } - uint64_t prev_diff_bytes() const { return prev_diff_bytes_; } - void set_prev_diff_bytes(uint64_t value) { prev_diff_bytes_ = value; } - uint64_t prev_diff_packets() const { return prev_diff_packets_; } - void set_prev_diff_packets(uint64_t value) { prev_diff_packets_ = value; } uint16_t underlay_source_port() const { return underlay_source_port_; } void set_underlay_source_port(uint16_t port) { underlay_source_port_ = port; @@ -61,12 +57,6 @@ class FlowExportInfo { uint64_t last_modified_time_; //used for aging uint64_t bytes_; uint64_t packets_; - /* When flow samples are dropped the diff stats for that sample is - * accumulated in the following fields. This used to compute aggregate diff - * when the flow is being sent again. On successful send the following - * fields are reset */ - uint64_t prev_diff_bytes_; - uint64_t prev_diff_packets_; //IP address of the src vrouter for egress flows and dst vrouter for //ingress flows. Used only during flow-export //Underlay IP protocol type. Used only during flow-export diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_stats.sandesh b/src/vnsw/agent/vrouter/flow_stats/flow_stats.sandesh index ddca7a26c46..3f99388051c 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_stats.sandesh +++ b/src/vnsw/agent/vrouter/flow_stats/flow_stats.sandesh @@ -177,6 +177,6 @@ request sandesh DeleteAgingConfig { traceobject sandesh FlowExportStatsTrace { 1: u32 export_rate; 2: u32 export_rate_without_sampling; - 3: u32 old_threshold; - 4: u32 new_threshold; + 3: u64 old_threshold; + 4: u64 new_threshold; } diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.cc b/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.cc index 1863a92138a..2311d3d91dd 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.cc +++ b/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.cc @@ -769,11 +769,6 @@ void FlowStatsCollector::ExportFlow(FlowExportInfo *info, return; } - /* Compute diff stats by adding the previous diff stats of sample that - * was dropped */ - diff_bytes += info->prev_diff_bytes(); - diff_pkts += info->prev_diff_packets(); - /* Subject a flow to sampling algorithm only when all of below is met:- * a. if Log is not configured as action for flow * b. actual flow-export-rate is >= 80% of configured flow-export-rate. This @@ -801,8 +796,6 @@ void FlowStatsCollector::ExportFlow(FlowExportInfo *info, /* Do not export the flow, if the random number generated is more * than the diff_bytes */ flow_stats_manager_->flow_export_sampling_drops_++; - info->set_prev_diff_bytes(diff_bytes); - info->set_prev_diff_packets(diff_pkts); /* The second part of the if condition below is not required but * added for better readability. It is not required because * exported_atleast_once() will always be false if teardown time is @@ -822,10 +815,6 @@ void FlowStatsCollector::ExportFlow(FlowExportInfo *info, diff_pkts = diff_pkts/probability; } } - /* Reset diff stats since flow will be exported now */ - info->set_prev_diff_bytes(0); - info->set_prev_diff_packets(0); - /* Mark the flow as exported */ info->set_exported_atleast_once(true); @@ -964,7 +953,7 @@ bool FlowStatsManager::UpdateFlowThreshold() { (cfg_rate == prev_cfg_flow_export_rate_)) { return true; } - uint32_t cur_t = threshold(), new_t = 0; + uint64_t cur_t = threshold(), new_t = 0; // Update sampling threshold based on flow_export_rate_ if (flow_export_rate_ < ((double)cfg_rate) * 0.8) { /* There are two reasons why we can be here. @@ -975,20 +964,20 @@ bool FlowStatsManager::UpdateFlowThreshold() { * Threshold should be updated here depending on which of the above two * situations we are in. */ if (!flows_sampled_atleast_once_) { - UpdateThreshold(kDefaultFlowSamplingThreshold); + UpdateThreshold(kDefaultFlowSamplingThreshold, false); } else { if (flow_export_rate_ < ((double)cfg_rate) * 0.5) { - UpdateThreshold((threshold_ / 4)); + UpdateThreshold((threshold_ / 4), false); } else { - UpdateThreshold((threshold_ / 2)); + UpdateThreshold((threshold_ / 2), false); } } } else if (flow_export_rate_ > (cfg_rate * 3)) { - UpdateThreshold((threshold_ * 4)); + UpdateThreshold((threshold_ * 4), true); } else if (flow_export_rate_ > (cfg_rate * 2)) { - UpdateThreshold((threshold_ * 3)); + UpdateThreshold((threshold_ * 3), true); } else if (flow_export_rate_ > ((double)cfg_rate) * 1.25) { - UpdateThreshold((threshold_ * 2)); + UpdateThreshold((threshold_ * 2), true); } prev_cfg_flow_export_rate_ = cfg_rate; new_t = threshold(); @@ -997,7 +986,7 @@ bool FlowStatsManager::UpdateFlowThreshold() { return true; } -uint32_t FlowStatsCollector::threshold() const { +uint64_t FlowStatsCollector::threshold() const { return flow_stats_manager_->threshold(); } diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h b/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h index 5eeb9c9ba66..ec68e588af3 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h +++ b/src/vnsw/agent/vrouter/flow_stats/flow_stats_collector.h @@ -103,7 +103,7 @@ class FlowStatsCollector : public StatsCollector { void set_flow_tcp_syn_age_time(uint64_t interval) { flow_tcp_syn_age_time_ = interval; } - uint32_t threshold() const; + uint64_t threshold() const; boost::uuids::uuid rand_gen(); bool Run(); bool RunAgeingTask(); diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.cc b/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.cc index cebf80a6e70..eef55c68af8 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.cc +++ b/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.cc @@ -34,7 +34,11 @@ SandeshTraceBufferPtr FlowExportStatsTraceBuf(SandeshTraceBufferCreate( "FlowExportStats", 3000)); const uint8_t FlowStatsManager::kCatchAllProto; -void FlowStatsManager::UpdateThreshold(uint32_t new_value) { +void FlowStatsManager::UpdateThreshold(uint64_t new_value, bool check_oflow) { + if (check_oflow && new_value < threshold_) { + /* Retain the same value for threshold if it results in overflow */ + return; + } if (new_value < kMinFlowSamplingThreshold) { threshold_ = kMinFlowSamplingThreshold; } else { diff --git a/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.h b/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.h index a5744cb498a..c635c0da00b 100644 --- a/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.h +++ b/src/vnsw/agent/vrouter/flow_stats/flow_stats_manager.h @@ -149,7 +149,7 @@ class FlowStatsManager { return flow_export_drops_; } - uint32_t threshold() const { return threshold_;} + uint64_t threshold() const { return threshold_;} bool delete_short_flow() const { return delete_short_flow_; } @@ -173,7 +173,7 @@ class FlowStatsManager { friend class FlowStatsCollectorReq; friend class FlowStatsCollector; bool UpdateFlowThreshold(void); - void UpdateThreshold(uint32_t new_value); + void UpdateThreshold(uint64_t new_value, bool check_oflow); Agent *agent_; WorkQueue > request_queue_; FlowAgingTableMap flow_aging_table_map_; @@ -181,7 +181,7 @@ class FlowStatsManager { tbb::atomic flow_export_count_; uint64_t prev_flow_export_rate_compute_time_; uint32_t flow_export_rate_; - uint32_t threshold_; + uint64_t threshold_; tbb::atomic flow_export_disable_drops_; tbb::atomic flow_export_sampling_drops_; tbb::atomic flow_export_without_sampling_;