-
Notifications
You must be signed in to change notification settings - Fork 273
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
[sairedis]vs SAI support for voq neighbor #725
Changes from 1 commit
184e5f6
afd3b78
035ee0b
e06db53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,6 +154,12 @@ sai_status_t SwitchStateBase::create( | |
return createHostif(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); | ||
} | ||
|
||
|
@@ -2662,6 +2668,111 @@ 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 (encap_index == 0) | ||
{ | ||
// Encap index not provided. 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. according to:
if encap index is not provided default value is zero and not ipv4 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after if, and remove empty line 2751
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll fix them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed