Skip to content

[meta] Flush fdb entries after flush api success #581

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
qiluo-msft merged 3 commits into from
Apr 1, 2020
Merged

[meta] Flush fdb entries after flush api success #581

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1735f87
[meta] Flush fdb entries after flush api success
kcudnik Mar 25, 2020
5587aaf
[meta] Use correct fdb entry type
kcudnik Mar 26, 2020
9338ff8
[meta] Use correct enum values for fdb entry
kcudnik Mar 26, 2020
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
27 changes: 25 additions & 2 deletions meta/Meta.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1683,7 +1683,29 @@ sai_status_t Meta::flushFdbEntries(

if (status == SAI_STATUS_SUCCESS)
{
SWSS_LOG_WARN("TODO, remove all matching fdb entries here, currently removed in FDB notification");
// use same logic as notification, so create notification event

sai_fdb_event_notification_data_t data = {};

auto *bv_id = sai_metadata_get_attr_by_id(SAI_FDB_FLUSH_ATTR_BV_ID, attr_count, attr_list);
auto *bp_id = sai_metadata_get_attr_by_id(SAI_FDB_FLUSH_ATTR_BRIDGE_PORT_ID, attr_count, attr_list);
auto *et = sai_metadata_get_attr_by_id(SAI_FDB_FLUSH_ATTR_ENTRY_TYPE, attr_count, attr_list);

sai_attribute_t list[2];

list[0].id = SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID;
list[0].value.oid = bp_id ? bp_id->value.oid : SAI_NULL_OBJECT_ID;

list[1].id = SAI_FDB_ENTRY_ATTR_TYPE;
list[1].value.s32 = et ? et->value.s32 : SAI_FDB_FLUSH_ENTRY_TYPE_ALL;

data.event_type = SAI_FDB_EVENT_FLUSHED;
data.fdb_entry.switch_id = switch_id;
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should we have fdb_flush_entry as well in sai_fdb_event_notification_data_t and use it when event is SAI_FDB_EVENT_FLUSHED. Otherwise, we are converting FDB_FLUSH_ENTRY attributes to FDB_ENTRY attributes and in-fact if you look at the second attribute here, attribute type is SAI_FDB_ENTRY_ATTR_TYPE and value is SAI_FDB_FLUSH_ENTRY_TYPE_ALL. what do you say?

Copy link
Copy Markdown
Collaborator Author

@kcudnik kcudnik Mar 26, 2020

Choose a reason for hiding this comment

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

yes, already addressed in below commit

data.fdb_entry.bv_id = (bv_id) ? bv_id->value.oid : SAI_NULL_OBJECT_ID;
data.attr_count = 2;
data.attr = list;

meta_sai_on_fdb_flush_event_consolidated(data);
}

return status;
Expand Down Expand Up @@ -6954,7 +6976,8 @@ void Meta::meta_sai_on_fdb_flush_event_consolidated(
continue;
}

if (fdbTypeAttr->getSaiAttr()->value.s32 != type->value.s32)
if ((type->value.s32 != SAI_FDB_FLUSH_ENTRY_TYPE_ALL) &&
Copy link
Copy Markdown
Contributor

@vasant17 vasant17 Mar 26, 2020

Choose a reason for hiding this comment

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

How about this?

if (type->value.s32 == SAI_FDB_FLUSH_ENTRY_TYPE_ALL) {
    // add fdb to remove vector and continue;
}

// check type match
// check bridge_port_id matches
// check vlan ID matches

If you dont like this, then when someone specified type == SAI_FDB_FLUSH_ENTRY_TYPE_ALL, and also provides vlan_id or bridge_port_id or both, we are NOT going to flush all. Is that the expected behavior? Or we should honor the type attribute?

Copy link
Copy Markdown
Contributor

@vasant17 vasant17 Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Also, what do we print on line number 6963 when value is SAI_FDB_FLUSH_ENTRY_TYPE_ALL? Because we care casting from FLUSH_ENTRY_TYPE to FDB_ENTRY_TYPE enum

Copy link
Copy Markdown
Collaborator Author

@kcudnik kcudnik Mar 26, 2020

Choose a reason for hiding this comment

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

cant be like that , since if it's ALL (static and dynamic, other attributes also needs to be checked, like vlan id, or bridge port, ALL here appllies only to static or dynamic

Copy link
Copy Markdown
Contributor

@vasant17 vasant17 Mar 26, 2020

Choose a reason for hiding this comment

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

Got it. SAI_FDB_FLUSH_ENTRY_TYPE_ALL means BOTH(STATIC and DYNAMIC) type! NOT really ALL FDB entries!

Copy link
Copy Markdown
Collaborator Author

@kcudnik kcudnik Mar 26, 2020

Choose a reason for hiding this comment

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

there are no other entries except static and dynamic, so ALL means static and dynamic together

(fdbTypeAttr->getSaiAttr()->value.s32 != type->value.s32))
{
// entry type is not matching on this fdb entry
continue;
Expand Down