DAOS-6808 rpc: fix memleak in MGMT_TGT_CREATE corpc #5037
Conversation
1. provide co_post_reply to free memory 2. change corpc to not call co_post_reply on root node. Signed-off-by: Xuezhao Liu <xuezhao.liu@intel.com>
| RPC_ADDREF(parent_rpc_priv); | ||
| crt_corpc_complete(parent_rpc_priv); | ||
|
|
||
| if (co_ops && co_ops->co_post_reply) | ||
| am_root = (co_info->co_grp_priv->gp_self == |
There was a problem hiding this comment.
Should we call this callback for all (not only NONE root)? Maybe move all free in single place?
There was a problem hiding this comment.
the problem is that when call co_post_reply, in corpc code here it does not know whether or not user will still use the memory (for example, user possibly addref the rpc and use its output for some other purpose).
There was a problem hiding this comment.
on leaf node or middle node, user lost control once the reply_send is called. But at root node, user's behavior is more flexible.
There was a problem hiding this comment.
Hmm. So, anyway it will be better to change current proc and call free for all structures at RPC destroy time.
I'm going to work for this patch later. Now let's land this solution.
There was a problem hiding this comment.
@liuxuezhao how is memory then free-ed on the root node? or does not not get allocated at all in this case on the root node as it is no longer sending the response to anyone and as such wont have those buffers?
There was a problem hiding this comment.
root node free in "DAOS-6808 pool: Fix cleanuping resources #5008" in src/mgmt/srv_pool.c
There was a problem hiding this comment.
@liuxuezhao if RPC is sent then proc functions are called. How it's possible to not use proc functions at all?
There was a problem hiding this comment.
on corpc root node, the bcast rpc is a "virtual" RPC, it just create a set of "real" RPC to its children. When forward to children it just copy the input struct to child real RPC, to get reply it just aggregate child's real rpc reply.
There was a problem hiding this comment.
you may refer crt_corpc_req_hdlr()
There was a problem hiding this comment.
@liuxuezhao Thanks! I will learn this logic in more details.
|
Can we land this? |
It's still in testing and like with your PR with this type of change it's important that we get a clean test run. |
Hmm. I'm not realized this is still under testing. It looks tests going verly long time. |
It was merged with master 19 hours ago...patches seem to be taking more than 24 lately. |
|
@jolivier23 seem this PR has not been lended to master yet, could you please land it? thx |
1. provide co_post_reply to free memory 2. change corpc to not call co_post_reply on root node. Signed-off-by: Xuezhao Liu <xuezhao.liu@intel.com>
1. provide co_post_reply to free memory 2. change corpc to not call co_post_reply on root node. Master-PR: #5037 Signed-off-by: Xuezhao Liu <xuezhao.liu@intel.com>
Signed-off-by: Xuezhao Liu xuezhao.liu@intel.com