Skip to content

Check MDIO server thread joinable before join the thread#1342

Merged
kcudnik merged 4 commits intosonic-net:masterfrom
jiahua-wang:mdio_thread
Mar 7, 2024
Merged

Check MDIO server thread joinable before join the thread#1342
kcudnik merged 4 commits intosonic-net:masterfrom
jiahua-wang:mdio_thread

Conversation

@jiahua-wang
Copy link
Contributor

Signed-off-by: Jiahua Wang <jiahua.wang@broadcom.com>
@kcudnik
Copy link
Collaborator

kcudnik commented Jan 25, 2024

Is the root case already known? Was it double join?

@jiahua-wang
Copy link
Contributor Author

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.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 26, 2024

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 ?

@kenneth-arista
Copy link

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.

@kenneth-arista
Copy link

I recall that a syncd crash was observed once during sonic-mgmt override_config_table tests.

@jiahua-wang
Copy link
Contributor Author

This patch will address both cases of double join and thread not starting properly.

@rlhui
Copy link

rlhui commented Feb 21, 2024

@jiahua-wang - please update the branch? thanks

@xumia
Copy link
Collaborator

xumia commented Feb 22, 2024

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jiahua-wang
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1342 in repo sonic-net/sonic-sairedis

@jiahua-wang
Copy link
Contributor Author

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1342 in repo sonic-net/sonic-sairedis

@judyjoseph
Copy link
Contributor

@kcudnik Can we merge this PR

@judyjoseph
Copy link
Contributor

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jiahua-wang
Copy link
Contributor Author

@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?

@kcudnik
Copy link
Collaborator

kcudnik commented Mar 7, 2024

All passed do you want me to merge?

@jiahua-wang
Copy link
Contributor Author

@kcudnik Please merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants