Skip to content

[fpmsyncd] Fix memory leak bug#864

Merged
prsunny merged 4 commits intosonic-net:masterfrom
baiwei0427:master
Apr 29, 2019
Merged

[fpmsyncd] Fix memory leak bug#864
prsunny merged 4 commits intosonic-net:masterfrom
baiwei0427:master

Conversation

@baiwei0427
Copy link
Copy Markdown
Contributor

@baiwei0427 baiwei0427 commented Apr 28, 2019

fpmsyncd: Fix memory leak bug

  • Use nl_cache_refill to refill the link cache instead of re-allocating link cache

Signed-off-by: Wei Bai baiwei0427@gmail.com

m_nl_sock = nl_socket_alloc();
nl_connect(m_nl_sock, NETLINK_ROUTE);
rtnl_link_alloc_cache(m_nl_sock, AF_UNSPEC, &m_link_cache);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove empty line

Copy link
Copy Markdown

@Jipan-Yang Jipan-Yang left a comment

Choose a reason for hiding this comment

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

Could you check https://github.com/Azure/sonic-swss-common/blob/master/common/linkcache.cpp ? It might be more efficient to allocate cache once and reuse it with nl_cache_refill().

@baiwei0427
Copy link
Copy Markdown
Contributor Author

@jipanyang @stcheng I guess the memory leak bug is due to the reallocation of link cache when interface name cannot be resolved. Is this bug first introduced on 142673a#diff-4a3ea7752832ebd49f989cc95ae042e1?

{
m_nl_sock = nl_socket_alloc();
nl_connect(m_nl_sock, NETLINK_ROUTE);
rtnl_link_alloc_cache(m_nl_sock, AF_UNSPEC, &m_link_cache);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this netlink connection and cache allocation be kept here and reused at RouteSync::getIfName()? So no need to create netlink connection and alloc cache for each route message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jipanyang This is a very good idea!

@stcheng
Copy link
Copy Markdown
Contributor

stcheng commented Apr 29, 2019

@baiwei0427 yes, this pull request #61 is the place that introduced the function rtnl_link_alloc_cache in onMsg function. Thanks for narrowing down the issue.

One question I have here is that, since you replaced the function rtnl_link_alloc_cache with nl_cache_refill, do we need to re-allocate socket and cache every time we want to get the interface name, or could it be done during the initialization time and only do the nl_cache_refill in getIfName function?

In this case, the memory leak issue could also be addressed, is that correct?

Copy link
Copy Markdown
Contributor

@stcheng stcheng left a comment

Choose a reason for hiding this comment

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

thanks for the update!

@prsunny prsunny merged commit ea4cba6 into sonic-net:master Apr 29, 2019
dgsudharsan pushed a commit to dgsudharsan/sonic-swss that referenced this pull request May 7, 2019
* Fix netlink memory leak of fpmsyncd

* Refill netlink cache instead of reallocation

* Allocate netlink socket and cache on the constructor function
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
* Add docker to 'show techsupport' command

Signed-off-by: Shlomi Bitton <shlomibi@mellanox.com>
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025
* Fix netlink memory leak of fpmsyncd

* Refill netlink cache instead of reallocation

* Allocate netlink socket and cache on the constructor function
jianyuewu pushed a commit to jianyuewu/sonic-swss that referenced this pull request Dec 24, 2025
* Change pubsub get_message API timeout to seconds

* Update pubsub.cpp

* Update pubsub.h

---------

Co-authored-by: Qi Luo <qiluo-msft@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants