[fdborch]Add the support for a clear fdb cli #426

keboliu · 2018-01-12T11:52:44Z

What I did
revise the FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id) function to support a clear fdb cli

Why I did it
the current implementation is assuming that SAI will send all the flushed fdb entries to upper layers, but actually SAI only send one event to indicate the flush type, like flush all, flush fdb entries in certain port or certain vlan. upper layer need to find out all the flushed fdb entries by itself and then update it.

How I verified it

tested on MLNX plat form with various flush fdb cases.

Details if related
design doc see https://github.com/Azure/SONiC/wiki/SONiC-Clear-FDB-CLI-Design

merge from Azure/sonic-swss master

pavel-shirshov

As commented

pavel-shirshov · 2018-01-12T22:11:56Z

orchagent/fdborch.cpp

+
+        for (auto observer: m_observers)
+        {
+            observer->update(SUBJECT_TYPE_FDB_CHANGE, static_cast<void *>(&update));


you can cast to void pointer without any cast mechanisms.
Check https://stackoverflow.com/questions/18929225/casting-class-pointer-to-void-pointer
So
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update); is enough

pavel-shirshov · 2018-01-12T22:14:00Z

orchagent/fdborch.cpp

+
+        for (auto observer: m_observers)
+        {
+            observer->update(SUBJECT_TYPE_FDB_CHANGE, static_cast<void *>(&update));


you can cast to void pointer without any cast mechanisms.
Check https://stackoverflow.com/questions/18929225/casting-class-pointer-to-void-pointer
So
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update); is enough

pavel-shirshov · 2018-01-12T22:14:38Z

orchagent/fdborch.cpp

+        break;
+
+    case SAI_FDB_EVENT_FLUSHED:
+        if( !bridge_port_id && !(entry->vlan_id))


Parenthesis around entry->vlan_id are redundant

pavel-shirshov · 2018-01-12T22:16:43Z

orchagent/fdborch.cpp

+
+                for (auto observer: m_observers)
+                {
+                    observer->update(SUBJECT_TYPE_FDB_CHANGE, static_cast<void *>(&update));


you can cast to void pointer without any cast mechanisms.
Check https://stackoverflow.com/questions/18929225/casting-class-pointer-to-void-pointer
So
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update); is enough

pavel-shirshov · 2018-01-12T22:17:10Z

orchagent/fdborch.cpp

+                }
+            }
+        }
+        else if(bridge_port_id && !(entry->vlan_id))


redundand parenthesis around entry->vlan_id

pavel-shirshov · 2018-01-12T22:17:38Z

orchagent/fdborch.cpp

+            /*this is a placeholder for flush port fdb case, not supported yet.*/
+            SWSS_LOG_ERROR("FdbOrch notification: not supported flush type");
+        }
+        else if(!bridge_port_id && (entry->vlan_id))


redundand parenthesis around entry->vlan_id

pavel-shirshov · 2018-01-12T22:22:23Z

orchagent/fdborch.cpp

+    case SAI_FDB_EVENT_FLUSHED:
+        if( !bridge_port_id && !(entry->vlan_id))
+        {
+            for(set<FdbEntry>::iterator itr = m_entries.begin(); itr != m_entries.end(); ++itr )


for (auto itr = ....)

lguohan · 2018-01-16T06:07:19Z

orchagent/fdborch.cpp

+        break;
+
+    case SAI_FDB_EVENT_FLUSHED:
+        if( !bridge_port_id && !entry->vlan_id)


please follow existing code convention for the whitespaces.

change to bridge_port_id == SAI_NULL_OBJECT_ID

lguohan · 2018-01-16T06:09:09Z

orchagent/fdborch.cpp

+        for (auto observer: m_observers)
+        {
+            observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
+        }


this is about the fdb flush, why touch both LEARNED, AGED, MOVE events?

separate the handling FLUSH from AGED and MOVE, at the same time remove unnecessary type cast. No impaction to the handling of other events.

lguohan · 2018-01-16T06:10:50Z

orchagent/fdborch.cpp

+    case SAI_FDB_EVENT_FLUSHED:
+        if( !bridge_port_id && !entry->vlan_id)
+        {
+            for(auto itr = m_entries.begin(); itr != m_entries.end(); ++itr )


keep white space convention as existing code, thanks.

lguohan · 2018-01-16T06:24:19Z

orchagent/fdborch.cpp

+            /*this is a placeholder for flush vlan fdb case, not supported yet.*/
+            SWSS_LOG_ERROR("FdbOrch notification: not supported flush vlan fdb action, port_id = %lu, vlan_id = %d.", bridge_port_id, entry->vlan_id);
+        }
+        else


@prsunny , in case the fdb flush will send notification of deleted mac one-by-one, does it fall into this category?

Observed in testing that per-entry delete is coming with event type SAI_FDB_EVENT_AGED instead of event FLUSHED, which is handled in the above case. Also, I think, deleting mac one-by-one doesn't fall into this category as there could be multiple mac entries learnt on the same port-vlan combination

prsunny · 2018-01-18T01:08:03Z

orchagent/fdborch.cpp

+                update.entry.vlan = itr->vlan;
+                update.add = false;
+
+                m_entries.erase(itr);


Looks to me as a non-standard way of deleting the elements of a set in for-loop as it invalidates the iterator after erase. It might have worked in test. Can you check this?

Will check. and I am revising the solution as Guohan suggested, will update all of these with the new solution.

prsunny · 2018-01-18T01:33:46Z

orchagent/fdborch.cpp

+            /*this is a placeholder for flush vlan fdb case, not supported yet.*/
+            SWSS_LOG_ERROR("FdbOrch notification: not supported flush vlan fdb action, port_id = %lu, vlan_id = %d.", bridge_port_id, entry->vlan_id);
+        }
+        else


Observed in testing that per-entry delete is coming with event type SAI_FDB_EVENT_AGED instead of event FLUSHED, which is handled in the above case. Also, I think, deleting mac one-by-one doesn't fall into this category as there could be multiple mac entries learnt on the same port-vlan combination

kcudnik · 2018-01-24T15:33:11Z

orchagent/fdborch.cpp

+        attr.id = SAI_FDB_FLUSH_ATTR_ENTRY_TYPE;
+        attr.value.s32 = SAI_FDB_FLUSH_ENTRY_TYPE_DYNAMIC;
+
+        status = sai_fdb_api->flush_fdb_entries(gSwitchId, 1, &attr);


when flushing all, there should be 0 attributes passed, all static and dynamic entries should be cleared

Signed-off-by: Wataru Ishida <ishida@nel-america.com>

keboliu added 2 commits January 12, 2018 11:39

Merge pull request #1 from Azure/master

3bf6269

merge from Azure/sonic-swss master

add code to support clear fdb cli

17c4235

keboliu changed the title ~~[orchagent]Add the support for a clear fdb cli~~ [fdborch]Add the support for a clear fdb cli Jan 12, 2018

pavel-shirshov suggested changes Jan 12, 2018

View reviewed changes

remove redundant code and revise the error log

7a220af

lguohan reviewed Jan 16, 2018

View reviewed changes

prsunny reviewed Jan 18, 2018

View reviewed changes

trigger the fdb flush from orchagent

d2f43c5

kcudnik reviewed Jan 24, 2018

View reviewed changes

keboliu added 2 commits January 25, 2018 07:38

remove unnecessary attr list

5eca26e

Merge branch 'master' into clear-fdb-cli

6fe005e

kcudnik approved these changes Jan 25, 2018

View reviewed changes

prsunny approved these changes Jan 25, 2018

View reviewed changes

lguohan approved these changes Jan 25, 2018

View reviewed changes

pavel-shirshov approved these changes Jan 26, 2018

View reviewed changes

Merge branch 'master' into clear-fdb-cli

af8c2b1

lguohan merged commit 918cf5a into sonic-net:master Jan 26, 2018

keboliu deleted the clear-fdb-cli branch January 27, 2018 05:12

keboliu mentioned this pull request Jan 30, 2018

update the pointers to pick up clear fdb cli support sonic-net/sonic-buildimage#1354

Merged

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

avoid using pkg_resources for faster command execution (sonic-net#426)

da3934e

Signed-off-by: Wataru Ishida <ishida@nel-america.com>

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

Fix compilation issues in stretch docker with gcc-6.3 (sonic-net#426)

da35224

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fdborch]Add the support for a clear fdb cli #426

[fdborch]Add the support for a clear fdb cli #426

keboliu commented Jan 12, 2018

pavel-shirshov left a comment

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

pavel-shirshov Jan 12, 2018

keboliu Jan 16, 2018

lguohan Jan 16, 2018

lguohan Jan 16, 2018

lguohan Jan 16, 2018

keboliu Jan 25, 2018

lguohan Jan 16, 2018

lguohan Jan 16, 2018

prsunny Jan 18, 2018

prsunny Jan 18, 2018

keboliu Jan 18, 2018

prsunny Jan 18, 2018

kcudnik Jan 24, 2018

keboliu Jan 25, 2018

[fdborch]Add the support for a clear fdb cli #426

[fdborch]Add the support for a clear fdb cli #426

Conversation

keboliu commented Jan 12, 2018

pavel-shirshov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment