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

[sairedis]vs SAI support for voq neighbor #725

Merged
merged 4 commits into from
Dec 15, 2020
Merged

[sairedis]vs SAI support for voq neighbor #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
184e5f6
[sairedis]vs SAI support for voq neighbor
Dec 3, 2020
afd3b78
[sairedis]vs sai voq neigh support - review comments fix 1
Dec 3, 2020
035ee0b
Merge branch 'master' into vssai-voq-nbr-1
vganesan-nokia Dec 14, 2020
e06db53
[switchstate] vs sai voq neigh support - review comments fix 2
Dec 15, 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
6 changes: 6 additions & 0 deletions vslib/inc/SwitchStateBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,12 @@ namespace saivs
bool check_object_list_default_state(
_Out_ const std::vector<sai_object_id_t>& objlist);

sai_status_t createVoqSystemNeighborEntry(
_In_ const std::string &serializedObjectId,
_In_ sai_object_id_t switch_id,
_In_ uint32_t attr_count,
_In_ const sai_attribute_t *attr_list);

protected: // custom debug counter

uint32_t getNewDebugCounterIndex();
Expand Down
110 changes: 110 additions & 0 deletions vslib/src/SwitchStateBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@ sai_status_t SwitchStateBase::create(
return createMACsecSA(object_id, switch_id, attr_count, attr_list);
}

if (object_type == SAI_OBJECT_TYPE_NEIGHBOR_ENTRY && m_system_port_list.size())
{
// Neighbor entry programming for VOQ systems
return createVoqSystemNeighborEntry(serializedObjectId, switch_id, attr_count, attr_list);
}

return create_internal(object_type, serializedObjectId, switch_id, attr_count, attr_list);
}

Expand Down Expand Up @@ -2764,6 +2770,110 @@ sai_status_t SwitchStateBase::refresh_system_port_list(
return SAI_STATUS_SUCCESS;
}

sai_status_t SwitchStateBase::createVoqSystemNeighborEntry(
_In_ const std::string &serializedObjectId,
_In_ sai_object_id_t switch_id,
_In_ uint32_t attr_count,
_In_ const sai_attribute_t *attr_list)
{
SWSS_LOG_ENTER();

// For VOQ switch, encap index attribute should be set for local neighbors. For
// remote neighbors check for presence of encap index supplied from upper layer.
// And also validate isLocal and impose encap index flags in the attribute list

// Determine whether this neighbor is local neighbor or remote neighbor by checking the
// RIF id provided. If the port of the RIF is system port, the neighbor is remote neighbor

bool is_system_neigh = false;
sai_attribute_t attr;
sai_neighbor_entry_t nbr_entry;

sai_deserialize_neighbor_entry(serializedObjectId, nbr_entry);

attr.id = SAI_ROUTER_INTERFACE_ATTR_PORT_ID;

CHECK_STATUS(get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, nbr_entry.rif_id, 1, &attr));

if (objectTypeQuery(attr.value.oid) == SAI_OBJECT_TYPE_SYSTEM_PORT)
{
is_system_neigh = true;
}

uint32_t encap_index = 0;
bool impose_encap_index = false;
bool is_local = true;

for (uint32_t i = 0; i < attr_count; i++)
{
switch (attr_list[i].id)
{
case SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_INDEX:

encap_index = attr_list[i].value.u32;

break;

case SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_IMPOSE_INDEX:

impose_encap_index = attr_list[i].value.booldata;

break;

case SAI_NEIGHBOR_ENTRY_ATTR_IS_LOCAL:

is_local = attr_list[i].value.booldata;

break;

default:
break;
}
}

if (impose_encap_index && encap_index == 0)
{
SWSS_LOG_ERROR("Impose invalid encap index %d for %s neigh %s",
encap_index,
(is_system_neigh ? "VOQ" : "local"),
serializedObjectId.c_str());

return SAI_STATUS_FAILURE;
}

if (is_system_neigh && is_local)
{
SWSS_LOG_ERROR("VOQ neigh info mismatch for %s. RIF is remote. attr is_local is true", serializedObjectId.c_str());

return SAI_STATUS_FAILURE;
}

