diff --git a/src/vnsw/agent/pkt/flow_entry.cc b/src/vnsw/agent/pkt/flow_entry.cc index bcd83bf447c..97dd84384fa 100644 --- a/src/vnsw/agent/pkt/flow_entry.cc +++ b/src/vnsw/agent/pkt/flow_entry.cc @@ -1719,7 +1719,7 @@ void FlowEntry::SetRemoteFlowEcmpIndex() { label = 0; boost::system::error_code ec; Ip4Address dest_ip = Ip4Address::from_string(peer_vrouter_, ec); - if (ec.value() == 0) { + if (ec.value() != 0) { return; } diff --git a/src/vnsw/agent/pkt/flow_handler.cc b/src/vnsw/agent/pkt/flow_handler.cc index 3184cebae49..52b15b87d88 100644 --- a/src/vnsw/agent/pkt/flow_handler.cc +++ b/src/vnsw/agent/pkt/flow_handler.cc @@ -103,6 +103,7 @@ bool FlowHandler::Run() { pkt_info_->vrf = fe->data().vrf; pkt_info_->l3_forwarding = fe->l3_flow(); info.l3_flow = fe->l3_flow(); + info.out_component_nh_idx = fe->data().component_nh_idx; } else { info.l3_flow = pkt_info_->l3_forwarding = IsL3ModeFlow(); } diff --git a/src/vnsw/agent/pkt/flow_table.cc b/src/vnsw/agent/pkt/flow_table.cc index 4955a0618aa..e4691eba269 100644 --- a/src/vnsw/agent/pkt/flow_table.cc +++ b/src/vnsw/agent/pkt/flow_table.cc @@ -157,20 +157,31 @@ void FlowTable::Add(FlowEntry *flow, FlowEntry *rflow) { uint64_t time = UTCTimestampUsec(); FlowEntry *new_flow = Locate(flow, time); FlowEntry *new_rflow = (rflow != NULL) ? Locate(rflow, time) : NULL; + FLOW_LOCK(new_flow, new_rflow); - AddInternal(flow, new_flow, rflow, new_rflow, false); + AddInternal(flow, new_flow, rflow, new_rflow, false, false); } void FlowTable::Update(FlowEntry *flow, FlowEntry *rflow) { + bool fwd_flow_update = true; FlowEntry *new_flow = Find(flow->key()); + FlowEntry *new_rflow = (rflow != NULL) ? Find(rflow->key()) : NULL; + bool rev_flow_update = true; + if (rflow && new_rflow == NULL) { + uint64_t time = UTCTimestampUsec(); + new_rflow = Locate(rflow, time); + rev_flow_update = false; + } + FLOW_LOCK(new_flow, new_rflow); - AddInternal(flow, new_flow, rflow, new_rflow, true); + AddInternal(flow, new_flow, rflow, new_rflow, fwd_flow_update, + rev_flow_update); } void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow, FlowEntry *rflow_req, FlowEntry *rflow, - bool update) { + bool fwd_flow_update, bool rev_flow_update) { // The forward and reverse flow in request are linked. Unlink the flows // first. Flow table processing will link them if necessary flow_req->set_reverse_flow_entry(NULL); @@ -178,7 +189,7 @@ void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow, rflow_req->set_reverse_flow_entry(NULL); bool force_update_rflow = false; - if (update) { + if (fwd_flow_update) { if (flow == NULL) return; @@ -195,13 +206,13 @@ void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow, // so trigger a force update instead of add for reverse flow force_update_rflow = true; } - Copy(flow, flow_req, update); + Copy(flow, flow_req, fwd_flow_update); flow->set_deleted(false); } if (rflow) { if (rflow_req != rflow) { - Copy(rflow, rflow_req, (update || force_update_rflow)); + Copy(rflow, rflow_req, (rev_flow_update || force_update_rflow)); // if the reverse flow was marked delete, reset its flow handle // to invalid index to assure it is attempted to reprogram using // kInvalidFlowHandle, this also ensures that flow entry wont @@ -252,11 +263,11 @@ void FlowTable::AddInternal(FlowEntry *flow_req, FlowEntry *flow, // While the scenario above cannot be totally avoided, programming reverse // flow first will reduce the probability if (rflow) { - UpdateKSync(rflow, (update || force_update_rflow)); + UpdateKSync(rflow, (rev_flow_update || force_update_rflow)); AddFlowInfo(rflow); } - UpdateKSync(flow, update); + UpdateKSync(flow, fwd_flow_update); AddFlowInfo(flow); } diff --git a/src/vnsw/agent/pkt/flow_table.h b/src/vnsw/agent/pkt/flow_table.h index f14e426701b..57ec07febf0 100644 --- a/src/vnsw/agent/pkt/flow_table.h +++ b/src/vnsw/agent/pkt/flow_table.h @@ -254,7 +254,10 @@ class FlowTable { void UpdateUnLocked(FlowEntry *flow, FlowEntry *rflow); void AddInternal(FlowEntry *flow, FlowEntry *new_flow, FlowEntry *rflow, - FlowEntry *new_rflow, bool update); + FlowEntry *new_rflow, bool fwd_flow_update, + bool rev_flow_update); + void Add(FlowEntry *flow, FlowEntry *new_flow, FlowEntry *rflow, + FlowEntry *new_rflow, bool fwd_flow_update, bool rev_flow_update); void GetMutexSeq(tbb::mutex &mutex1, tbb::mutex &mutex2, tbb::mutex **mutex_ptr_1, tbb::mutex **mutex_ptr_2); void EvictFlow(FlowEntry *flow, FlowEntry *rflow); diff --git a/src/vnsw/agent/pkt/pkt_flow_info.cc b/src/vnsw/agent/pkt/pkt_flow_info.cc index 09ad138935e..762477555a6 100644 --- a/src/vnsw/agent/pkt/pkt_flow_info.cc +++ b/src/vnsw/agent/pkt/pkt_flow_info.cc @@ -212,6 +212,12 @@ static bool NhDecode(const NextHop *nh, const PktInfo *pkt, PktFlowInfo *info, info->ecmp = true; const CompositeNH *comp_nh = static_cast(nh); + if (pkt->type == PktType::MESSAGE && + info->out_component_nh_idx == + CompositeNH::kInvalidComponentNHIdx) { + info->out_component_nh_idx = 0; + } + // Compute out_component_nh_idx if, // 1. out_compoenent_nh_idx was set, but points to a deleted NH // This can happen if flow is trapped for ECMP resolution from @@ -1594,8 +1600,9 @@ void PktFlowInfo::Add(const PktInfo *pkt, PktControlInfo *in, UpdateEvictedFlowStats(pkt); } - if (pkt->type == PktType::MESSAGE && - pkt->agent_hdr.cmd == AgentHdr::TRAP_FLOW_MISS) { + if ((pkt->type == PktType::MESSAGE && + pkt->agent_hdr.cmd == AgentHdr::TRAP_FLOW_MISS) || + pkt->agent_hdr.cmd == AgentHdr::TRAP_ECMP_RESOLVE) { update = true; } @@ -1647,7 +1654,7 @@ void PktFlowInfo::Add(const PktInfo *pkt, PktControlInfo *in, bool swap_flows = false; // If this is message processing, then retain forward and reverse flows - if (pkt->type == PktType::MESSAGE && + if (pkt->type == PktType::MESSAGE && !short_flow && flow_entry->is_flags_set(FlowEntry::ReverseFlow)) { // for cases where we need to swap flows rflow should always // be Non-NULL diff --git a/src/vnsw/agent/pkt/pkt_flow_info.h b/src/vnsw/agent/pkt/pkt_flow_info.h index b89a0eb3253..2c5ea740d2f 100644 --- a/src/vnsw/agent/pkt/pkt_flow_info.h +++ b/src/vnsw/agent/pkt/pkt_flow_info.h @@ -79,8 +79,6 @@ class PktFlowInfo { void Add(const PktInfo *pkt, PktControlInfo *in, PktControlInfo *out); bool Process(const PktInfo *pkt, PktControlInfo *in, PktControlInfo *out); - void SetEcmpFlowInfo(const PktInfo *pkt, const PktControlInfo *in, - const PktControlInfo *out); static bool GetIngressNwPolicyAclList(const Interface *intf, const VnEntry *vn, MatchPolicy *m_policy); diff --git a/src/vnsw/agent/pkt/test/test_ecmp.cc b/src/vnsw/agent/pkt/test/test_ecmp.cc index 5132b4b7f74..86fb4dadab7 100644 --- a/src/vnsw/agent/pkt/test/test_ecmp.cc +++ b/src/vnsw/agent/pkt/test/test_ecmp.cc @@ -102,19 +102,19 @@ class EcmpTest : public ::testing::Test { //Add couple of remote VM routes for generating packet Inet4TunnelRouteAdd(bgp_peer, "vrf2", remote_vm_ip1_, 32, - remote_server_ip_, TunnelType::AllType(), + remote_server_ip_, TunnelType::GREType(), 30, "vn2", SecurityGroupList(), PathPreference()); Inet4TunnelRouteAdd(bgp_peer, "default-project:vn3:vn3", remote_vm_ip2_, 32, - remote_server_ip_, TunnelType::AllType(), + remote_server_ip_, TunnelType::GREType(), 30, "default-project:vn3", SecurityGroupList(), PathPreference()); Inet4TunnelRouteAdd(bgp_peer, "default-project:vn4:vn4", remote_vm_ip3_, 32, - remote_server_ip_, TunnelType::AllType(), + remote_server_ip_, TunnelType::GREType(), 30, "default-project:vn4", SecurityGroupList(), PathPreference()); client->WaitForIdle(); @@ -184,7 +184,7 @@ class EcmpTest : public ::testing::Test { label, Agent::GetInstance()->fabric_vrf_name(), Agent::GetInstance()->router_id(), Ip4Address(remote_server_ip++), - false, TunnelType::AllType())); + false, TunnelType::GREType())); comp_nh_list.push_back(comp_nh); if (!same_label) { label++; @@ -207,6 +207,7 @@ class EcmpTest : public ::testing::Test { VxLanTable::kInvalidvxlan_id, false, vn_list, InterfaceNHFlags::INET4, SecurityGroupList(), PathPreference(), ControllerPeerPath::kInvalidPeerIdentifier, + EcmpLoadBalance(), NULL); InetUnicastAgentRouteTable *rt_table = agent_->vrf_table()->GetInet4UnicastRouteTable(vrf_name); @@ -224,7 +225,7 @@ class EcmpTest : public ::testing::Test { ControllerVmRoute *data = ControllerVmRoute::MakeControllerVmRoute(bgp_peer, agent_->fabric_vrf_name(), agent_->router_id(), - vrf_name, addr, TunnelType::AllType(), 16, + vrf_name, addr, TunnelType::GREType(), 16, vn_list, SecurityGroupList(), PathPreference(), false, EcmpLoadBalance()); InetUnicastAgentRouteTable::AddRemoteVmRouteReq(bgp_peer, @@ -431,19 +432,19 @@ TEST_F(EcmpTest, EcmpTest_8) { ComponentNHKeyPtr comp_nh_data1(new ComponentNHKey( 16, Agent::GetInstance()->fabric_vrf_name(), Agent::GetInstance()->router_id(), server_ip1, false, - TunnelType::AllType())); + TunnelType::GREType())); comp_nh.push_back(comp_nh_data1); ComponentNHKeyPtr comp_nh_data2(new ComponentNHKey( 17, Agent::GetInstance()->fabric_vrf_name(), Agent::GetInstance()->router_id(), - server_ip2, false, TunnelType::AllType())); + server_ip2, false, TunnelType::GREType())); comp_nh.push_back(comp_nh_data2); ComponentNHKeyPtr comp_nh_data3(new ComponentNHKey( 18, Agent::GetInstance()->fabric_vrf_name(), Agent::GetInstance()->router_id(), - server_ip3, false, TunnelType::AllType())); + server_ip3, false, TunnelType::GREType())); comp_nh.push_back(comp_nh_data3); SecurityGroupList sg_list; @@ -549,7 +550,6 @@ TEST_F(EcmpTest, EcmpTest_10) { FlowEntry *entry = FlowGet(VrfGet("vrf2")->vrf_id(), "2.1.1.1", "9.1.1.1", 1, 0, 0, GetFlowKeyNH(2)); - FlowEntry *old_entry = entry; EXPECT_TRUE(entry != NULL); EXPECT_TRUE(entry->data().component_nh_idx == CompositeNH::kInvalidComponentNHIdx); @@ -573,10 +573,6 @@ TEST_F(EcmpTest, EcmpTest_10) { EXPECT_TRUE(entry->data().component_nh_idx == CompositeNH::kInvalidComponentNHIdx); EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false); - //Old flow and new flow have same key, hence old flow should become - //short flow - EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true); - old_entry = entry; //Reverse flow is no ECMP rev_entry = entry->reverse_flow_entry(); @@ -596,9 +592,6 @@ TEST_F(EcmpTest, EcmpTest_10) { EXPECT_TRUE(entry->data().component_nh_idx == CompositeNH::kInvalidComponentNHIdx); EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false); - //Old flow and new flow have same key, hence old flow should become - //short flow - EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true); //Reverse flow is no ECMP rev_entry = entry->reverse_flow_entry(); @@ -667,7 +660,6 @@ TEST_F(EcmpTest, EcmpTest_11) { EXPECT_TRUE(nh->GetInterface()->name() == "vnet2"); EXPECT_TRUE(rev_entry->is_flags_set(FlowEntry::ShortFlow) == false); - FlowEntry *old_entry = entry; TxIpPacket(VmPortGetId(3), "2.1.1.1", "9.1.1.1", 1); client->WaitForIdle(); entry = FlowGet(VrfGet("vrf2")->vrf_id(), @@ -676,9 +668,6 @@ TEST_F(EcmpTest, EcmpTest_11) { EXPECT_TRUE(entry->data().component_nh_idx == CompositeNH::kInvalidComponentNHIdx); EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false); - //Old entry becomes short flow since key are same - EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true); - old_entry = entry; //Reverse flow is no ECMP rev_entry = entry->reverse_flow_entry(); @@ -698,8 +687,6 @@ TEST_F(EcmpTest, EcmpTest_11) { EXPECT_TRUE(entry->data().component_nh_idx == CompositeNH::kInvalidComponentNHIdx); EXPECT_TRUE(entry->is_flags_set(FlowEntry::ShortFlow) == false); - //Old entry becomes short flow since key are same - EXPECT_TRUE(old_entry->is_flags_set(FlowEntry::ShortFlow) == true); //Reverse flow is no ECMP rev_entry = entry->reverse_flow_entry(); @@ -1201,7 +1188,7 @@ TEST_F(EcmpTest, EcmpReEval_2) { Ip4Address remote_vm_ip = Ip4Address::from_string("3.1.1.10"); Ip4Address remote_server_ip = Ip4Address::from_string("10.10.10.10"); Inet4TunnelRouteAdd(bgp_peer, "vrf2",remote_vm_ip, 32, - remote_server_ip, TunnelType::AllType(), 16, "vn2", + remote_server_ip, TunnelType::GREType(), 16, "vn2", SecurityGroupList(), PathPreference()); client->WaitForIdle(); TxIpPacket(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1); @@ -1219,7 +1206,7 @@ TEST_F(EcmpTest, EcmpReEval_2) { client->WaitForIdle(); //Enqueue a re-evaluate request - TxIpPacket(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1); + TxIpPacketEcmp(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1); client->WaitForIdle(); entry = FlowGet(VrfGet("vrf2")->vrf_id(), "1.1.1.1", "3.1.1.10", 1, 0, 0, GetFlowKeyNH(1)); @@ -1269,7 +1256,7 @@ TEST_F(EcmpTest, EcmpReEval_3) { client->WaitForIdle(); //Enqueue a re-evaluate request - TxIpPacket(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1); + TxIpPacketEcmp(VmPortGetId(1), "1.1.1.1", "3.1.1.10", 1); client->WaitForIdle(); entry = FlowGet(VrfGet("vrf2")->vrf_id(), "1.1.1.1", "3.1.1.10", 1, 0, 0, GetFlowKeyNH(1)); @@ -1476,7 +1463,7 @@ TEST_F(EcmpTest, ServiceVlanTest_3) { std::string vn_name_10("vn10"); std::string vn_name_11("vn11"); EXPECT_TRUE(VnMatch(entry->data().source_vn_list, vn_name_10)); - EXPECT_TRUE(VnMatch(entry->data().dest_vn _list, vn_name_11)); + EXPECT_TRUE(VnMatch(entry->data().dest_vn_list, vn_name_11)); //Reverse flow is no ECMP FlowEntry *rev_entry = entry->reverse_flow_entry(); @@ -1493,7 +1480,7 @@ TEST_F(EcmpTest, ServiceVlanTest_3) { } EXPECT_TRUE(rev_entry->data().dest_vrf == vrf_id); EXPECT_TRUE(VnMatch(rev_entry->data().source_vn_list, vn_name_11)); - EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn _list, vn_name_10)); + EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn_list, vn_name_10)); sport++; dport++; } @@ -1617,7 +1604,7 @@ TEST_F(EcmpTest, ServiceVlanTest_4) { std::string vn_name_10("vn10"); std::string vn_name_11("vn11"); EXPECT_TRUE(VnMatch(entry->data().source_vn_list, vn_name_10)); - EXPECT_TRUE(VnMatch(entry->data().dest_vn _list, vn_name_11)); + EXPECT_TRUE(VnMatch(entry->data().dest_vn_list, vn_name_11)); //Reverse flow is no ECMP FlowEntry *rev_entry = entry->reverse_flow_entry(); @@ -1628,7 +1615,7 @@ TEST_F(EcmpTest, ServiceVlanTest_4) { EXPECT_TRUE(rev_entry->data().vrf == service_vrf_id); EXPECT_TRUE(rev_entry->data().dest_vrf == vrf_id); EXPECT_TRUE(VnMatch(rev_entry->data().source_vn_list, vn_name_11)); - EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn _list, vn_name_10)); + EXPECT_TRUE(VnMatch(rev_entry->data().dest_vn_list, vn_name_10)); sport++; dport++; }