cosmetic code changes according to code review

3 files changed, 124 insertions, 80 deletions

diff --git a/src/flow_stat.h b/src/flow_stat.h
index f9761454..3387c2d3 100644
--- a/src/flow_stat.h
+++ b/src/flow_stat.h
@@ -133,6 +133,7 @@ class rfc2544_info_t_ {
         m_seq_err_ev_low = 0;
         m_jitter = 0;
         m_latency = Json::Value("");
+        m_last_max_latency = 0;
     }
 
     inline rfc2544_info_t_ operator+ (const rfc2544_info_t_ &t_in) {
diff --git a/src/stateless/rx/trex_stateless_rx_core.cpp b/src/stateless/rx/trex_stateless_rx_core.cpp
index 6cabc820..f9150ff7 100644
--- a/src/stateless/rx/trex_stateless_rx_core.cpp
+++ b/src/stateless/rx/trex_stateless_rx_core.cpp
@@ -1,3 +1,24 @@
+/*
+  Ido Barnea
+  Cisco Systems, Inc.
+*/
+
+/*
+  Copyright (c) 2016-2016 Cisco Systems, Inc.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+*/
+
 #include <stdio.h>
 #include "bp_sim.h"
 #include "flow_stat_parser.h"
@@ -6,6 +27,39 @@
 #include "trex_stateless_messaging.h"
 #include "trex_stateless_rx_core.h"
 
+void CRFC2544Info::create() {
+    m_latency.Create();
+    // This is the seq num value we expect next packet to have.
+    // Init value should match m_seq_num in CVirtualIFPerSideStats
+    m_seq = UINT32_MAX - 1;  // catch wrap around issues early
+    reset();
+}
+
+void CRFC2544Info::reset() {
+    m_seq_err = 0;
+    m_seq_err_events_too_big = 0;
+    m_seq_err_events_too_low = 0;
+    m_ooo = 0;
+    m_dup = 0;
+    m_latency.Reset();
+    m_jitter.reset();
+}
+
+void CRFC2544Info::export_data(rfc2544_info_t_ &obj) {
+    std::string json_str;
+    Json::Reader reader;
+    Json::Value json;
+
+    obj.set_err_cntrs(m_seq_err, m_ooo, m_dup, m_seq_err_events_too_big, m_seq_err_events_too_low);
+    obj.set_jitter(m_jitter.get_jitter());
+    json_str = "";
+    m_latency.dump_json("", json_str);
+    // This is a hack. We need to make the dump_json return json object.
+    reader.parse( json_str.c_str(), json);
+    obj.set_latency_json(json);
+    obj.set_last_max(m_last_max.getMax());
+};
+
 void CCPortLatencyStl::reset() {
     for (int i = 0; i < MAX_FLOW_STATS; i++) {
         m_rx_pg_stat[i].clear();
@@ -30,16 +84,7 @@ void CRxCoreStateless::create(const CRxSlCfg &cfg) {
     m_cpu_cp_u.Create(&m_cpu_dp_u);
 
     for (int i = 0; i < MAX_FLOW_STATS_PAYLOAD; i++) {
-        // This is the seq num value we expect next packet to have.
-        // Init value should match m_seq_num in CVirtualIFPerSideStats
-        m_rfc2544[i].seq = UINT32_MAX - 1;  // catch wrap around issues early
-        m_rfc2544[i].latency.Create();
-        m_rfc2544[i].jitter.reset();
-        m_rfc2544[i].seq_err = 0;
-        m_rfc2544[i].seq_err_events_too_big = 0;
-        m_rfc2544[i].seq_err_events_too_low = 0;
-        m_rfc2544[i].out_of_order = 0;
-        m_rfc2544[i].dup = 0;
+        m_rfc2544[i].create();
     }
 }
 
@@ -133,60 +178,57 @@ void CRxCoreStateless::handle_rx_pkt(CLatencyManagerPerPortStl *lp, rte_mbuf_t *
             if (is_flow_stat_id(ip_id)) {
                 uint16_t hw_id;
                 if (is_flow_stat_payload_id(ip_id)) {
-                    uint32_t seq;
                     uint8_t *p = rte_pktmbuf_mtod(m, uint8_t*);
                     struct flow_stat_payload_header *fsp_head = (struct flow_stat_payload_header *)
                         (p + m->pkt_len - sizeof(struct flow_stat_payload_header));
                     if (fsp_head->magic == FLOW_STAT_PAYLOAD_MAGIC) {
                         hw_id = fsp_head->hw_id;
-                        seq = fsp_head->seq;
-                        if (unlikely(seq != m_rfc2544[hw_id].seq)) {
-                            if (seq < m_rfc2544[hw_id].seq) {
-                                if (m_rfc2544[hw_id].seq - seq > 100000) {
+                        CRFC2544Info &curr_rfc2544 = m_rfc2544[hw_id];
+                        uint32_t pkt_seq = fsp_head->seq;
+                        uint32_t exp_seq = curr_rfc2544.get_seq();
+                        if (unlikely(pkt_seq != exp_seq)) {
+                            if (pkt_seq < exp_seq) {
+                                if (exp_seq - pkt_seq > 100000) {
                                     // packet loss while we had wrap around
-                                    m_rfc2544[hw_id].seq_err += seq - m_rfc2544[hw_id].seq;
-                                    m_rfc2544[hw_id].seq_err_events_too_big++;
-                                    m_rfc2544[hw_id].seq = seq + 1;
+                                    curr_rfc2544.inc_seq_err(pkt_seq - exp_seq);
+                                    curr_rfc2544.inc_seq_err_too_big();
+                                    curr_rfc2544.set_seq(pkt_seq + 1);
                                 } else {
-                                    if (seq == (m_rfc2544[hw_id].seq - 1)) {
-                                        m_rfc2544[hw_id].dup += 1;
+                                    if (pkt_seq == (exp_seq - 1)) {
+                                        curr_rfc2544.inc_dup();
                                     } else {
-                                         m_rfc2544[hw_id].out_of_order += 1;
+                                        curr_rfc2544.inc_ooo();
                                         // We thought it was lost, but it was just out of order
-                                         if (m_rfc2544[hw_id].seq_err > 0)
-                                             m_rfc2544[hw_id].seq_err -= 1;
+                                        curr_rfc2544.dec_seq_err();
                                     }
-                                    m_rfc2544[hw_id].seq_err_events_too_low++;
+                                    curr_rfc2544.inc_seq_err_too_low();
                                 }
                             } else {
-                                if (unlikely (seq - m_rfc2544[hw_id].seq > 100000)) {
+                                if (unlikely (pkt_seq - exp_seq > 100000)) {
                                     // packet reorder while we had wrap around
-                                    if (seq == (m_rfc2544[hw_id].seq - 1)) {
-                                        m_rfc2544[hw_id].dup += 1;
+                                    if (pkt_seq == (exp_seq - 1)) {
+                                        curr_rfc2544.inc_dup();
                                     } else {
-                                         m_rfc2544[hw_id].out_of_order += 1;
+                                        curr_rfc2544.inc_ooo();
                                         // We thought it was lost, but it was just out of order
-                                         if (m_rfc2544[hw_id].seq_err > 0)
-                                             m_rfc2544[hw_id].seq_err -= 1;
+                                        curr_rfc2544.dec_seq_err();
                                     }
-                                    m_rfc2544[hw_id].seq_err_events_too_low++;
+                                    curr_rfc2544.inc_seq_err_too_low();
                                 } else {
-                                // seq > m_rfc2544[hw_id].seq. Assuming lost packets
-                                    m_rfc2544[hw_id].seq_err += seq - m_rfc2544[hw_id].seq;
-                                    m_rfc2544[hw_id].seq_err_events_too_big++;
-                                    m_rfc2544[hw_id].seq = seq + 1;
+                                // seq > curr_rfc2544.seq. Assuming lost packets
+                                    curr_rfc2544.inc_seq_err(pkt_seq - exp_seq);
+                                    curr_rfc2544.inc_seq_err_too_big();
+                                    curr_rfc2544.set_seq(pkt_seq + 1);
                                 }
                             }
                         } else {
-                            m_rfc2544[hw_id].seq = seq + 1;
+                            curr_rfc2544.set_seq(pkt_seq + 1);
                         }
                         lp->m_port.m_rx_pg_stat_payload[hw_id].add_pkts(1);
                         lp->m_port.m_rx_pg_stat_payload[hw_id].add_bytes(m->pkt_len);
                         uint64_t d = (os_get_hr_tick_64() - fsp_head->time_stamp );
                         dsec_t ctime = ptime_convert_hr_dsec(d);
-                        m_rfc2544[hw_id].latency.Add(ctime);
-                        m_rfc2544[hw_id].last_max.update(ctime);
-                        m_rfc2544[hw_id].jitter.calc(ctime);
+                        curr_rfc2544.add_sample(ctime);
                     }
                 } else {
                     hw_id = get_hw_id(ip_id);
@@ -334,30 +376,13 @@ int CRxCoreStateless::get_rx_stats(uint8_t port_id, rx_per_flow_t *rx_stats, int
 }
 
 int CRxCoreStateless::get_rfc2544_info(rfc2544_info_t *rfc2544_info, int min, int max, bool reset) {
-    std::string json_str;
-    Json::Value json;
-    Json::Reader reader;
-
     for (int hw_id = min; hw_id <= max; hw_id++) {
-        rfc2544_info[hw_id - min].set_err_cntrs(m_rfc2544[hw_id].seq_err, m_rfc2544[hw_id].out_of_order
-                                                , m_rfc2544[hw_id].dup, m_rfc2544[hw_id].seq_err_events_too_big
-                                                , m_rfc2544[hw_id].seq_err_events_too_low);
-        rfc2544_info[hw_id - min].set_jitter(m_rfc2544[hw_id].jitter.get_jitter());
-        m_rfc2544[hw_id].latency.update();
-        json_str = "";
-        m_rfc2544[hw_id].latency.dump_json("", json_str);
-        // This is a hack. We need to make the dump_json return json object.
-        reader.parse( json_str.c_str(), json);
-        rfc2544_info[hw_id - min].set_latency_json(json);
-        rfc2544_info[hw_id - min].set_last_max(m_rfc2544[hw_id].last_max.switchMax());
+        CRFC2544Info &curr_rfc2544 = m_rfc2544[hw_id];
+        curr_rfc2544.sample_period_end();
+        curr_rfc2544.export_data(rfc2544_info[hw_id - min]);
 
         if (reset) {
-            m_rfc2544[hw_id].seq_err = 0;
-            m_rfc2544[hw_id].seq_err_events_too_big = 0;
-            m_rfc2544[hw_id].seq_err_events_too_low = 0;
-            m_rfc2544[hw_id].out_of_order = 0;
-            m_rfc2544[hw_id].latency.Reset();
-            m_rfc2544[hw_id].jitter.reset();
+            curr_rfc2544.reset();
         }
     }
     return 0;
diff --git a/src/stateless/rx/trex_stateless_rx_core.h b/src/stateless/rx/trex_stateless_rx_core.h
index abe8e754..d18356b6 100644
--- a/src/stateless/rx/trex_stateless_rx_core.h
+++ b/src/stateless/rx/trex_stateless_rx_core.h
@@ -52,26 +52,22 @@ class CLastMax {
 
     dsec_t getMax() {
         if (m_choose)
-            return m_max1;
-        else
             return m_max2;
+        else
+            return m_max1;
     }
 
-    dsec_t switchMax() {
-        dsec_t ret;
+    void switchMax() {
         if (m_choose) {
+            m_max2 = 0;
             m_choose = false;
             sanb_smp_memory_barrier();
-            ret = m_max1;
-            m_max1 = 0;
         }
         else {
+            m_max1 = 0;
             m_choose = true;
             sanb_smp_memory_barrier();
-            ret = m_max2;
-            m_max2 = 0;
         }
-        return ret;
     }
 
  private:
@@ -109,16 +105,38 @@ class CRxSlCfg {
     CPortLatencyHWBase * m_ports[TREX_MAX_PORTS];
 };
 
-struct per_flow_rfc2544_info {
-    uint32_t seq; // expected next seq num
-    CTimeHistogram  latency; // latency info
-    CJitter         jitter; 
-    uint64_t seq_err; // How many packet seq num gaps we saw (packets lost or out of order)
-    uint64_t seq_err_events_too_big; // How many packet seq num greater than expected events we had
-    uint64_t seq_err_events_too_low; // How many packet seq num lower than expected events we had
-    uint64_t out_of_order; // Packets we got with seq num lower than expected
-    uint64_t dup; // Packets we got with same seq num
-    CLastMax last_max; // maximum for last measurement period (reset whenever we read it).
+class CRFC2544Info {
+ public:
+    void create();
+    void reset();
+    void export_data(rfc2544_info_t_ &obj);
+    inline void add_sample(double stime) {
+        m_latency.Add(stime);
+        m_last_max.update(stime);
+        m_jitter.calc(stime);
+    }
+    inline void sample_period_end() {
+        m_latency.update();
+        m_last_max.switchMax();
+    }
+    inline uint32_t get_seq() {return m_seq;}
+    inline void set_seq(uint32_t val) {m_seq = val;}
+    inline void inc_seq_err(uint64_t val) {m_seq_err += val;}
+    inline void dec_seq_err() {if (m_seq_err >0) {m_seq_err--;}}
+    inline void inc_seq_err_too_big() {m_seq_err_events_too_big++;}
+    inline void inc_seq_err_too_low() {m_seq_err_events_too_low++;}
+    inline void inc_dup() {m_dup++;}
+    inline void inc_ooo() {m_ooo++;}
+ private:
+    uint32_t m_seq; // expected next seq num
+    CTimeHistogram  m_latency; // latency info
+    CJitter         m_jitter;
+    uint64_t m_seq_err; // How many packet seq num gaps we saw (packets lost or out of order)
+    uint64_t m_seq_err_events_too_big; // How many packet seq num greater than expected events we had
+    uint64_t m_seq_err_events_too_low; // How many packet seq num lower than expected events we had
+    uint64_t m_ooo; // Packets we got with seq num lower than expected (We guess they are out of order)
+    uint64_t m_dup; // Packets we got with same seq num
+    CLastMax m_last_max; // maximum for last measurement period (reset whenever we read it).
 };
 
 class CRxCoreStateless {
@@ -168,6 +186,6 @@ class CRxCoreStateless {
     CCpuUtlCp m_cpu_cp_u;
     // Used for acking "work" (go out of idle) messages from cp
     volatile bool m_ack_start_work_msg __rte_cache_aligned;
-    struct per_flow_rfc2544_info m_rfc2544[MAX_FLOW_STATS_PAYLOAD];
+    CRFC2544Info m_rfc2544[MAX_FLOW_STATS_PAYLOAD];
 };
 #endif