-
Notifications
You must be signed in to change notification settings - Fork 287
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
Check MDIO server thread joinable before join the thread #1342
Conversation
Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>
Is the root case already known? Was it double join? |
Double join could be an explanation. More likely scenario is that m_taskThread is not joinable because after MdioIpcServer constructor startMdioThread() was not called or because startMdioThread() failed. @kenneth-arista can provide more test details. |
Can you investigate which exactly scenario is that ? |
Unfortunately, I don't have all the logs when the problem occurred before integrating Jiahua's patch. Prior to the patch, I would often seen several syncd core files after each full sonic-mgmt test runs. I suspect these crashes are triggered in tests involving reboot. |
I recall that a syncd crash was observed once during sonic-mgmt |
This patch will address both cases of double join and thread not starting properly. |
@jiahua-wang - please update the branch? thanks |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Commenter does not have sufficient privileges for PR 1342 in repo sonic-net/sonic-sairedis |
/AzurePipelines run Azure.sonic-sairedis |
Commenter does not have sufficient privileges for PR 1342 in repo sonic-net/sonic-sairedis |
@kcudnik Can we merge this PR |
/AzurePipelines run Azure.sonic-sairedis |
Azure Pipelines successfully started running 1 pipeline(s). |
@kcudnik @rlhui @judyjoseph All checks were passed when I created the PR for the first time. After I merged other commits, 2 checks always fail at the place not related to my code change. Should I close this one and create another one with the same code change? |
All passed do you want me to merge? |
@kcudnik Please merge. |
This PR is to address the issue syncd crash observed during sonic-mgmt reboot tests #17346