Address racey SAI PHY Serdes object causing bad syncd setup#1945
Address racey SAI PHY Serdes object causing bad syncd setup#1945justin-wong-ce wants to merge 11 commits into
Conversation
Mitigate a race condition in Port PHY Serdes initialization (syncd) where the SAI is still busy with the object from its creation when syncd's intialization attempts to read attributes from the object for syncd's own setup. Do this by allowing 3 retries with 10ms wait intervals in between tries. The retry will only trigger when the failure reason is SAI_STATUS_OBJECT_IN_USE. Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| port_rid = attr.value.oid; | ||
| return true; | ||
| } | ||
| sai_status_t status = Base::m_vendorSai->get(Base::m_objectType, port_serdes_rid, 1, &attr); |
There was a problem hiding this comment.
hey Justin - the fix is a bit of "by-chance".
Also supposedly,
- flex counter should not fail syncd init
- flex counter should work after the objects gets created
so I wonder if we have looked deeper into the failure/race and see what might be happening?
There was a problem hiding this comment.
As mentioned above, the race condition is from the SAI not being able to handle the Port PHY Serdes object creation and accessing of its attribute in extremely quick succession.
Either the SAI needs to be able to delay a request and not return SAI_STATUS_OBJECT_IN_USE, or Sonic (syncd initialization for this case) needs to be able to handle SAI_STATUS_OBJECT_IN_USE properly.
the fix is a bit of "by-chance"
I can change the fix such that it will retry until SAI does not return the SAI_STATUS_OBJECT_IN_USE, rather than just 3 times - this will remove the chance element.
I think it is reasonable to retry when the SAI returns SAI_STATUS_OBJECT_IN_USE since the object is busy and not available for read.
There was a problem hiding this comment.
Please let me know what you think of the latest changes
There was a problem hiding this comment.
what if we just simply skip the failed round and retry in the next pulling interval?
There was a problem hiding this comment.
retrying 3 times is not going to guarantee the fix anyway.
There was a problem hiding this comment.
This error is occuring during the initialization stage and this code that is being edited is for the initialization of Port PHY Serdes.
This will print the following ERR syslog once - during syncd's intialization:
30979 2026 Jun 7 20:56:03.499522 qspf201 ERR syncd#syncd: :- initAttrData: PORT_PHY_SERDES_ATTR: port_serdes_rid:0xb005700000000 has no serdes count attribute information
30980 2026 Jun 7 20:56:03.499522 qspf201 WARNING syncd#syncd: :- collectData: PORT_PHY_SERDES_ATTR: Failed to initialize attribute SAI_PORT_SERDES_ATTR_TX_FIR_TAPS_LIST for RID:0xb005700000000, skipping object
If SAI_STATUS_OBJECT_IN_USE is returned here and we don't retry, this will result missing info (maps) that is used to do other initalization steps later on, as it is treated as a failure.
If the info is missing, it will cause polling to fail all the time.
The following ERR syslog will print at every single poll interval:
26929 2026 Jun 7 20:55:59.007758 qspf201 ERR syncd#syncd: [none] SAI_API_PORT:brcm_sai_get_port_serdes_attribute_cmn:18248 Port 11 hardware lane count get failed with error -17.
26930 2026 Jun 7 20:55:59.007758 qspf201 ERR syncd#syncd: :- updatePortSerdesTapsCountMap: PORT_PHY_SERDES_ATTR: Failed to get port serdes count attr SAI_PORT_SERDES_ATTR_TX_FIR_COUNT for port_serdes RID:0xb005700000000, status:-17
There was a problem hiding this comment.
@justin-wong-ce i see you have removed the retry loop, I dont think this is a right approach.
We should not retry without an upper bound. If we believe a retry of 3 times with a 10ms is not enough we can increase this to 5 retries but we should not leave it unbounded.
@r12f fyi.
There was a problem hiding this comment.
@r12f please advise whether if we can go forward with 5 retries
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @justin-wong-ce , can we get this PR merged? Are there any remaining blockers? This issue is the biggest source of loganalyzer teardown failures across our SONiC 202511 hardware qualification on Broadcom TH5 (Arista 7060X6-512). We're seeing the same periodic ERR from initAttrData (port_serdes_rid:0x1005700000000 has no lane count information) every 10 second. |
I am awaiting feedback in this thread: #1945 (comment) I do not have merge permissions. To pre-emptively resolve any potential code coerage issues I am working on a unit test at the moment. |
Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Note: I am aware of the cherry-pick conflict to 202511. I have a local branch that addresses this conflict ready and will make a direct backport when this PR is approved. |
| flexCounter->removeCounter(testPortSerdesOid); | ||
| } | ||
|
|
||
| TEST_F(TestPortPhySerdesAttr, RetryOnObjectInUseThenSucceed) |
Signed-off-by: Justin Wong <jvwong@arista.com>
3ac7f55 to
a33c0e8
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@justin-wong-ce - are these failure logs only seen on a specific platform? |
I do not have access to other platforms - I can only test on Broadcom. I cannot say for sure if it happens on other platforms so I only noted |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
23718ac to
8aa4ae9
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Restarting CI due to failure from: |
|
/azpw retry |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1164893: ✅Stage Test:
|
|
hi @tjchadaga - do you mind to help check this PR? |
Description of PR
Summary:
Mitigate a race condition in Port PHY Serdes initialization (
syncd) where the SAI is still busy with the object from its creation whensyncd's intialization attempts to read attributes from the object for syncd's own setup.This race condition will cause a failing SAI call that is needed for initializing
syncd, causing polls to the Port PHY Serdes object to fail continuously later on. The chances of this happening is around ~2.5% perconfig reload/reboot.Do this by allowing 3 retries with 10ms wait intervals in between tries. The retry will only trigger when the failure reason is
SAI_STATUS_OBJECT_IN_USE.Fixes #1946
Type of change
Approach
What is the motivation for this PR?
Fix syslog ERRs that logs continuously when this scenario occurs.
Work item tracking
How did you do it?
Added multiple attempts for interacing with Port PHY Serdes object over SAI during setup.
How did you verify/test it?
Continuous syslog ERRs no longer occur. The retry logic can be seen working in the syslogs as well. The most retry attempts observed is 1.
Any platform specific information?
Broadcom.
Documentation