[swss] L2 Forwarding Enhancements by dr412113 · Pull Request #1716 · sonic-net/sonic-swss

dr412113 · 2021-04-20T10:48:35Z

What I did

Invoke flush Fdb entries per port/vlan
Instead of maintaining the vlan-members inside Port structure, added a new portVlanmap for vlan members.
/* Performance issue are seen during mac event processing with large number of vlans on a

port. Large data should not be added to port structure. */

created a portOidToName map to improve performace
delay lag delete until bridge port is deleted
delay bridge port removal until all asociated fdb entries are
removed. delete bridge port from portsorch instead of fdborch

Why I did it

To improve FDB performance.

How I verified it

Verified all mac operations.

Details if related

This reverts commit 047841e.

- Invoke flush Fdb entries per port/vlan - Instead of maintaining the vlan-members inside Port structure, added a new portVlanmap for vlan members. /* Performance issue are seen during mac event processing with large number of vlans on a * port. Large data should not be added to port structure. */ - created a portOidToName map to improve performace - delay lag delete until bridge port is deleted - delay bridge port removal until all asociated fdb entries are removed. delete bridge port from portsorch instead of fdborch

lguohan · 2021-05-12T21:28:51Z

@prsunny , can you take a look at this?

prsunny · 2021-05-18T15:49:10Z

orchagent/port.h

+     * port. Large data should not be added to this
+     * structure.
+     */
+    //vlan_members_t      m_vlan_members;


Please remove the commented section. You can mention it in the PR description.

prsunny · 2021-05-18T15:51:47Z

orchagent/portsorch.cpp

-        }
-    }
+    auto itr = portOidToName.find(id);
+    if (itr == portOidToName.end())


Please add {

prsunny · 2021-05-18T15:51:58Z

orchagent/portsorch.cpp

+    auto itr = portOidToName.find(id);
+    if (itr == portOidToName.end())
+        return false;
+    else {


{ -> next line

prsunny · 2021-05-18T15:58:33Z

orchagent/portsorch.cpp

            }
-            else
-                /* Cannot locate the VLAN */
+            if ((ret == true) && m_portVlanMember[port.m_alias].empty())


Is the logic changed here?. Just keep the previous one and use m_portVlanMember instead of port.m_vlan_members

also checking the return value of removeBridgePort() and retry if it returns false

prsunny · 2021-05-18T16:12:23Z

orchagent/portsorch.cpp


    if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID)
    {
+        /* If the port is being added to the first VLAN,


I didn't quite understand this section. Is this for the previous bridge port? In my understanding, this API is for the first bridge port addition, correct?

The previous bridge port delete is still not done, as fdb flush is in progress. But the port is added to a vlan again and now we want to reuse the previous bridgeport, so just make the admin state up.
When removing bridgeport, if fdb entries exist, we just disable admin state of the bridgeport and keep retrying until no more references exist before deleting the bridgeport.

prsunny · 2021-05-18T16:14:47Z

orchagent/portsorch.cpp

-
-    sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr);
-    if (status != SAI_STATUS_SUCCESS)
+    if (port.m_bridge_port_admin_state)


In this flow, in which case the m_bridge_port_admin_state would be false?

bridgeport deletion is not done if fdb entries exist, only admin state is set to down. bridgeport deletion will be retried and deleted when fdb entries are deleted.

prsunny · 2021-05-18T16:16:09Z

orchagent/portsorch.cpp

    }

-    SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu", vlan_alias.c_str(), vlan_id);
+    SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu vlan_oid 0x%lx", vlan_alias.c_str(), vlan_id, (long unsigned int)vlan_oid);


Please use PRIx64. You can refer other logs

prsunny · 2021-11-03T04:26:55Z

/azp run

azure-pipelines · 2021-11-03T04:27:05Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2021-11-04T17:14:17Z

/azp run

azure-pipelines · 2021-11-04T17:14:27Z

Azure Pipelines successfully started running 1 pipeline(s).

adyeung · 2021-11-05T16:37:06Z

@prsunny please help complete the review and merge

prsunny

Minor comments

prsunny · 2021-11-09T18:14:31Z

orchagent/portsorch.cpp

+    {
+        getPort(itr->second, port);
+        return true;
+	}


Pls fix alignment

prsunny · 2021-11-09T18:14:56Z

orchagent/portsorch.cpp

+    auto itr = portOidToName.find(bridge_port_id);
+    if (itr == portOidToName.end())
+        return false;
+    else {


{ -> new line

prsunny · 2021-11-09T18:14:58Z

orchagent/portsorch.cpp

-            return true;
-        }
+    auto itr = portOidToName.find(bridge_port_id);
+    if (itr == portOidToName.end())


Please have {

prsunny · 2021-11-09T18:22:32Z

@dgsudharsan , can you please review?

dgsudharsan · 2021-11-09T22:50:59Z

orchagent/portsorch.cpp

    if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID)
    {
+        /* If the port is being added to the first VLAN,
+         * set bridge port admin status to UP.


Won't the iterator in the remove_vlan member come back and delete this since it loops until last FDB is removed and then does remove_bridge_port?

Agree with @dgsudharsan that there could be race condition since the removeBridgePort is not complete and the iterator is retried for removal after this addition. I think this logic is slightly complicated. Since the main motivation is to separate the vlan member map from the Port class, could you please make that change and get it merged for this release. We can have the bridge port handling as a different PR.

dgsudharsan · 2021-11-09T23:03:17Z

orchagent/portsorch.cpp

 {
    SWSS_LOG_ENTER();

+    auto lagport = m_portList.find(lag_alias);


Please add some comments. I believe this is the scenario where removeBridgePort is not yet successful due to fdb flush right?

dgsudharsan · 2021-11-09T23:04:53Z

orchagent/portsorch.cpp

        return false;
    }
    m_portList[port.m_alias] = port;
+    portOidToName[port.m_bridge_port_id] = port.m_alias;


Are we saving both port object id to alias an bridge pot id to alias in data structure? It also has vlan and lag. Shouldn't we rename this to something like saiOidToAlias?

Yes. will change the name to saiOidToAlias.

prsunny · 2021-11-16T00:50:37Z

@dr412113 , @anilkpan , @anilkpandey , please check the compilation issue.

anilkpan · 2021-11-16T01:00:22Z

@prsunny the compilation error does not seem to be related to any changes in this PR. There is no change in aclorch.
Not sure why only the armhf build is failing.

             from aclorch.h:9,
             from aclorch.cpp:5:

/usr/include/c++/8/bits/stl_map.h: In member function 'virtual void AclRuleDTelFlowWatchListEntry::update(SubjectType, void*)':
/usr/include/c++/8/bits/stl_map.h:518:8: note: parameter passing for argument of type 'std::_Rb_tree<_sai_acl_entry_attr_t, std::pair<const _sai_acl_entry_attr_t, _sai_attribute_value_t>, std::_Select1st<std::pair<const _sai_acl_entry_attr_t, _sai_attribute_value_t> >, std::less<_sai_acl_entry_attr_t>, std::allocator<std::pair<const _sai_acl_entry_attr_t, _sai_attribute_value_t> > >::const_iterator' {aka 'std::_Rb_tree_const_iterator<std::pair<const _sai_acl_entry_attr_t, _sai_attribute_value_t> >'} changed in GCC 7.1
__i = _M_t._M_emplace_hint_unique(__i, std::piecewise_construct,
/usr/include/c++/8/bits/stl_map.h: In member function 'bool AclOrch::updateAclTablePorts(AclTable&, AclTable&)':
/usr/include/c++/8/bits/stl_map.h:499:8: note: parameter passing for argument of type 'std::_Rb_tree<long long unsigned int, std::pair<const long long unsigned int, long long unsigned int>, std::_Select1st<std::pair<const long long unsigned int, long long unsigned int> >, std::less, std::allocator<std::pair<const long long unsigned int, long long unsigned int> > >::const_iterator' {aka 'std::_Rb_tree_const_iterator<std::pair<const long long unsigned int, long long unsigned int> >'} changed in GCC 7.1
__i = _M_t._M_emplace_hint_unique(__i, std::piecewise_construct,
make[3]: Leaving directory '/__w/4/s/orchagent'
make[2]: *** [Makefile:416: all-recursive] Error 1

prsunny · 2021-11-16T01:25:42Z

/azp run

azure-pipelines · 2021-11-16T01:25:54Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2021-11-16T03:27:48Z

not sure why its consistently failing for this PR.. could you please rebase once and lets retry?

anilkpan · 2021-11-16T22:46:58Z

The GCC 7.1 warnings are not causing the failure, failure is due to a type mismatch in logging. I just fixed and updated.

anilkpan · 2021-11-17T19:43:21Z

/azpw run

anilkpan · 2021-11-17T20:00:23Z

/azpw run

anilkpan · 2021-11-17T20:04:59Z

vs test passed earlier. The only change in my last commit was to fix a type mismatch in SWSS_LOG.
I tried "/azpw run", but it does not restart the test.

anilkpan · 2021-11-17T23:06:15Z

/AzurePipelines run

azure-pipelines · 2021-11-17T23:06:20Z

Commenter does not have sufficient privileges for PR 1716 in repo Azure/sonic-swss

anilkpan · 2021-11-19T01:10:09Z

This seems to be the failure:

[2021-11-19 00:47:19] [build-stderr] saiaclschema_ut.cpp:1:10: fatal error: gmock/gmock.h: No such file or directory
[2021-11-19 00:47:19] [build-stderr] 1 | #include <gmock/gmock.h>
[2021-11-19 00:47:19] [build-stderr] | ^~~~~~~~~~~~~~~
[2021-11-19 00:47:19] [build-stderr] compilation terminated.

prsunny · 2021-11-19T01:16:42Z

@anilkpan , retrying now!

prsunny · 2021-11-19T18:11:33Z

@anilkpan , could you please rebase and push?

anilkpan · 2021-11-19T23:33:20Z

@prsunny Its passing now after rebase

* L2 Forwarding Enhancements

dr412113 added 5 commits April 5, 2021 09:23

PR1

047841e

Revert "PR1"

b19d1c6

This reverts commit 047841e.

Fix armhf compilation issue

34cec7b

Fix Learn Mode config failure

eace766

lguohan requested a review from prsunny April 24, 2021 19:52

prsunny requested changes May 18, 2021

View reviewed changes

prsunny mentioned this pull request May 18, 2021

[fdb]fdborch: fdb entries are deleted according to vlan or port or vlan&&port #1064

Merged

anilkpandey mentioned this pull request Oct 22, 2021

Layer 2 forwarding enhancements HLD sonic-net/SONiC#379

Merged

anilkpan added 3 commits October 26, 2021 23:44

updated as per review comments

5f166a2

Merge remote-tracking branch 'upstream/master'

015317e

Update fdborch.cpp

5d0ee12

prsunny reviewed Nov 9, 2021

View reviewed changes

Updated as per review comments

26fdd6c

dgsudharsan reviewed Nov 9, 2021

View reviewed changes

anilkpandey added 2 commits November 10, 2021 17:06

updated as per review comments

3008db6

Update orch.cpp

12be1fb

prsunny previously approved these changes Nov 16, 2021

View reviewed changes

dgsudharsan previously approved these changes Nov 16, 2021

View reviewed changes

anilkpandey added 2 commits November 15, 2021 23:09

Merge remote-tracking branch 'upstream/master'

d1c52aa

fix compile issue

902a621

anilkpan dismissed stale reviews from dgsudharsan and prsunny via 902a621 November 16, 2021 22:45

Merge remote-tracking branch 'upstream/master'

1479667

Merge remote-tracking branch 'upstream/master'

f038961

prsunny approved these changes Nov 20, 2021

View reviewed changes

prsunny merged commit b91d8ba into sonic-net:master Nov 20, 2021

stephenxs mentioned this pull request Nov 22, 2021

[sonic-swss] Advance submodule sonic-net/sonic-buildimage#9288

Merged

5 tasks

raphaelt-nvidia mentioned this pull request Jan 20, 2022

FDB entry not removed in case when remove port from VLAN sonic-net/sonic-buildimage#7538

Open

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

[MACsec]: Allow upgrade-docker for macsec container (sonic-net#1716)

679a4ba

dgsudharsan mentioned this pull request Apr 19, 2022

[FDB] [OA] [Warm-reboot] Cannot remove vlan since FDB Flush per port per vlan doesn't clear FDB entries sonic-net/sonic-buildimage#10605

Closed

Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025

[swss] L2 Forwarding Enhancements (sonic-net#1716)

bd67a82

* L2 Forwarding Enhancements

Conversation

dr412113 commented Apr 20, 2021

Uh oh!

lguohan commented May 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Nov 3, 2021

Uh oh!

azure-pipelines bot commented Nov 3, 2021

Uh oh!

prsunny commented Nov 4, 2021

Uh oh!

azure-pipelines bot commented Nov 4, 2021

Uh oh!

adyeung commented Nov 5, 2021

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Nov 16, 2021

Uh oh!

anilkpan commented Nov 16, 2021

Uh oh!

prsunny commented Nov 16, 2021

Uh oh!

azure-pipelines bot commented Nov 16, 2021

Uh oh!

prsunny commented Nov 16, 2021

Uh oh!