-
Notifications
You must be signed in to change notification settings - Fork 558
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
[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities when Asym PFC is enabled #1360
[PFCWD + Asym PFC]: Allow PFCWD to detect PFC storm on all priorities when Asym PFC is enabled #1360
Changes from 4 commits
0f3e9f3
93fb08e
3ab862b
571e991
a6b31ea
acf21f5
0d36d37
53cf04e
a946c21
4837534
9586b6f
1e001f5
d710235
6fd9206
c7f26fc
0c720df
7990b32
13b56d8
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 |
---|---|---|
|
@@ -385,16 +385,18 @@ PfcWdLossyHandler::PfcWdLossyHandler(sai_object_id_t port, sai_object_id_t queue | |
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
uint8_t pfcMask = 0; | ||
uint8_t pfcTxMask = 0; | ||
uint8_t pfcRxMask = 0; | ||
|
||
if (!gPortsOrch->getPortPfc(port, &pfcMask)) | ||
if (!gPortsOrch->getPortPfc(port, &pfcTxMask, &pfcRxMask)) | ||
{ | ||
SWSS_LOG_ERROR("Failed to get PFC mask on port 0x%" PRIx64, port); | ||
} | ||
|
||
pfcMask = static_cast<uint8_t>(pfcMask & ~(1 << queueId)); | ||
pfcTxMask = static_cast<uint8_t>(pfcTxMask & ~(1 << queueId)); | ||
pfcRxMask = static_cast<uint8_t>(pfcRxMask & ~(1 << queueId)); | ||
|
||
if (!gPortsOrch->setPortPfc(port, pfcMask)) | ||
if (!gPortsOrch->setPortPfc(port, pfcTxMask, pfcRxMask)) | ||
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. In normal mode, when pfcwd is triggered, we reset the bit corresponding to that queueId in the 'pfcMask'. The new logic in L404 does it only for 'pfcRxMask'. In 'setPortPfc' func, the new condition in L1076 (check if txmask and rxmask are not the same in combined mode) will evaluate to true and we will return false. Also in asym mode, when any of the queues specified in the pfc_enable field of PORT_QOS_MAP is in storm, we need to disable pfc for that queue. so pfcTxMask needs to be updated for that case as well. |
||
{ | ||
SWSS_LOG_ERROR("Failed to set PFC mask on port 0x%" PRIx64, port); | ||
} | ||
|
@@ -404,16 +406,18 @@ PfcWdLossyHandler::~PfcWdLossyHandler(void) | |
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
uint8_t pfcMask = 0; | ||
uint8_t pfcTxMask = 0; | ||
uint8_t pfcRxMask = 0; | ||
|
||
if (!gPortsOrch->getPortPfc(getPort(), &pfcMask)) | ||
if (!gPortsOrch->getPortPfc(getPort(), &pfcTxMask, &pfcRxMask)) | ||
{ | ||
SWSS_LOG_ERROR("Failed to get PFC mask on port 0x%" PRIx64, getPort()); | ||
} | ||
|
||
pfcMask = static_cast<uint8_t>(pfcMask | (1 << getQueueId())); | ||
pfcTxMask = static_cast<uint8_t>(pfcTxMask | (1 << getQueueId())); | ||
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. We shouldn't be modifying Tx Mask since it contains only priorities 3 and 4. We will setting tx mask to incorrect values on pfcwd storm restore if pfcwd was triggered on a lossy priority 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. Agree, we don' need it. Fixed to modify only Rx mask. 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. Thinking about this more, we need to set the txmask but need to do it only for queues set in 'pfc_enable' field 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. Ok, I will change it back and verify |
||
pfcRxMask = static_cast<uint8_t>(pfcRxMask | (1 << getQueueId())); | ||
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 to update pfcTxMask as well in combined mode and in asym mode, update pfcTxMask if the queue is one specified in 'pfc_enable' fields of PORT_QOS_MAP |
||
|
||
if (!gPortsOrch->setPortPfc(getPort(), pfcMask)) | ||
if (!gPortsOrch->setPortPfc(getPort(), pfcTxMask, pfcRxMask)) | ||
{ | ||
SWSS_LOG_ERROR("Failed to set PFC mask on port 0x%" PRIx64, getPort()); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,13 @@ struct SystemPortInfo | |
uint32_t num_voq = 8; | ||
}; | ||
|
||
struct PfcInfo | ||
{ | ||
sai_port_priority_flow_control_mode_t pfc_mode = SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_COMBINED; | ||
uint8_t pfc_tx_bitmask = 0; | ||
uint8_t pfc_rx_bitmask = 0; | ||
}; | ||
|
||
class Port | ||
{ | ||
public: | ||
|
@@ -120,6 +127,7 @@ class Port | |
std::vector<sai_object_id_t> m_priority_group_ids; | ||
sai_port_priority_flow_control_mode_t m_pfc_asym = SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_COMBINED; | ||
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. We have two variable for save port priority flow control mode: |
||
uint8_t m_pfc_bitmask = 0; | ||
PfcInfo m_pfc_info; | ||
uint32_t m_nat_zone_id = 0; | ||
uint32_t m_vnid = VNID_NONE; | ||
uint32_t m_fdb_count = 0; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -907,7 +907,7 @@ bool PortsOrch::setPortFec(Port &port, sai_port_fec_mode_t mode) | |
return true; | ||
} | ||
|
||
bool PortsOrch::getPortPfc(sai_object_id_t portId, uint8_t *pfc_bitmask) | ||
bool PortsOrch::getPortPfc(sai_object_id_t portId, uint8_t *pfc_tx_bitmask, uint8_t *pfc_rx_bitmask) | ||
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
|
@@ -919,12 +919,13 @@ bool PortsOrch::getPortPfc(sai_object_id_t portId, uint8_t *pfc_bitmask) | |
return false; | ||
} | ||
|
||
*pfc_bitmask = p.m_pfc_bitmask; | ||
*pfc_tx_bitmask = p.m_pfc_info.pfc_tx_bitmask; | ||
*pfc_rx_bitmask = p.m_pfc_info.pfc_rx_bitmask; | ||
|
||
return true; | ||
} | ||
|
||
bool PortsOrch::setPortPfc(sai_object_id_t portId, uint8_t pfc_bitmask) | ||
bool PortsOrch::setPortPfc(sai_object_id_t portId, uint8_t pfc_tx_bitmask, uint8_t pfc_rx_bitmask) | ||
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
|
@@ -937,34 +938,60 @@ bool PortsOrch::setPortPfc(sai_object_id_t portId, uint8_t pfc_bitmask) | |
return false; | ||
} | ||
|
||
if (p.m_pfc_asym == SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_COMBINED) | ||
if (p.m_pfc_info.pfc_mode == SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_COMBINED) | ||
{ | ||
if (pfc_tx_bitmask != pfc_rx_bitmask) | ||
{ | ||
SWSS_LOG_ERROR("Cannot set unequal Tx 0x%x and Rx 0x%x PFC bitmasks " | ||
"for the port 0x%" PRIx64 " with disabled \"asymmetric\" PFC mode", | ||
pfc_tx_bitmask, pfc_rx_bitmask, portId); | ||
return false; | ||
} | ||
|
||
attr.id = SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL; | ||
attr.value.u8 = pfc_tx_bitmask; | ||
|
||
sai_status_t status = sai_port_api->set_port_attribute(portId, &attr); | ||
if (status != SAI_STATUS_SUCCESS) | ||
{ | ||
SWSS_LOG_ERROR("Failed to set PFC bitmask 0x%x for the port 0x%" PRIx64 " (rc:%d)", | ||
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. not sure i understand this part. 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. This logic is explained in the "Asymmentric PFC" HLD in the "2.2.1 PortsOrch" section |
||
attr.value.u8, portId, status); | ||
return false; | ||
} | ||
} | ||
else if (p.m_pfc_asym == SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_SEPARATE) | ||
else if (p.m_pfc_info.pfc_mode == SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_SEPARATE) | ||
{ | ||
attr.id = SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL_TX; | ||
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("Incorrect asymmetric PFC mode: %u", p.m_pfc_asym); | ||
return false; | ||
} | ||
attr.value.u8 = pfc_tx_bitmask; | ||
|
||
attr.value.u8 = pfc_bitmask; | ||
sai_status_t status = sai_port_api->set_port_attribute(portId, &attr); | ||
if (status != SAI_STATUS_SUCCESS) | ||
{ | ||
SWSS_LOG_ERROR("Failed to set Tx PFC bitmask 0x%x for the port 0x%" PRIx64 " (rc:%d)", | ||
attr.value.u8, portId, status); | ||
return false; | ||
} | ||
|
||
sai_status_t status = sai_port_api->set_port_attribute(portId, &attr); | ||
if (status != SAI_STATUS_SUCCESS) | ||
attr.id = SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL_RX; | ||
attr.value.u8 = pfc_rx_bitmask; | ||
|
||
status = sai_port_api->set_port_attribute(portId, &attr); | ||
if (status != SAI_STATUS_SUCCESS) | ||
{ | ||
SWSS_LOG_ERROR("Failed to set Rx PFC bitmask 0x%x for the port 0x%" PRIx64 " (rc:%d)", | ||
attr.value.u8, portId, status); | ||
return false; | ||
} | ||
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("Failed to set PFC 0x%x to port id 0x%" PRIx64 " (rc:%d)", attr.value.u8, portId, status); | ||
SWSS_LOG_ERROR("Incorrect PFC mode %u for the port 0x%" PRIx64, p.m_pfc_info.pfc_mode, portId); | ||
return false; | ||
} | ||
|
||
if (p.m_pfc_bitmask != pfc_bitmask) | ||
{ | ||
p.m_pfc_bitmask = pfc_bitmask; | ||
m_portList[p.m_alias] = p; | ||
} | ||
p.m_pfc_info.pfc_tx_bitmask = pfc_tx_bitmask; | ||
p.m_pfc_info.pfc_rx_bitmask = pfc_rx_bitmask; | ||
m_portList[p.m_alias] = p; | ||
|
||
return true; | ||
} | ||
|
@@ -974,59 +1001,57 @@ bool PortsOrch::setPortPfcAsym(Port &port, string pfc_asym) | |
SWSS_LOG_ENTER(); | ||
|
||
sai_attribute_t attr; | ||
uint8_t pfc = 0; | ||
uint8_t pfc_tx_bitmask = 0; | ||
uint8_t pfc_rx_bitmask = 0; | ||
|
||
if (!getPortPfc(port.m_port_id, &pfc)) | ||
if (pfc_asym_map.find(pfc_asym) == pfc_asym_map.end()) | ||
{ | ||
SWSS_LOG_ERROR("Incorrect asymmetric PFC mode: %s", pfc_asym.c_str()); | ||
return false; | ||
} | ||
|
||
auto found = pfc_asym_map.find(pfc_asym); | ||
if (found == pfc_asym_map.end()) | ||
if (port.m_pfc_info.pfc_mode == pfc_asym_map[pfc_asym]) | ||
{ | ||
SWSS_LOG_NOTICE("Asymmetric PFC mode is alredy set to \"%s\" for the port 0x%" PRIx64, | ||
neethajohn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
pfc_asym.c_str(), port.m_port_id); | ||
return true; | ||
} | ||
|
||
if (!getPortPfc(port.m_port_id, &pfc_tx_bitmask, &pfc_rx_bitmask)) | ||
{ | ||
SWSS_LOG_ERROR("Incorrect asymmetric PFC mode: %s", pfc_asym.c_str()); | ||
return false; | ||
} | ||
|
||
auto new_pfc_asym = found->second; | ||
if (port.m_pfc_asym == new_pfc_asym) | ||
if (pfc_asym_map[pfc_asym] == SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_SEPARATE) | ||
{ | ||
SWSS_LOG_NOTICE("Already set asymmetric PFC mode: %s", pfc_asym.c_str()); | ||
return true; | ||
pfc_rx_bitmask = static_cast<uint8_t>(0xff); | ||
} | ||
else | ||
{ | ||
pfc_rx_bitmask = pfc_tx_bitmask; | ||
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. logic has changed, can you elaborate it in the code? 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. Logic is exactly the same but code was refactored according to changes for set/getPortPfc API. |
||
} | ||
|
||
port.m_pfc_asym = new_pfc_asym; | ||
port.m_pfc_info.pfc_mode = pfc_asym_map[pfc_asym]; | ||
m_portList[port.m_alias] = port; | ||
|
||
attr.id = SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL_MODE; | ||
attr.value.s32 = (int32_t) port.m_pfc_asym; | ||
attr.value.s32 = static_cast<int32_t>(port.m_pfc_info.pfc_mode); | ||
|
||
sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr); | ||
if (status != SAI_STATUS_SUCCESS) | ||
{ | ||
SWSS_LOG_ERROR("Failed to set PFC mode %d to port id 0x%" PRIx64 " (rc:%d)", port.m_pfc_asym, port.m_port_id, status); | ||
SWSS_LOG_ERROR("Failed to set PFC mode %d for port 0x%" PRIx64 " (rc:%d)", | ||
port.m_pfc_info.pfc_mode, port.m_port_id, status); | ||
return false; | ||
} | ||
|
||
if (!setPortPfc(port.m_port_id, pfc)) | ||
if (!setPortPfc(port.m_port_id, pfc_tx_bitmask, pfc_rx_bitmask)) | ||
{ | ||
return false; | ||
} | ||
|
||
if (port.m_pfc_asym == SAI_PORT_PRIORITY_FLOW_CONTROL_MODE_SEPARATE) | ||
{ | ||
attr.id = SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL_RX; | ||
attr.value.u8 = static_cast<uint8_t>(0xff); | ||
|
||
sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr); | ||
if (status != SAI_STATUS_SUCCESS) | ||
{ | ||
SWSS_LOG_ERROR("Failed to set RX PFC 0x%x to port id 0x%" PRIx64 " (rc:%d)", attr.value.u8, port.m_port_id, status); | ||
return false; | ||
} | ||
} | ||
|
||
SWSS_LOG_INFO("Set asymmetric PFC %s to port id 0x%" PRIx64, pfc_asym.c_str(), port.m_port_id); | ||
SWSS_LOG_INFO("Set asymmetric PFC mode to \"%s\" for the port id 0x%" PRIx64, | ||
pfc_asym.c_str(), port.m_port_id); | ||
|
||
return true; | ||
} | ||
|
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.
Why do we need to modify Tx Mask? In any case, its a noop if pfcwd is triggered on a lossy priority because the txMask only contains priorities 3 and 4
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.
Agree, we don' need it. Fixed to modify only Rx mask.