Fix accounting of linklocal flows

The flow-accouting routines do not update linklocal flow counters in flow-prot if the vm_ in VmFlowRef is NULL. This results in inconsitent linklocal flow counter at flow-proto level. Modified flow accouting with following, - Update flow-proto counters irrespective of vm_ value in VmFlowRef - Add/Delete entries into LinkLocalFlowInfoMap from VmFlowRef module - Add UT for linklocal flow eviction (cherry picked from commit fca09d0497e39931d30665da76063eb23c521ca5) Fixes-Bug: #1549617 Change-Id: I125347fdaed57cad0687b94c803a060e492165b1
Juniper · Mar 2, 2016 · 47ab5f3 · 47ab5f3
1 parent eab97b1
commit 47ab5f3
Show file tree

Hide file tree

Showing 10 changed files with 506 additions and 42 deletions.
diff --git a/src/vnsw/agent/cmn/agent.cc b/src/vnsw/agent/cmn/agent.cc
@@ -772,7 +772,11 @@ Agent::ForwardingMode Agent::TranslateForwardingMode
 
 void Agent::set_flow_table_size(uint32_t count) {
     flow_table_size_ = count;
-    max_vm_flows_ = (count * params_->max_vm_flows()) / 100;
+    if (params_->max_vm_flows() >= 100) {
+        max_vm_flows_ = 0;
+    } else {
+        max_vm_flows_ = (count * params_->max_vm_flows()) / 100;
+    }
 }
 
 void Agent::set_controller_xmpp_channel(AgentXmppChannel *channel, uint8_t idx) {

diff --git a/src/vnsw/agent/oper/global_vrouter.h b/src/vnsw/agent/oper/global_vrouter.h
@@ -6,6 +6,7 @@
 #define vnsw_agent_global_router_h_
 
 #include <cmn/agent_cmn.h>
+#include <oper/ecmp_load_balance.h>
 
 class OperDB;
 class VnEntry;

diff --git a/src/vnsw/agent/pkt/flow_entry.cc b/src/vnsw/agent/pkt/flow_entry.cc
@@ -93,7 +93,8 @@ SecurityGroupList FlowEntry::default_sg_list_;
 // VmFlowRef
 /////////////////////////////////////////////////////////////////////////////
 const int VmFlowRef::kInvalidFd;
-VmFlowRef::VmFlowRef() : vm_(NULL), fd_(kInvalidFd), port_(0) {
+VmFlowRef::VmFlowRef() :
+    vm_(NULL), fd_(kInvalidFd), port_(0), flow_(NULL) {
 }
 
 VmFlowRef::VmFlowRef(const VmFlowRef &rhs) {
@@ -104,21 +105,46 @@ VmFlowRef::VmFlowRef(const VmFlowRef &rhs) {
 }
 
 VmFlowRef:: ~VmFlowRef() {
-    Reset();
+    Reset(true);
+}
+
+void VmFlowRef::Init(FlowEntry *flow) {
+    flow_ = flow;
 }
 
 void VmFlowRef::operator=(const VmFlowRef &rhs) {
-    // When flow is evicted by vrouter, reuse the older fd and port
-    Reset();
-    fd_ = rhs.fd_;
-    port_ = rhs.port_;
-    SetVm(rhs.vm_.get());
+    assert(rhs.fd_ == VmFlowRef::kInvalidFd);
+    assert(rhs.port_ == 0);
+    // For linklocal flows, we should have called Move already. It would
+    // reset vm_. Validate it
+    if (fd_ != VmFlowRef::kInvalidFd)
+        assert(rhs.vm_.get() == NULL);
+}
+
+// Move is called from Copy() routine when flow is evicted by vrouter and a
+// new flow-add is received by agent. Use the fd_ and port_ from new flow
+// since reverse flow will be setup based on these
+void VmFlowRef::Move(VmFlowRef *rhs) {
+    // Release the old values
+    Reset(false);
+
+    fd_ = rhs->fd_;
+    port_ = rhs->port_;
+    SetVm(rhs->vm_.get());
+
+    // Ownership for fd_ is transferred. Reset RHS fields
+    // Reset VM first before resetting fd_
+    rhs->SetVm(NULL);
+    rhs->fd_ = VmFlowRef::kInvalidFd;
+    rhs->port_ = 0;
 }
 
-void VmFlowRef::Reset() {
+void VmFlowRef::Reset(bool reset_flow) {
     FreeRef();
     FreeFd();
     vm_.reset(NULL);
+    if (reset_flow)
+        flow_ = NULL;
 }
 
 void VmFlowRef::FreeRef() {
@@ -128,8 +154,6 @@ void VmFlowRef::FreeRef() {
     vm_->update_flow_count(-1);
     if (fd_ != kInvalidFd) {
         vm_->update_linklocal_flow_count(-1);
-        Agent *agent = static_cast<VmTable *>(vm_->get_table())->agent();
-        agent->pkt()->get_flow_proto()->update_linklocal_flow_count(-1);
     }
 }
 
@@ -139,6 +163,9 @@ void VmFlowRef::FreeFd() {
         return;
     }
 
+    FlowProto *proto = flow_->flow_table()->agent()->pkt()->get_flow_proto();
+    proto->update_linklocal_flow_count(-1);
+    flow_->flow_table()->DelLinkLocalFlowInfo(fd_);
     close(fd_);
     fd_ = kInvalidFd;
     port_ = 0;
@@ -153,24 +180,22 @@ void VmFlowRef::SetVm(const VmEntry *vm) {
     if (vm == NULL)
         return;
 
-    // Add flow-ref-count for both forward and reverse flow
+    // update per-vm flow accounting
     vm->update_flow_count(1);
     if (fd_ != kInvalidFd) {
         vm_->update_linklocal_flow_count(1);
-        Agent *agent = static_cast<VmTable *>(vm->get_table())->agent();
-        agent->pkt()->get_flow_proto()->update_linklocal_flow_count(1);
     }
 
     return;
 }
 
-bool VmFlowRef::AllocateFd(Agent *agent, FlowEntry *flow, uint8_t l3_proto) {
+bool VmFlowRef::AllocateFd(Agent *agent, uint8_t l3_proto) {
     if (fd_ != kInvalidFd)
         return true;
 
     port_ = 0;
     // Short flows are always dropped. Dont allocate FD for short flow
-    if (flow->IsShortFlow())
+    if (flow_->IsShortFlow())
         return false;
 
     if (l3_proto == IPPROTO_TCP) {
@@ -183,6 +208,11 @@ bool VmFlowRef::AllocateFd(Agent *agent, FlowEntry *flow, uint8_t l3_proto) {
         return false;
     }
 
+    // Update agent accounting info
+    agent->pkt()->get_flow_proto()->update_linklocal_flow_count(1);
+    flow_->flow_table()->AddLinkLocalFlowInfo(fd_, flow_->flow_handle(),
+                                              flow_->key(), UTCTimestampUsec());
+
     // allow the socket to be reused upon close
     int optval = 1;
     setsockopt(fd_, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof(optval));
@@ -230,8 +260,8 @@ void FlowData::Reset() {
     match_p.Reset();
     vn_entry.reset(NULL);
     intf_entry.reset(NULL);
-    in_vm_entry.Reset();
-    out_vm_entry.Reset();
+    in_vm_entry.Reset(true);
+    out_vm_entry.Reset(true);
     nh.reset(NULL);
     vrf = VrfEntry::kInvalidIndex;
     mirror_vrf = VrfEntry::kInvalidIndex;
@@ -357,17 +387,31 @@ void FlowEntry::Init() {
 
 FlowEntry *FlowEntry::Allocate(const FlowKey &key, FlowTable *flow_table) {
     // flow_table will be NULL for some UT cases
+    FlowEntry *flow;
     if (flow_table == NULL) {
-        FlowEntry *flow = new FlowEntry(flow_table);
+        flow = new FlowEntry(flow_table);
         flow->Reset(key);
-        return flow;
+    } else {
+        flow = flow_table->free_list()->Allocate(key);
     }
 
-    return flow_table->free_list()->Allocate(key);
+    flow->data_.in_vm_entry.Init(flow);
+    flow->data_.out_vm_entry.Init(flow);
+    return flow;
 }
 
 // selectively copy fields from RHS
-void FlowEntry::Copy(const FlowEntry *rhs, bool update) {
+void FlowEntry::Copy(FlowEntry *rhs, bool update) {
+    if (update) {
+        rhs->data_.in_vm_entry.FreeFd();
+        rhs->data_.out_vm_entry.FreeFd();
+    } else {
+        // The operator= below will call VmFlowRef operator=. In case of flow
+        // eviction, we want to move ownership from rhs to lhs. However rhs is
+        // const ref in operator so, invode Move API to transfer ownership
+        data_.in_vm_entry.Move(&rhs->data_.in_vm_entry);
+        data_.out_vm_entry.Move(&rhs->data_.out_vm_entry);
+    }
     data_ = rhs->data_;
     flags_ = rhs->flags_;
     short_flow_reason_ = rhs->short_flow_reason_;
@@ -469,9 +513,6 @@ void FlowEntry::InitFwdFlow(const PktFlowInfo *info, const PktInfo *pkt,
         return;
     }
     if (info->linklocal_bind_local_port) {
-        flow_table_->AddLinkLocalFlowInfo(data_.in_vm_entry.fd(),
-                                          flow_handle_,
-                                          key_, UTCTimestampUsec());
         set_flags(FlowEntry::LinkLocalBindLocalSrcPort);
     } else {
         reset_flags(FlowEntry::LinkLocalBindLocalSrcPort);

diff --git a/src/vnsw/agent/pkt/flow_entry.h b/src/vnsw/agent/pkt/flow_entry.h
@@ -54,12 +54,14 @@ class VmFlowRef {
     VmFlowRef(const VmFlowRef &rhs);
     ~VmFlowRef();
 
+    void Init(FlowEntry *flow);
     void operator=(const VmFlowRef &rhs);
-    void Reset();
+    void Reset(bool reset_flow);
     void FreeRef();
     void FreeFd();
     void SetVm(const VmEntry *vm);
-    bool AllocateFd(Agent *agent, FlowEntry *flow, uint8_t l3_proto);
+    bool AllocateFd(Agent *agent, uint8_t l3_proto);
+    void Move(VmFlowRef *rhs);
 
     int fd() const { return fd_; }
     uint16_t port() const { return port_; }
@@ -70,6 +72,7 @@ class VmFlowRef {
     VmEntryConstRef vm_;
     int fd_;
     uint16_t port_;
+    FlowEntry *flow_;
 };
 
 typedef boost::intrusive_ptr<FlowEntry> FlowEntryPtr;
@@ -352,7 +355,7 @@ class FlowEntry {
     void Reset();
 
     // Copy data fields from rhs
-    void Copy(const FlowEntry *rhs, bool update);
+    void Copy(FlowEntry *rhs, bool update);
 
     void InitFwdFlow(const PktFlowInfo *info, const PktInfo *pkt,
                      const PktControlInfo *ctrl,

diff --git a/src/vnsw/agent/pkt/flow_table.cc b/src/vnsw/agent/pkt/flow_table.cc
@@ -133,7 +133,7 @@ FlowEntry *FlowTable::Find(const FlowKey &key) {
     }
 }
 
-void FlowTable::Copy(FlowEntry *lhs, const FlowEntry *rhs, bool update) {
+void FlowTable::Copy(FlowEntry *lhs, FlowEntry *rhs, bool update) {
     DeleteFlowInfo(lhs);
     if (rhs)
         lhs->Copy(rhs, update);

diff --git a/src/vnsw/agent/pkt/flow_table.h b/src/vnsw/agent/pkt/flow_table.h
@@ -190,7 +190,7 @@ class FlowTable {
 
     static const char *TaskName() { return kTaskFlowEvent; }
     // Sandesh routines
-    void Copy(FlowEntry *lhs, const FlowEntry *rhs, bool update);
+    void Copy(FlowEntry *lhs, FlowEntry *rhs, bool update);
     void SetAclFlowSandeshData(const AclDBEntry *acl, AclFlowResp &data, 
                                const int last_count);
     void SetAceSandeshData(const AclDBEntry *acl, AclFlowCountResp &data, 

diff --git a/src/vnsw/agent/pkt/pkt_flow_info.cc b/src/vnsw/agent/pkt/pkt_flow_info.cc
@@ -1502,11 +1502,13 @@ void PktFlowInfo::ApplyFlowLimits(const PktControlInfo *in,
     }
 
     bool limit_exceeded = false;
-    if (in->vm_ && ((in->vm_->flow_count() + 2) > agent->max_vm_flows())) {
+    if (agent->max_vm_flows() &&
+        (in->vm_ && ((in->vm_->flow_count() + 2) > agent->max_vm_flows()))) {
         limit_exceeded = true;
     }
 
-    if (out->vm_ && ((out->vm_->flow_count() + 2) > agent->max_vm_flows())) {
+    if (agent->max_vm_flows() &&
+        (out->vm_ && ((out->vm_->flow_count() + 2) > agent->max_vm_flows()))) {
         limit_exceeded = true;
     }
 
@@ -1526,9 +1528,13 @@ void PktFlowInfo::ApplyFlowLimits(const PktControlInfo *in,
         limit_exceeded = true;
     }
 
-    if (in->vm_ && in->vm_->linklocal_flow_count() >=
-        agent->params()->linklocal_vm_flows()) {
-        limit_exceeded = true;
+    // Check per-vm linklocal flow-limits if specified
+    if ((agent->params()->linklocal_vm_flows() !=
+         agent->params()->linklocal_system_flows())) {
+        if (in->vm_ && in->vm_->linklocal_flow_count() >=
+            agent->params()->linklocal_vm_flows()) {
+            limit_exceeded = true;
+        }
     }
 
     if (limit_exceeded) {
@@ -1556,7 +1562,7 @@ void PktFlowInfo::LinkLocalPortBind(const PktInfo *pkt,
     if (short_flow)
         return;
 
-    if (flow->in_vm_flow_ref()->AllocateFd(agent, flow, pkt->ip_proto) == false) {
+    if (flow->in_vm_flow_ref()->AllocateFd(agent, pkt->ip_proto) == false) {
         // Could not allocate FD. Make it short flow
         agent->stats()->incr_flow_drop_due_to_max_limit();
         short_flow = true;

diff --git a/src/vnsw/agent/pkt/test/SConscript b/src/vnsw/agent/pkt/test/SConscript
@@ -26,6 +26,7 @@ test_pkt_flow_mock = AgentEnv.MakeTestCmd(env, 'test_pkt_flow_mock',
                                           pkt_flaky_test_suite)
 
 test_pkt_flow_limits = AgentEnv.MakeTestCmd(env, 'test_pkt_flow_limits', pkt_test_suite)
+test_pkt_linklocal = AgentEnv.MakeTestCmd(env, 'test_pkt_linklocal', pkt_test_suite)
 test_flow_adit = AgentEnv.MakeTestCmd(env, 'test_flow_audit', pkt_test_suite)
 test_pkt_flow = AgentEnv.MakeTestCmd(env, 'test_pkt_flow', pkt_flaky_test_suite)
 test_pkt_flowv6 = AgentEnv.MakeTestCmd(env, 'test_pkt_flowv6', pkt_test_suite)

diff --git a/src/vnsw/agent/pkt/test/test_pkt_flow_limits.cc b/src/vnsw/agent/pkt/test/test_pkt_flow_limits.cc
@@ -192,6 +192,11 @@ class FlowTest : public ::testing::Test {
         EXPECT_EQ(0U, agent()->vn_table()->Size());
         EXPECT_EQ(0U, agent()->acl_table()->Size());
         EXPECT_EQ(1U, agent()->vrf_table()->Size());
+
+        EXPECT_EQ(flow_proto_->linklocal_flow_count(), 0);
+        EXPECT_EQ(flow_proto_->linklocal_flow_count(), 0);
+        FlowTable *table = flow_proto_->GetTable(0);
+        EXPECT_EQ(table->linklocal_flow_info_map().size(), 0);
     }
 
     Agent *agent() {return agent_;}
@@ -242,12 +247,6 @@ TEST_F(FlowTest, LinkLocalFlow_1) {
                         GetFlowKeyNH(input[0].intf_id)) != NULL);
 
     EXPECT_EQ(2U, get_flow_proto()->FlowCount());
-    EXPECT_TRUE(FlowGet(0, fabric_ip.c_str(), vhost_ip_addr, IPPROTO_TCP,
-                        fabric_port, linklocal_src_port,
-                        vhost->flow_key_nh()->id()) != NULL);
-    EXPECT_TRUE(FlowGet(VrfGet("vrf5")->vrf_id(), vm1_ip, linklocal_ip,
-                        IPPROTO_TCP, 3000, linklocal_port,
-                        GetFlowKeyNH(input[0].intf_id)) != NULL);
 
     //Delete forward flow and expect both flows to be deleted
     DeleteFlow(nat_flow, 1);