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.

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

[syncd] Add workaround for snoop SAI_PORT_ATTR_SPEED #1132

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions syncd/Syncd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3044,6 +3044,24 @@ void Syncd::snoopGetResponse(
continue;
}

if (meta->objecttype == SAI_OBJECT_TYPE_PORT &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a check that this is part of the warmreboot to skip this or we can skip this completely for other boot causes (cold/fastreboot) and not causing issue for those boot cases?

Choose a reason for hiding this comment

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

I believe that the issue was that SAI_PORT_ATTR_SPEED was getting queried on a port with the port in auto negotiation enabled. I suggest that SONIC check the port's configured auto negotiation mode and query the right attribute accordingly i.e., SAI_PORT_ATTR_SPEED or SAI_PORT_ATTR_ADVERTISED_SPEED.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at this point check is not that easy, there should be no issue with other modes then warmboot

Copy link
Contributor

Choose a reason for hiding this comment

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

@prsunny can you please review this change too? If this is a blanket ignore on SPEED GET for PORT ATTR:

  1. Do you think this should be better handled at portorch level? I mean that's where the call initiates from, so ignoring from the source might be better?
  2. Do you see any concerns on ignoring this GET call for all the flows, and not just warmboot?

Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned in PR, it a workaround anyways for bcm platform. So fine with placing it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

discussed offline with Vaibhav and think this should preferably be done from orchagent side as suggested in above comments.

meta->attrid == SAI_PORT_ATTR_SPEED)
{
/*
* This is workaround. Skip port speed because of broadcom bug,
* after warm reboot SAI returns different speed then before warm
* reboot, which causes to generate SET operation on comparison
* logic and this SET operation fails. There is no SET operation
* for speed generated by orchagent, since all values are snooped
* here.
*
* TODO: This should be removed when SAI will be fixed.
*/

SWSS_LOG_INFO("skipping %s for %s", meta->attridname, strObjectId.c_str());
continue;
}

/*
* Put non readonly, and non oid attribute value to temp view.
*
Expand Down
2 changes: 1 addition & 1 deletion syncd/Syncd.h
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ namespace syncd
void sendNotifyResponse(
_In_ sai_status_t status);

private: // snoop get response oids
public: // snoop get response oids

void snoopGetResponse(
_In_ sai_object_type_t object_type,
Expand Down
1 change: 1 addition & 0 deletions unittest/syncd/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ tests_SOURCES = main.cpp \
TestFlexCounter.cpp \
TestVirtualOidTranslator.cpp \
TestNotificationQueue.cpp \
TestSyncd.cpp \
TestVendorSai.cpp

tests_CXXFLAGS = $(DBGFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS_COMMON)
Expand Down
51 changes: 51 additions & 0 deletions unittest/syncd/TestSyncd.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#include "Syncd.h"
#include "VendorSai.h"
#include "CommandLineOptions.h"
#include "CommandLineOptionsParser.h"
#include "SaiAttr.h"

#include "swss/table.h"

#include "meta/sai_serialize.h"

#include <gtest/gtest.h>

using namespace syncd;
using namespace saimeta;

TEST(Syncd, snoopGetResponse)
{
auto commandLineOptions = CommandLineOptionsParser::parseCommandLine(0, NULL);

auto vendorSai = std::make_shared<VendorSai>();

auto syncd = std::make_shared<Syncd>(vendorSai, commandLineOptions, false);

sai_attribute_t attr;

attr.id = SAI_PORT_ATTR_SPEED;
attr.value.u32 = 25000;

syncd->snoopGetResponse(
SAI_OBJECT_TYPE_PORT,
sai_serialize_object_id(0x1000000000007L),
1,
&attr);

swss::DBConnector db("ASIC_DB", 0, true);

swss::Table t(&db, "ASIC_STATE");

sai_object_meta_key_t metaKey;

metaKey.objecttype = SAI_OBJECT_TYPE_PORT;
metaKey.objectkey.key.object_id = 0x1000000000007L;

std::string key = sai_serialize_object_meta_key(metaKey);

std::string value;

bool r = t.hget(key, "SAI_PORT_ATTR_SPEED", value);

EXPECT_FALSE(r);
}