if (!is_local && encap_index == 0)
{
SWSS_LOG_ERROR("VOQ neigh info mismatch for %s. attr is_local is false. encap_index is 0", serializedObjectId.c_str());

return SAI_STATUS_FAILURE;
}

CHECK_STATUS(create_internal(SAI_OBJECT_TYPE_NEIGHBOR_ENTRY, serializedObjectId, switch_id, attr_count, attr_list));

if (impose_encap_index == false)
{
// Encap index is not imposed. Assign encap index.
// The only requirement for the encap index is it must be locally
// unique in asic. Lower 32 bits of the ip address is used as encap index

encap_index = nbr_entry.ip_address.addr.ip4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

according to:

119     /**
120      * @brief Encapsulation Index
121      *
122      * Defines the neighbor's encapsulation index
123      *
124      * @type sai_uint32_t
125      * @flags CREATE_AND_SET
126      * @default 0
127      */
128     SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_INDEX,

if encap index is not provided default value is zero and not ipv4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we create local neighbors, we either set this to 0 or do not send this attribute at all. Imposing encap index is optional for local neighbors. In SAI, when we see encap index = 0 and if the neighbor is local SAI knows that it has to assign the encap index by itself. This is what I do in here. If it was remote neighbor, encap index = 0 is an error, because for remote neighbor a valid encap index must be imposed. This is what is done in bcm SAI code, which I also emulate here.

Please also note that using last 32 bits of ip address as the encap index is vs test thing. I just wanted to have a locally unique number. We can use any mechanism to get a unique number. I just chose IP address since under normal conditions, it is expected that host ip address attached to any given asic must be unique. In actual SAI, encap index will be a unique number starting from a base number representing the FEC (used in egress encapsulation, depending on platfrom).

Copy link
Collaborator

Choose a reason for hiding this comment

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

issue i have here is that this attribute is set internally, by headers definition this value is 0, so possibly when we create this neighbor, this value will be not zero, but current sonic code will expect zero, if attribute is not provided, seems like this default value should be set to "inernal" in SAI headers ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if user will pass this attribute would it be set to user value or also assigned internally ?

Copy link
Contributor Author

@vganesan-nokia vganesan-nokia Dec 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The encap index is not set by user but assigned internally by SAI. The normal work flow is: When a neighbor is learnt, it is sent to SAI without encap index (since it is local). For VOQ switch, SAI assigns a unique encap index to the neighbor internally. The upper layer (orchagent), retrieves this encap index after neighbor is created and syncs the neighbor info including the retrieved encap index with other asics (via chassis app db in supervisor card). In other asics, this neighbor is remote neighbor. For VOQ switches, all asics should have equivalent neighbor record for every neighbor from other asics to have proper forwarding for hosts from different asics. So when an asic creates neighbor record for remote neighbors, it uses the same exact encap index synced and the encap index attribute is sent to SAI along with attributes for is local flag = false and impose index flag = true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I gave the line #2834 after I rebased to resolve conflict. The correct line number (from commit 1) is #2732.

Copy link

Choose a reason for hiding this comment

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

but when it's at line 2759, it could still mean SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_IMPOSE_INDEX is true, and doing the encap_index assignment when (encap_index == 0) is not the intended usage of the SAI API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. impose encap index == true and encap_index == 0 is invalid combination. We'll return error (based on check in line #2732) even before hitting #2759.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to check for impose encap index flag instead of ecap index value == 0 to decide on allocating unique encap index. This is to align with SAI API intended usage as suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

need fixing: opencomputeproject/SAI#1165


attr.id = SAI_NEIGHBOR_ENTRY_ATTR_ENCAP_INDEX;
attr.value.u32 = encap_index;

CHECK_STATUS(set_internal(SAI_OBJECT_TYPE_NEIGHBOR_ENTRY, serializedObjectId, &attr));
}

return SAI_STATUS_SUCCESS;
}

void SwitchStateBase::findObjects(
_In_ sai_object_type_t object_type,
_In_ const sai_attribute_t &expect,
Expand Down