From 4560f5e6a1d228da1e37b719afab2382eebeab9c Mon Sep 17 00:00:00 2001 From: Kiran Kella Date: Sat, 26 Jan 2019 19:05:07 +0530 Subject: [PATCH 1/5] In a routing vlan, the neighbor entry in the /31 subnet is not getting added to hardware. Fix: Don't add the net broadcast neighbor entry on the /31 subnet routin vlan as there is no net broadcast address in /31 subnets. Signed-off-by: kiran.kella@broadcom.com --- orchagent/intfsorch.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 4006d48511..dfee09ac90 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -160,7 +160,12 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre if (port.m_type == Port::VLAN && ip_prefix->isV4()) { - addDirectedBroadcast(port, ip_prefix->getBroadcastIp()); + /* For /31 IPv4 subnets, there is no broadcast address, hence don't + * add a net directed broadcast route specific neighbor entry */ + if (ip_prefix.getMaskLength() != 31) + { + addDirectedBroadcast(port, ip_prefix->getBroadcastIp()); + } } m_syncdIntfses[alias].ip_addresses.insert(*ip_prefix); @@ -346,7 +351,12 @@ void IntfsOrch::doTask(Consumer &consumer) removeIp2MeRoute(vrf_id, ip_prefix); if(port.m_type == Port::VLAN && ip_prefix.isV4()) { - removeDirectedBroadcast(port, ip_prefix.getBroadcastIp()); + /* For /31 IPv4 subnets, there is no net directed broadcast route + * specific neighbor entry that was added */ + if (ip_prefix.getMaskLength() != 31) + { + removeDirectedBroadcast(port, ip_prefix.getBroadcastIp()); + } } m_syncdIntfses[alias].ip_addresses.erase(ip_prefix); From ec372b84254cdd08700c8e80bb9ee5c3f5343dcb Mon Sep 17 00:00:00 2001 From: Kiran Kella Date: Sat, 26 Jan 2019 19:05:07 +0530 Subject: [PATCH 2/5] In a routing vlan, the neighbor entry in the /31 subnet is not getting added to hardware. Fix: Don't add the net broadcast neighbor entry on the /31 subnet routin vlan as there is no net broadcast address in /31 subnets. Signed-off-by: kiran.kella@broadcom.com --- orchagent/intfsorch.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 4006d48511..787d5a4ab4 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -160,7 +160,12 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre if (port.m_type == Port::VLAN && ip_prefix->isV4()) { - addDirectedBroadcast(port, ip_prefix->getBroadcastIp()); + /* For /31 IPv4 subnets, there is no broadcast address, hence don't + * add a net directed broadcast route specific neighbor entry */ + if (ip_prefix->getMaskLength() != 31) + { + addDirectedBroadcast(port, ip_prefix->getBroadcastIp()); + } } m_syncdIntfses[alias].ip_addresses.insert(*ip_prefix); @@ -346,7 +351,12 @@ void IntfsOrch::doTask(Consumer &consumer) removeIp2MeRoute(vrf_id, ip_prefix); if(port.m_type == Port::VLAN && ip_prefix.isV4()) { - removeDirectedBroadcast(port, ip_prefix.getBroadcastIp()); + /* For /31 IPv4 subnets, there is no net directed broadcast route + * specific neighbor entry that was added */ + if (ip_prefix.getMaskLength() != 31) + { + removeDirectedBroadcast(port, ip_prefix.getBroadcastIp()); + } } m_syncdIntfses[alias].ip_addresses.erase(ip_prefix); From a7052db48638784daaec289cac19cc00fb72c5f1 Mon Sep 17 00:00:00 2001 From: Kiran Kella Date: Sun, 27 Jan 2019 13:42:56 +0530 Subject: [PATCH 3/5] Review comments incorporated. Check for /31 subnet is moved to inside addDirectedBroadcast() and removeDirectedBroadcast APIs. --- orchagent/intfsorch.cpp | 38 ++++++++++++++++++++++++-------------- orchagent/intfsorch.h | 4 ++-- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 787d5a4ab4..e110f14e9b 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -160,12 +160,7 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre if (port.m_type == Port::VLAN && ip_prefix->isV4()) { - /* For /31 IPv4 subnets, there is no broadcast address, hence don't - * add a net directed broadcast route specific neighbor entry */ - if (ip_prefix->getMaskLength() != 31) - { - addDirectedBroadcast(port, ip_prefix->getBroadcastIp()); - } + addDirectedBroadcast(port, *ip_prefix); } m_syncdIntfses[alias].ip_addresses.insert(*ip_prefix); @@ -349,14 +344,10 @@ void IntfsOrch::doTask(Consumer &consumer) { removeSubnetRoute(port, ip_prefix); removeIp2MeRoute(vrf_id, ip_prefix); + if(port.m_type == Port::VLAN && ip_prefix.isV4()) { - /* For /31 IPv4 subnets, there is no net directed broadcast route - * specific neighbor entry that was added */ - if (ip_prefix.getMaskLength() != 31) - { - removeDirectedBroadcast(port, ip_prefix.getBroadcastIp()); - } + removeDirectedBroadcast(port, ip_prefix); } m_syncdIntfses[alias].ip_addresses.erase(ip_prefix); @@ -633,10 +624,20 @@ void IntfsOrch::removeIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_pref } } -void IntfsOrch::addDirectedBroadcast(const Port &port, const IpAddress &ip_addr) +void IntfsOrch::addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix) { sai_status_t status; sai_neighbor_entry_t neighbor_entry; + IpAddress ip_addr; + + /* For /31 v4 subnets, there is no broadcast address, hence don't + * add a broadcast route. */ + if (ip_prefix.getMaskLength() == 31) + { + return; + } + ip_addr = ip_prefix.getBroadcastIp(); + neighbor_entry.rif_id = port.m_rif_id; neighbor_entry.switch_id = gSwitchId; copy(neighbor_entry.ip_address, ip_addr); @@ -656,10 +657,19 @@ void IntfsOrch::addDirectedBroadcast(const Port &port, const IpAddress &ip_addr) SWSS_LOG_NOTICE("Add broadcast route for ip:%s", ip_addr.to_string().c_str()); } -void IntfsOrch::removeDirectedBroadcast(const Port &port, const IpAddress &ip_addr) +void IntfsOrch::removeDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix) { sai_status_t status; sai_neighbor_entry_t neighbor_entry; + IpAddress ip_addr; + + /* For /31 v4 subnets, there is no broadcast address */ + if (ip_prefix.getMaskLength() == 31) + { + return; + } + ip_addr = ip_prefix.getBroadcastIp(); + neighbor_entry.rif_id = port.m_rif_id; neighbor_entry.switch_id = gSwitchId; copy(neighbor_entry.ip_address, ip_addr); diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index 33ad4a6f51..e46e46232e 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -52,8 +52,8 @@ class IntfsOrch : public Orch void addIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix); void removeIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix); - void addDirectedBroadcast(const Port &port, const IpAddress &ip_addr); - void removeDirectedBroadcast(const Port &port, const IpAddress &ip_addr); + void addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix); + void removeDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix); }; #endif /* SWSS_INTFSORCH_H */ From 00ef161ba05041f309ddc8d2cd5777e8c08b68df Mon Sep 17 00:00:00 2001 From: Kiran Kella Date: Mon, 28 Jan 2019 11:15:36 +0530 Subject: [PATCH 4/5] Incorporated review comments. --- orchagent/intfsorch.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index e110f14e9b..8e0c193fcf 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -158,7 +158,7 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre addSubnetRoute(port, *ip_prefix); addIp2MeRoute(vrf_id, *ip_prefix); - if (port.m_type == Port::VLAN && ip_prefix->isV4()) + if (port.m_type == Port::VLAN) { addDirectedBroadcast(port, *ip_prefix); } @@ -345,7 +345,7 @@ void IntfsOrch::doTask(Consumer &consumer) removeSubnetRoute(port, ip_prefix); removeIp2MeRoute(vrf_id, ip_prefix); - if(port.m_type == Port::VLAN && ip_prefix.isV4()) + if(port.m_type == Port::VLAN) { removeDirectedBroadcast(port, ip_prefix); } @@ -630,9 +630,9 @@ void IntfsOrch::addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix sai_neighbor_entry_t neighbor_entry; IpAddress ip_addr; - /* For /31 v4 subnets, there is no broadcast address, hence don't + /* For /31 and /32 v4 subnets, there is no broadcast address, hence don't * add a broadcast route. */ - if (ip_prefix.getMaskLength() == 31) + if ((ip_prefix.isV4()) && (ip_prefix.getMaskLength() > 30)) { return; } @@ -663,8 +663,8 @@ void IntfsOrch::removeDirectedBroadcast(const Port &port, const IpPrefix &ip_pre sai_neighbor_entry_t neighbor_entry; IpAddress ip_addr; - /* For /31 v4 subnets, there is no broadcast address */ - if (ip_prefix.getMaskLength() == 31) + /* For /31 and /32 v4 subnets, there is no broadcast address */ + if ((ip_prefix.isV4()) && (ip_prefix.getMaskLength() > 30)) { return; } From 4b249f5e162bde74a7447e527fe8ed5556f35c94 Mon Sep 17 00:00:00 2001 From: Kiran Kella Date: Mon, 28 Jan 2019 23:21:52 +0530 Subject: [PATCH 5/5] Review comment addressed. --- orchagent/intfsorch.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 8e0c193fcf..34d857b51a 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -630,9 +630,9 @@ void IntfsOrch::addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix sai_neighbor_entry_t neighbor_entry; IpAddress ip_addr; - /* For /31 and /32 v4 subnets, there is no broadcast address, hence don't + /* If not IPv4 subnet or if /31 or /32 subnet, there is no broadcast address, hence don't * add a broadcast route. */ - if ((ip_prefix.isV4()) && (ip_prefix.getMaskLength() > 30)) + if (!(ip_prefix.isV4()) || (ip_prefix.getMaskLength() > 30)) { return; } @@ -663,8 +663,8 @@ void IntfsOrch::removeDirectedBroadcast(const Port &port, const IpPrefix &ip_pre sai_neighbor_entry_t neighbor_entry; IpAddress ip_addr; - /* For /31 and /32 v4 subnets, there is no broadcast address */ - if ((ip_prefix.isV4()) && (ip_prefix.getMaskLength() > 30)) + /* If not IPv4 subnet or if /31 or /32 subnet, there is no broadcast address */ + if (!(ip_prefix.isV4()) || (ip_prefix.getMaskLength() > 30)) { return; }