Skip to content
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

[mclagsyncd] fix out of order initialization #2112

Conversation

globaltrouble
Copy link
Contributor

What I did

Reorder class member variables.

Why I did it

I've encountered an issue with m_bufSize, it's initialization is random, some times it may be initializied with 0, some times with random value.

m_bufSize use MSG_BATCH_SIZE for initialization:

MclagLink::MclagLink(Select *select, int port) :
    MSG_BATCH_SIZE(256),
    m_bufSize(MCLAG_MAX_MSG_LEN * MSG_BATCH_SIZE),
    ...

but MSG_BATCH_SIZE declared after m_bufSize

class MclagLink : public Selectable {
  private:
    unsigned int m_bufSize;
    ...
    const int MSG_BATCH_SIZE;
    ...

so MSG_BATCH_SIZE will be initialized after m_bufSize, and m_bufSize will use MSG_BATCH_SIZE with uninitialized value.

How I verified it

Setup mclag topology, some times mclagsyncd will not be able to receive data after startup.

Details if related

More about this issue in cpp reference

IMHO it must be treated as compilation error, so it will be better to use-Werror=reorder parameter for compiler to avoid such issues.

@globaltrouble globaltrouble marked this pull request as draft January 13, 2022 11:56
@globaltrouble globaltrouble marked this pull request as ready for review January 13, 2022 17:04
@globaltrouble
Copy link
Contributor Author

@gechiang could you please review?

@globaltrouble globaltrouble force-pushed the fix/mclagsyncd-out-of-order-initialization branch from 86c14e5 to 36da3e8 Compare January 17, 2022 13:16
@msosyak
Copy link
Contributor

msosyak commented Jan 18, 2022

@prsunny Could we merge this?

@gechiang
Copy link
Contributor

Adding @Praveen-Brcm for visibility and the compilation enabling question.
The changes look good to me. Once Praveen approves it will merge it.
Thanks!

Copy link
Contributor

@gechiang gechiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's wait for @Praveen-Brcm approval before I merge this in.

@msosyak
Copy link
Contributor

msosyak commented Jan 21, 2022

@Praveen-Brcm Have a look, please.

@gechiang gechiang merged commit 69f9ee5 into sonic-net:master Jan 22, 2022
@Praveen-Brcm
Copy link
Contributor

@gechiang , @akokhan @msosyak : sorry to respond late on this. I missed on it.
The changes looks good. Would like to understand if the issue is seen consistently.? We have not see such issue in Broadcom testing.

@globaltrouble
Copy link
Contributor Author

@Praveen-Brcm it is undefined behavior, so the issue is not consistent, it is random. The issue could be easily see only when MSG_BATCH_SIZE = 0 before initialization (depends on previous data on stack). On my setup the issue seen only during first start of mclagsyncd on specific hardware.

dprital pushed a commit to dprital/sonic-swss that referenced this pull request May 8, 2022
bratashX pushed a commit to bratashX/sonic-swss that referenced this pull request May 13, 2022
bratashX pushed a commit to bratashX/sonic-swss that referenced this pull request May 18, 2022
bratashX pushed a commit to bratashX/sonic-swss that referenced this pull request May 19, 2022
bratashX pushed a commit to bratashX/sonic-swss that referenced this pull request Jun 6, 2022
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
bratashX added a commit to bratashX/sonic-swss that referenced this pull request Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants