[202012][MuxOrch] disable pfcwd when switching over #2499
Conversation
Ndancejic
force-pushed
the
pfc_switchover
branch
from
00011a1 to
32df126
Compare
|
|
||
| // Disable PfcWd on this port | ||
| PfcWdOrch<DropHandler, ForwardHandler>::deleteEntry(port.m_alias); | ||
| return true; |
There was a problem hiding this comment.
Why return here? why don't walk through all queues?
There was a problem hiding this comment.
yeah I may need to add onto the local table I'm using to store configurations for all queues then. I'll have to review this PR again since it's been a while. I'll look into that though.
| } | ||
|
|
||
| SWSS_LOG_INFO("Starting Wd on port %s", port.m_alias.c_str()); | ||
| return PfcWdOrch<DropHandler, ForwardHandler>::createEntry(port.m_alias, pos->second); |
There was a problem hiding this comment.
The PFC_WD_DETECTION_TIME and PFC_WD_RESTORATION_TIME are divided by 1000 when saving into m_pfcwdconfigs. Should multi by 1000 here?
There was a problem hiding this comment.
I'll check again, but back when I was testing this it was giving me value out of range if I multiplied by 1000 again
|
|
||
| vector<FieldValueTuple> configs; | ||
| configs.push_back(FieldValueTuple(PFC_WD_DETECTION_TIME, to_string(detection/1000))); | ||
| configs.push_back(FieldValueTuple(PFC_WD_RESTORATION_TIME, to_string(restoration/1000))); |
There was a problem hiding this comment.
Why do we need to divide 1000? If the detection time is 200ms, what value will be saved?
There was a problem hiding this comment.
the value we get from PFC_WD_DETECTION_TIME seems to be 200000 from what I saw. I'll double check this again to make sure.
| @@ -1495,6 +1498,10 @@ bool MuxCableOrch::addOperation(const Request& request) | |||
| auto state = request.getAttrString("state"); | |||
| auto mux_obj = mux_orch->getMuxCable(port_name); | |||
|
|
|||
| /* Disable Pfc Watchdog for perfomance */ | |||
| SWSS_LOG_INFO("Disabling PFCWD for Mux Switchover on %s", port_name.c_str()); | |||
| gPfcWdSwOrch->stopWdOnAllPorts(); | |||
There was a problem hiding this comment.
I'm a little confused here. The mux cable state is per port, why we disable watchdog on all ports?
There was a problem hiding this comment.
We want to prevent polling from interrupting MUX switchover. since polling is asynchronous, a poll from any port could interrupt the thread in the middle of switchover
| /* Disable Pfc Watchdog for perfomance */ | ||
| SWSS_LOG_INFO("Disabling PFCWD for Mux Switchover on %s", portName.c_str()); | ||
| gPfcWdSwOrch->stopWdOnAllPorts(); | ||
|
|
There was a problem hiding this comment.
Why do we need to disable first, and then enable again? Is it better if we cherk the incoming muxstate first?
And same question here, why do we disable/enable watchdog on all ports?
There was a problem hiding this comment.
That's a good point, I'll make sure this only happens if we have a valid switchover happening
Why I did it: to improve switchover performance How I did it: Added stopPFCWdOnAllPorts and startPFCWdOnAllPorts function in PFCWdSwOrch. On each switchover, PFC Wd is stopped unless there is a storm currently on the port. Then when switchover is completed, PFC Wd is enabled again How to test: $ pytest test_mux.py::TestMuxTunnel::test_mux_pfcwd_switching Signed-off-by: Nikola Dancejic <[email protected]>
Ndancejic
force-pushed
the
pfc_switchover
branch
from
32df126 to
48eb600
Compare
Why I did it:
to improve switchover performance
How I did it:
Added stopPFCWdOnAllPorts and startPFCWdOnAllPorts function in PFCWdSwOrch. On each switchover, PFC Wd is stopped unless there is a storm currently on the port. Then when switchover is completed, PFC Wd is enabled again
How to test:
$ pytest test_mux.py::TestMuxTunnel::test_mux_pfcwd_switching
Signed-off-by: Nikola Dancejic [email protected]
What I did
Why I did it
How I verified it
Details if related