Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 26, 2018
Merged

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

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3bf6269
Merge pull request #1 from Azure/master
keboliu Jan 12, 2018
17c4235
add code to support clear fdb cli
keboliu Jan 12, 2018
7a220af
remove redundant code and revise the error log
keboliu Jan 16, 2018
d2f43c5
trigger the fdb flush from orchagent
keboliu Jan 24, 2018
5eca26e
remove unnecessary attr list
keboliu Jan 25, 2018
6fe005e
Merge branch 'master' into clear-fdb-cli
keboliu Jan 25, 2018
af8c2b1
Merge branch 'master' into clear-fdb-cli
lguohan Jan 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 51 additions & 6 deletions orchagent/fdborch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,66 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj

(void)m_entries.insert(update.entry);
SWSS_LOG_DEBUG("FdbOrch notification: mac %s was inserted into vlan %d", update.entry.mac.to_string().c_str(), entry->vlan_id);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


break;

case SAI_FDB_EVENT_AGED:
case SAI_FDB_EVENT_FLUSHED:
case SAI_FDB_EVENT_MOVE:
update.add = false;

(void)m_entries.erase(update.entry);
SWSS_LOG_DEBUG("FdbOrch notification: mac %s was removed from vlan %d", update.entry.mac.to_string().c_str(), entry->vlan_id);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

}

break;

case SAI_FDB_EVENT_FLUSHED:
if( !bridge_port_id && !(entry->vlan_id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parenthesis around entry->vlan_id are redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

{
for(set<FdbEntry>::iterator itr = m_entries.begin(); itr != m_entries.end(); ++itr )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (auto itr = ....)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

{
/*This is a flush all case, need to clear up all the fdb entries*/
update.entry.mac = itr->mac;
update.entry.vlan = itr->vlan;
update.add = false;

m_entries.erase(itr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


SWSS_LOG_DEBUG("FdbOrch notification: mac %s was removed", update.entry.mac.to_string().c_str());

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

}
}
}
else if(bridge_port_id && !(entry->vlan_id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundand parenthesis around entry->vlan_id

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

{
/*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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundand parenthesis around entry->vlan_id

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised.

{
/*this is a placeholder for flush vlan fdb case, not supported yet.*/
SWSS_LOG_ERROR("FdbOrch notification: not supported flush type");
}
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
SWSS_LOG_ERROR("FdbOrch notification: not supported flush type");
}
break;
}

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

return;
}

void FdbOrch::update(SubjectType type, void *cntx)
Expand Down