search

OpenAMP / open-amp

The main OpenAMP library implementing RPMSG, Virtio, and Remoteproc for RTOS etc
https://www.openampproject.org/
Other
706 stars 288 forks source link

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

Closed yintao707 closed 8 months ago

yintao707 commented 11 months ago

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

arnopo commented 11 months ago

Seems similar to https://github.com/OpenAMP/open-amp/pull/508, could you have a look?

yintao707 commented 11 months ago

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

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

arnopo commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@arnopo does this change make sense now?

arnopo commented 10 months ago

@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 commented 10 months ago

could we close this one as we merge #508?

arnopo commented 8 months ago

I close it as no news Please reopen it if needed