> -----Original Message----- > From: Richard Alpe > Sent: Thursday, 11 February, 2016 04:44 > To: Jon Maloy; [email protected] > Cc: Ying Xue; [email protected] > Subject: Re: [PATCH tipc-discussion v1] tipc: refactor node xmit and fix > memory leaks > > On 2016-02-10 17:34, Jon Maloy wrote: > > > > > >> -----Original Message----- > >> From: Richard Alpe > >> Sent: Friday, 05 February, 2016 10:29 > >> To: [email protected] > >> Cc: Jon Maloy; Ying Xue; [email protected]; Richard Alpe > >> Subject: [PATCH tipc-discussion v1] tipc: refactor node xmit and fix > >> memory leaks > >> > >> Refactor tipc_node_xmit() to fail fast and fail early. This commit > >> fixes several potential memory leaks in unexpected error paths. > >> > >> -- tipc discussion -- > >> Two thinks struck me when looking at this. > >> > >> 1. We claim to consume buffer chains in a lot of functions, but the > >> error path is often neglected. > > > > Admitted. It is certainly due to the fact that I changed the model at a late > stage. Before, we never consumed the buffer chain at this level. > > > >> > >> 2. Listing the potential return values in the function header is a bad > >> idea. > >> There is often a discrepancy between the listed and actual return > >> values. In the case of tail calls for example, someone has updated > >> the called function to return a new value, but "forgotten" to > >> backtrack all callers and updating there headers. > > > > Mea culpa again, for same reason as above. > > I don't think it is a bad idea per se; it is useful if we are just able to > > make it > right. After all, this is not a function that we are going to change every > month > (and hopefully not for a very long time from now on). The only code I see > missing now is -ENOBUF as return code from tipc_node_xmit(). > I think the risk of inconsistency causing confusion outweighs the benefit > here. > > I think it's fine to have comments like > "Returns number of bytes read or a negative error code" > "Returns non-zero if the page was successfully invalidated" > or perhaps even > "Returns zero on success, negative values on failure" > > These comments explains behaviour and only needs updating if the logic of > the function changes completely. For example a read function no longer > returning the bytes read. Listing all potential return codes like we do here > can > and will change if a called function is updated just slightly with a new error > code. Imagine if all functions had this. Changing a error code in one single > place would force you to backtrack through all callers and there callers, to > check if you need to update a comment. > > I don't think I have seen all potential return codes listed like this anywhere > else in the kernel(?) > > Sorry for the biskshedding.... :-)
With the current code, it is relatively easy to directly read out the potential return values, the only exception being when tiipc_link_xmit() makes an extra call to link_schedule_user() and returns its return value. It wasn't always that simple, e.g., when even the tipc_sk_rcv() call had a return value. That is why I wanted to document it this way. Admittedly, there is less need for it now, but I can't see that it hurts either, as long as it is correct. ///jon > > Regards > Richard > > > > > > If you fix that return code value (remove "errno" so it fits on one line) > > you > can add a "Reviewed-by" from me. > > > > > ///jon > > > >> > >> Reported-by: Dmitry Vyukov <[email protected]> > >> Signed-off-by: Richard Alpe <[email protected]> > >> --- > >> net/tipc/link.c | 8 ++++++-- > >> net/tipc/node.c | 52 > >> +++++++++++++++++++++++++++++++------------------ > >> --- > >> 2 files changed, 37 insertions(+), 23 deletions(-) > >> > >> diff --git a/net/tipc/link.c b/net/tipc/link.c index 0c2944f..d700be4 > >> 100644 > >> --- a/net/tipc/link.c > >> +++ b/net/tipc/link.c > >> @@ -904,8 +904,10 @@ int tipc_link_xmit(struct tipc_link *l, struct > >> sk_buff_head *list, > >> if (unlikely(l->backlog[i].len >= l->backlog[i].limit)) > >> return link_schedule_user(l, list); > >> } > >> - if (unlikely(msg_size(hdr) > mtu)) > >> + if (unlikely(msg_size(hdr) > mtu)) { > >> + skb_queue_purge(list); > >> return -EMSGSIZE; > >> + } > >> > >> /* Prepare each packet for sending, and add to relevant queue: */ > >> while (skb_queue_len(list)) { > >> @@ -917,8 +919,10 @@ int tipc_link_xmit(struct tipc_link *l, struct > >> sk_buff_head *list, > >> > >> if (likely(skb_queue_len(transmq) < maxwin)) { > >> _skb = skb_clone(skb, GFP_ATOMIC); > >> - if (!_skb) > >> + if (!_skb) { > >> + skb_queue_purge(list); > >> return -ENOBUFS; > >> + } > >> __skb_dequeue(list); > >> __skb_queue_tail(transmq, skb); > >> __skb_queue_tail(xmitq, _skb); > >> diff --git a/net/tipc/node.c b/net/tipc/node.c index fa97d96..61024ec > >> 100644 > >> --- a/net/tipc/node.c > >> +++ b/net/tipc/node.c > >> @@ -1174,33 +1174,43 @@ int tipc_node_xmit(struct net *net, struct > >> sk_buff_head *list, > >> struct tipc_link_entry *le = NULL; > >> struct tipc_node *n; > >> struct sk_buff_head xmitq; > >> - int bearer_id = -1; > >> - int rc = -EHOSTUNREACH; > >> + int bearer_id; > >> + int rc; > >> + > >> + if (in_own_node(net, dnode)) { > >> + tipc_sk_rcv(net, list); > >> + return 0; > >> + } > >> > >> - __skb_queue_head_init(&xmitq); > >> n = tipc_node_find(net, dnode); > >> - if (likely(n)) { > >> - tipc_node_read_lock(n); > >> - bearer_id = n->active_links[selector & 1]; > >> - if (bearer_id >= 0) { > >> - le = &n->links[bearer_id]; > >> - spin_lock_bh(&le->lock); > >> - rc = tipc_link_xmit(le->link, list, &xmitq); > >> - spin_unlock_bh(&le->lock); > >> - } > >> + if (unlikely(!n)) { > >> + skb_queue_purge(list); > >> + return -EHOSTUNREACH; > >> + } > >> + > >> + tipc_node_read_lock(n); > >> + bearer_id = n->active_links[selector & 1]; > >> + if (unlikely(bearer_id == INVALID_BEARER_ID)) { > >> tipc_node_read_unlock(n); > >> - if (likely(!rc)) > >> - tipc_bearer_xmit(net, bearer_id, &xmitq, &le- > >>> maddr); > >> - else if (rc == -ENOBUFS) > >> - tipc_node_link_down(n, bearer_id, false); > >> tipc_node_put(n); > >> - return rc; > >> + skb_queue_purge(list); > >> + return -EHOSTUNREACH; > >> } > >> > >> - if (likely(in_own_node(net, dnode))) { > >> - tipc_sk_rcv(net, list); > >> - return 0; > >> - } > >> + __skb_queue_head_init(&xmitq); > >> + le = &n->links[bearer_id]; > >> + spin_lock_bh(&le->lock); > >> + rc = tipc_link_xmit(le->link, list, &xmitq); > >> + spin_unlock_bh(&le->lock); > >> + tipc_node_read_unlock(n); > >> + > >> + if (likely(rc == 0)) > >> + tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr); > >> + else if (rc == -ENOBUFS) > >> + tipc_node_link_down(n, bearer_id, false); > >> + > >> + tipc_node_put(n); > >> + > >> return rc; > >> } > >> > >> -- > >> 2.1.4 > > ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
