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.

Sign up for GitHub

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

Jump to bottom

openamp/rpmsg: fix rpmsg_service memory leak when stop remote #525

Closed
wants to merge 1 commit into from
Closed

openamp/rpmsg: fix rpmsg_service memory leak when stop remote #525

wants to merge 1 commit into from

Conversation

yintao707
Copy link

When remote cannot send destroy messages, master cannot call ns_unbind_cb, this will cause the master to be unable to release resources

openamp/rpmsg: fix rpmsg_service memory leak when stop remote
8242ff0 
when remote cannot send destroy message, master cannot call ns_unbind_cb
that will cause service cannot release release resources

Signed-off-by: Tao Yin <yintao@xiaomi.com>
@arnopo arnopo mentioned this pull request Oct 19, 2023
Closed
@arnopo
Copy link
Collaborator

arnopo commented Oct 24, 2023

Seems similar to #508, could you have a look?

@arnopo arnopo requested a review from tnmysh October 24, 2023 14:36
edmooring
edmooring approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Looks good to go.

@yintao707
Copy link
Author

Seems similar to #508, could you have a look?

Thank you for your review, this has little interaction with #508

@arnopo
Copy link
Collaborator

arnopo commented Nov 2, 2023

Seems similar to #508, could you have a look?

Thank you for your review, this has little interaction with #508

Please could you explain what do you mean by "little interaction" ?
The ns_unbind_cb is used to inform that the other side destroys it endpoint. Using it for the deinit can introduce side effect in the existing code. The ept release introduced in #508 seems more appropriate. Is it an issue to use it?

@yintao707
Copy link
Author

Seems similar to #508, could you have a look?

Thank you for your review, this has little interaction with #508

Please could you explain what do you mean by "little interaction" ? The ns_unbind_cb is used to inform that the other side destroys it endpoint. Using it for the deinit can introduce side effect in the existing code. The ept release introduced in #508 seems more appropriate. Is it an issue to use it?

yes, you're right, sorry for my inaccurate expression. But the main reason for this modification is that master cannt received NS_DESTORY message, when remote is powered down or exits abnormally. at that time, master needs to actively call ns_unbind_cb to release resources

@xiaoxiang781216
Copy link
Collaborator

@arnopo does this change make sense now?

@arnopo
Copy link
Collaborator

arnopo commented Nov 10, 2023

@arnopo does this change make sense now?

As we go to integrate the ept->release ops, look to me that is should also address the need described here. But perhaps I miss something?

@arnopo
Copy link
Collaborator

arnopo commented Dec 5, 2023

could we close this one as we merge #508?

@arnopo
Copy link
Collaborator

arnopo commented Jan 9, 2024

I close it as no news Please reopen it if needed

@arnopo arnopo closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmooring edmooring edmooring approved these changes

@tammyleino tammyleino tammyleino approved these changes

@arnopo arnopo Awaiting requested review from arnopo

@tnmysh tnmysh Awaiting requested review from tnmysh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yintao707 @arnopo @xiaoxiang781216 @edmooring @tammyleino