Skip to content
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

[vs] Initialization of VOQ switch objects #702

Merged
merged 5 commits into from
Nov 13, 2020
Merged

[vs] Initialization of VOQ switch objects #702

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
03dbeb0
[vstest]SAI emulation for voq switch init
Nov 9, 2020
129c61f
[vs]VOQ Switch initialization
Nov 10, 2020
99189b4
Merge remote-tracking branch 'origin/master' into voq-vstest-pr6
Nov 13, 2020
259a851
[vs]VOQ Switch Initialization
Nov 13, 2020
9937954
[vs]VOQ Switch Initialization
Nov 13, 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
5 changes: 4 additions & 1 deletion vslib/inc/VirtualSwitchSaiInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,10 @@ namespace saivs
_In_ sai_object_id_t switch_id,
_In_ std::shared_ptr<SwitchConfig> config,
_In_ std::shared_ptr<WarmBootState> warmBootState,
_In_ std::weak_ptr<saimeta::Meta> meta);
_In_ std::weak_ptr<saimeta::Meta> meta,
_In_ uint32_t attr_count,
_In_ const sai_attribute_t *attr_list);

private:

static bool doesFdbEntryNotMatchFlushAttr(
Expand Down
21 changes: 13 additions & 8 deletions vslib/src/VirtualSwitchSaiInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,9 @@ std::shared_ptr<SwitchStateBase> VirtualSwitchSaiInterface::init_switch(
_In_ sai_object_id_t switch_id,
_In_ std::shared_ptr<SwitchConfig> config,
_In_ std::shared_ptr<WarmBootState> warmBootState,
_In_ std::weak_ptr<saimeta::Meta> meta)
_In_ std::weak_ptr<saimeta::Meta> meta,
_In_ uint32_t attr_count,
_In_ const sai_attribute_t *attr_list)
{
SWSS_LOG_ENTER();

Expand Down Expand Up @@ -602,6 +604,15 @@ std::shared_ptr<SwitchStateBase> VirtualSwitchSaiInterface::init_switch(
SWSS_LOG_THROW("unable to init switch %s", sai_serialize_status(status).c_str());
}

// Initialize switch for VOQ attributes

status = ss->initialize_voq_switch_objects(attr_count, attr_list);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, this looks better, but i would like you to make 1 more change, so i think it would be actually better to propagate those count/list to parameters initialize_default_objects() function, and call voq initialize inside initialize_devault_objects, this way we will have initialization part in 1 single function, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No issue in doing what you are proposing. However, here the voq objects are not defaults. They are configured attributes. Not sure if it will be clearer to have non default object initialization in function whose name indicates default object initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also see a comment "TODO" to move the calling "initialize_default_objects()" to constructor. If we modify to propagate attr_count/attr_list to this function, we may not be able move this to constructor. Is it still o.k, if I do the modification as proposed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

that move to constructor cant be moved to constructor since that is virtual function, it could be moved, but it would need to be called explicitly in each constructor, and not in SwitchStateBase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

o.k. I'll modify as proposed, then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks, this is actually interesting that some of the internal objects depends on the input attributes to create_switch function, and not actual hardware profile, this is wired to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified as proposed.


if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_THROW("VOQ switch initialization failed!");
}

SWSS_LOG_NOTICE("initialized switch %s", sai_serialize_object_id(switch_id).c_str());
}

Expand Down Expand Up @@ -652,19 +663,13 @@ sai_status_t VirtualSwitchSaiInterface::create(
}
}

auto ss = init_switch(switchId, config, warmBootState, m_meta);
auto ss = init_switch(switchId, config, warmBootState, m_meta, attr_count, attr_list);

if (!ss)
{
return SAI_STATUS_FAILURE;
}

// Initialize switch for VOQ attributes
if ((ss->initialize_voq_switch_objects(attr_count, attr_list)) != SAI_STATUS_SUCCESS)
{
SWSS_LOG_THROW("VOQ switch initialization failed!");
}

if (warmBootState != nullptr)
{
update_local_metadata(switchId);
Expand Down