* [PATCH net-next 0/3] message reassembly improvements
@ 2013-10-26 18:41 Jon Maloy
2013-10-26 18:41 ` [PATCH net-next 1/3] tipc: don't reroute message fragments Jon Maloy
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jon Maloy @ 2013-10-26 18:41 UTC (permalink / raw)
To: davem; +Cc: Jon Maloy, netdev, tipc-discussion
We introduce a new defragmentation algorithm that improves performance
and eliminates the risk of causing out-of-memory situations.
Erik Hugne (3):
tipc: don't reroute message fragments
tipc: message reassembly using fragment chain
tipc: reassembly failures should cause link reset
net/tipc/bcast.c | 16 ++++--
net/tipc/link.c | 161 +++++++++++++++---------------------------------------
net/tipc/link.h | 20 +++++--
net/tipc/msg.h | 12 ----
net/tipc/node.c | 7 ++-
net/tipc/node.h | 6 +-
6 files changed, 77 insertions(+), 145 deletions(-)
--
1.7.9.5
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH net-next 1/3] tipc: don't reroute message fragments 2013-10-26 18:41 [PATCH net-next 0/3] message reassembly improvements Jon Maloy @ 2013-10-26 18:41 ` Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 2/3] tipc: message reassembly using fragment chain Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 3/3] tipc: reassembly failures should cause link reset Jon Maloy 2 siblings, 0 replies; 9+ messages in thread From: Jon Maloy @ 2013-10-26 18:41 UTC (permalink / raw) To: davem; +Cc: Jon Maloy, netdev, tipc-discussion From: Erik Hugne <erik.hugne@ericsson.com> When a message fragment is received in a broadcast or unicast link, the reception code will append the fragment payload to a big reassembly buffer through a call to the function tipc_recv_fragm(). However, after the return of that call, the logics goes on and passes the fragment buffer to the function tipc_net_route_msg(), which will simply drop it. This behavior is a remnant from the now obsolete multi-cluster functionality, and has no relevance in the current code base. Although currently harmless, this unnecessary call would be fatal after applying the next patch in this series, which introduces a completely new defragmentation algorithm. So we change the code to eliminate the redundant call. Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com> --- net/tipc/bcast.c | 6 ++++-- net/tipc/link.c | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index 716de1a..766a6eb 100644 --- a/net/tipc/bcast.c +++ b/net/tipc/bcast.c @@ -487,11 +487,13 @@ receive: spin_lock_bh(&bc_lock); bclink_accept_pkt(node, seqno); bcl->stats.recv_fragments++; - if (ret > 0) + if (ret > 0) { bcl->stats.recv_fragmented++; + spin_unlock_bh(&bc_lock); + goto receive; + } spin_unlock_bh(&bc_lock); tipc_node_unlock(node); - tipc_net_route_msg(buf); } else if (msg_user(msg) == NAME_DISTRIBUTOR) { spin_lock_bh(&bc_lock); bclink_accept_pkt(node, seqno); diff --git a/net/tipc/link.c b/net/tipc/link.c index e8153f6..dfa29c8 100644 --- a/net/tipc/link.c +++ b/net/tipc/link.c @@ -1638,7 +1638,7 @@ deliver: } if (ret == -1) l_ptr->next_in_no--; - break; + continue; case CHANGEOVER_PROTOCOL: type = msg_type(msg); if (link_recv_changeover_msg(&l_ptr, -- 1.7.9.5 ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 2/3] tipc: message reassembly using fragment chain 2013-10-26 18:41 [PATCH net-next 0/3] message reassembly improvements Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 1/3] tipc: don't reroute message fragments Jon Maloy @ 2013-10-26 18:41 ` Jon Maloy 2013-10-28 5:07 ` David Miller 2013-10-26 18:41 ` [PATCH net-next 3/3] tipc: reassembly failures should cause link reset Jon Maloy 2 siblings, 1 reply; 9+ messages in thread From: Jon Maloy @ 2013-10-26 18:41 UTC (permalink / raw) To: davem; +Cc: Jon Maloy, netdev, tipc-discussion From: Erik Hugne <erik.hugne@ericsson.com> When the first fragment of a long data data message is received on a link, a reassembly buffer large enough to hold the data from this and all subsequent fragments of the message is allocated. The payload of each new fragment is copied into this buffer upon arrival. When the last fragment is received, the reassembled message is delivered upwards to the port/socket layer. Not only is this an inefficient approach, but it may also cause bursts of reassembly failures in low memory situations. since we may fail to allocate the necessary large buffer in the first place. Furthermore, after 100 subsequent such failures the link will be reset, something that in reality aggravates the situation. To remedy this problem, this patch introduces a different approach. Instead of allocating a big reassembly buffer, we now append the arriving fragments to a reassembly chain on the link, and deliver the whole chain up to the socket layer once the last fragment has been received. This is safe because the retransmission layer of a TIPC link always delivers packets in strict uninterrupted order, to the reassembly layer as to all other upper layers. Hence there can never be more than one fragment chain pending reassembly at any given time in a link, and we can trust (but still verify) that the fragments will be chained up in the correct order. Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com> --- net/tipc/bcast.c | 12 +++-- net/tipc/link.c | 157 +++++++++++++++--------------------------------------- net/tipc/link.h | 20 ++++--- net/tipc/msg.h | 12 ----- net/tipc/node.c | 7 +-- net/tipc/node.h | 6 ++- 6 files changed, 72 insertions(+), 142 deletions(-) diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index 766a6eb..4874be2 100644 --- a/net/tipc/bcast.c +++ b/net/tipc/bcast.c @@ -480,15 +480,19 @@ receive: tipc_node_unlock(node); tipc_link_recv_bundle(buf); } else if (msg_user(msg) == MSG_FRAGMENTER) { - int ret = tipc_link_recv_fragment(&node->bclink.defragm, - &buf, &msg); - if (ret < 0) + int ret = tipc_link_recv_fragment( + &node->bclink.reasm_head, + &node->bclink.reasm_tail, + &buf); + if (ret == LINK_REASM_ERROR) goto unlock; spin_lock_bh(&bc_lock); bclink_accept_pkt(node, seqno); bcl->stats.recv_fragments++; - if (ret > 0) { + if (ret == LINK_REASM_COMPLETE) { bcl->stats.recv_fragmented++; + /* Point msg to inner header */ + msg = buf_msg(buf); spin_unlock_bh(&bc_lock); goto receive; } diff --git a/net/tipc/link.c b/net/tipc/link.c index dfa29c8..8e4942d 100644 --- a/net/tipc/link.c +++ b/net/tipc/link.c @@ -404,15 +404,9 @@ static void link_release_outqueue(struct tipc_link *l_ptr) */ void tipc_link_reset_fragments(struct tipc_link *l_ptr) { - struct sk_buff *buf = l_ptr->defragm_buf; - struct sk_buff *next; - - while (buf) { - next = buf->next; - kfree_skb(buf); - buf = next; - } - l_ptr->defragm_buf = NULL; + kfree_skb(l_ptr->reasm_head); + l_ptr->reasm_head = NULL; + l_ptr->reasm_tail = NULL; } /** @@ -1630,14 +1624,18 @@ deliver: case MSG_FRAGMENTER: l_ptr->stats.recv_fragments++; ret = tipc_link_recv_fragment( - &l_ptr->defragm_buf, - &buf, &msg); - if (ret == 1) { + &l_ptr->reasm_head, + &l_ptr->reasm_tail, + &buf); + if (ret == LINK_REASM_COMPLETE) { l_ptr->stats.recv_fragmented++; + /* Point msg to inner header */ + msg = buf_msg(buf); goto deliver; } - if (ret == -1) + if (ret == LINK_REASM_ERROR) l_ptr->next_in_no--; + tipc_node_unlock(n_ptr); continue; case CHANGEOVER_PROTOCOL: type = msg_type(msg); @@ -2347,114 +2345,43 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf) } /* - * A pending message being re-assembled must store certain values - * to handle subsequent fragments correctly. The following functions - * help storing these values in unused, available fields in the - * pending message. This makes dynamic memory allocation unnecessary. - */ -static void set_long_msg_seqno(struct sk_buff *buf, u32 seqno) -{ - msg_set_seqno(buf_msg(buf), seqno); -} - -static u32 get_fragm_size(struct sk_buff *buf) -{ - return msg_ack(buf_msg(buf)); -} - -static void set_fragm_size(struct sk_buff *buf, u32 sz) -{ - msg_set_ack(buf_msg(buf), sz); -} - -static u32 get_expected_frags(struct sk_buff *buf) -{ - return msg_bcast_ack(buf_msg(buf)); -} - -static void set_expected_frags(struct sk_buff *buf, u32 exp) -{ - msg_set_bcast_ack(buf_msg(buf), exp); -} - -/* * tipc_link_recv_fragment(): Called with node lock on. Returns * the reassembled buffer if message is complete. */ -int tipc_link_recv_fragment(struct sk_buff **pending, struct sk_buff **fb, - struct tipc_msg **m) -{ - struct sk_buff *prev = NULL; - struct sk_buff *fbuf = *fb; - struct tipc_msg *fragm = buf_msg(fbuf); - struct sk_buff *pbuf = *pending; - u32 long_msg_seq_no = msg_long_msgno(fragm); - - *fb = NULL; - - /* Is there an incomplete message waiting for this fragment? */ - while (pbuf && ((buf_seqno(pbuf) != long_msg_seq_no) || - (msg_orignode(fragm) != msg_orignode(buf_msg(pbuf))))) { - prev = pbuf; - pbuf = pbuf->next; - } - - if (!pbuf && (msg_type(fragm) == FIRST_FRAGMENT)) { - struct tipc_msg *imsg = (struct tipc_msg *)msg_data(fragm); - u32 msg_sz = msg_size(imsg); - u32 fragm_sz = msg_data_sz(fragm); - u32 exp_fragm_cnt; - u32 max = TIPC_MAX_USER_MSG_SIZE + NAMED_H_SIZE; - - if (msg_type(imsg) == TIPC_MCAST_MSG) - max = TIPC_MAX_USER_MSG_SIZE + MCAST_H_SIZE; - if (fragm_sz == 0 || msg_size(imsg) > max) { - kfree_skb(fbuf); - return 0; - } - exp_fragm_cnt = msg_sz / fragm_sz + !!(msg_sz % fragm_sz); - pbuf = tipc_buf_acquire(msg_size(imsg)); - if (pbuf != NULL) { - pbuf->next = *pending; - *pending = pbuf; - skb_copy_to_linear_data(pbuf, imsg, - msg_data_sz(fragm)); - /* Prepare buffer for subsequent fragments. */ - set_long_msg_seqno(pbuf, long_msg_seq_no); - set_fragm_size(pbuf, fragm_sz); - set_expected_frags(pbuf, exp_fragm_cnt - 1); - } else { - pr_debug("Link unable to reassemble fragmented message\n"); - kfree_skb(fbuf); - return -1; - } - kfree_skb(fbuf); - return 0; - } else if (pbuf && (msg_type(fragm) != FIRST_FRAGMENT)) { - u32 dsz = msg_data_sz(fragm); - u32 fsz = get_fragm_size(pbuf); - u32 crs = ((msg_fragm_no(fragm) - 1) * fsz); - u32 exp_frags = get_expected_frags(pbuf) - 1; - skb_copy_to_linear_data_offset(pbuf, crs, - msg_data(fragm), dsz); - kfree_skb(fbuf); - - /* Is message complete? */ - if (exp_frags == 0) { - if (prev) - prev->next = pbuf->next; - else - *pending = pbuf->next; - msg_reset_reroute_cnt(buf_msg(pbuf)); - *fb = pbuf; - *m = buf_msg(pbuf); - return 1; - } - set_expected_frags(pbuf, exp_frags); +int tipc_link_recv_fragment(struct sk_buff **head, struct sk_buff **tail, + struct sk_buff **fbuf) +{ + struct sk_buff *frag = *fbuf; + struct tipc_msg *msg = buf_msg(frag); + u32 fragid = msg_type(msg); + + skb_pull(frag, msg_hdr_sz(msg)); + if (fragid == FIRST_FRAGMENT) { + if (*head) + goto out_free; + *head = frag; + skb_frag_list_init(*head); return 0; + } else { + if (!*head) + goto out_free; + if (!skb_has_frag_list(*head)) + skb_shinfo(*head)->frag_list = frag; + else + (*tail)->next = frag; + *tail = frag; + (*head)->truesize += frag->truesize; + } + if (fragid == LAST_FRAGMENT) { + *fbuf = *head; + *tail = *head = NULL; + return LINK_REASM_COMPLETE; } - kfree_skb(fbuf); return 0; +out_free: + pr_warn_ratelimited("Link unable to reassemble fragmented message\n"); + kfree_skb(*fbuf); + return LINK_REASM_ERROR; } static void link_set_supervision_props(struct tipc_link *l_ptr, u32 tolerance) diff --git a/net/tipc/link.h b/net/tipc/link.h index 55cf855..8a6c102 100644 --- a/net/tipc/link.h +++ b/net/tipc/link.h @@ -41,6 +41,12 @@ #include "node.h" /* + * Link reassembly status codes + */ +#define LINK_REASM_ERROR -1 +#define LINK_REASM_COMPLETE 1 + +/* * Out-of-range value for link sequence numbers */ #define INVALID_LINK_SEQ 0x10000 @@ -134,7 +140,8 @@ struct tipc_stats { * @next_out: ptr to first unsent outbound message in queue * @waiting_ports: linked list of ports waiting for link congestion to abate * @long_msg_seq_no: next identifier to use for outbound fragmented messages - * @defragm_buf: list of partially reassembled inbound message fragments + * @reasm_head: list head of partially reassembled inbound message fragments + * @reasm_tail: last fragment received * @stats: collects statistics regarding link activity */ struct tipc_link { @@ -196,9 +203,10 @@ struct tipc_link { struct sk_buff *next_out; struct list_head waiting_ports; - /* Fragmentation/defragmentation */ + /* Fragmentation/reassembly */ u32 long_msg_seq_no; - struct sk_buff *defragm_buf; + struct sk_buff *reasm_head; + struct sk_buff *reasm_tail; /* Statistics */ struct tipc_stats stats; @@ -229,9 +237,9 @@ int tipc_link_send_sections_fast(struct tipc_port *sender, struct iovec const *msg_sect, unsigned int len, u32 destnode); void tipc_link_recv_bundle(struct sk_buff *buf); -int tipc_link_recv_fragment(struct sk_buff **pending, - struct sk_buff **fb, - struct tipc_msg **msg); +int tipc_link_recv_fragment(struct sk_buff **reasm_head, + struct sk_buff **reasm_tail, + struct sk_buff **fbuf); void tipc_link_send_proto_msg(struct tipc_link *l_ptr, u32 msg_typ, int prob, u32 gap, u32 tolerance, u32 priority, u32 acked_mtu); diff --git a/net/tipc/msg.h b/net/tipc/msg.h index 559b73a..76d1269 100644 --- a/net/tipc/msg.h +++ b/net/tipc/msg.h @@ -554,12 +554,6 @@ static inline void msg_set_last_bcast(struct tipc_msg *m, u32 n) msg_set_bits(m, 4, 16, 0xffff, n); } - -static inline u32 msg_fragm_no(struct tipc_msg *m) -{ - return msg_bits(m, 4, 16, 0xffff); -} - static inline void msg_set_fragm_no(struct tipc_msg *m, u32 n) { msg_set_bits(m, 4, 16, 0xffff, n); @@ -576,12 +570,6 @@ static inline void msg_set_next_sent(struct tipc_msg *m, u32 n) msg_set_bits(m, 4, 0, 0xffff, n); } - -static inline u32 msg_long_msgno(struct tipc_msg *m) -{ - return msg_bits(m, 4, 0, 0xffff); -} - static inline void msg_set_long_msgno(struct tipc_msg *m, u32 n) { msg_set_bits(m, 4, 0, 0xffff, n); diff --git a/net/tipc/node.c b/net/tipc/node.c index 6e6c434..25100c0 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -298,9 +298,10 @@ static void node_lost_contact(struct tipc_node *n_ptr) } n_ptr->bclink.deferred_size = 0; - if (n_ptr->bclink.defragm) { - kfree_skb(n_ptr->bclink.defragm); - n_ptr->bclink.defragm = NULL; + if (n_ptr->bclink.reasm_head) { + kfree_skb(n_ptr->bclink.reasm_head); + n_ptr->bclink.reasm_head = NULL; + n_ptr->bclink.reasm_tail = NULL; } tipc_bclink_remove_node(n_ptr->addr); diff --git a/net/tipc/node.h b/net/tipc/node.h index 3c189b3..e5e96c0 100644 --- a/net/tipc/node.h +++ b/net/tipc/node.h @@ -74,7 +74,8 @@ * @deferred_size: number of OOS b'cast messages in deferred queue * @deferred_head: oldest OOS b'cast message received from node * @deferred_tail: newest OOS b'cast message received from node - * @defragm: list of partially reassembled b'cast message fragments from node + * @reasm_head: broadcast reassembly queue head from node + * @reasm_tail: last broadcast fragment received from node * @recv_permitted: true if node is allowed to receive b'cast messages */ struct tipc_node { @@ -98,7 +99,8 @@ struct tipc_node { u32 deferred_size; struct sk_buff *deferred_head; struct sk_buff *deferred_tail; - struct sk_buff *defragm; + struct sk_buff *reasm_head; + struct sk_buff *reasm_tail; bool recv_permitted; } bclink; }; -- 1.7.9.5 ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain 2013-10-26 18:41 ` [PATCH net-next 2/3] tipc: message reassembly using fragment chain Jon Maloy @ 2013-10-28 5:07 ` David Miller 2013-10-28 10:24 ` Jon Maloy 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2013-10-28 5:07 UTC (permalink / raw) To: jon.maloy; +Cc: netdev, tipc-discussion From: Jon Maloy <jon.maloy@ericsson.com> Date: Sat, 26 Oct 2013 14:41:02 -0400 > + int ret = tipc_link_recv_fragment( > + &node->bclink.reasm_head, > + &node->bclink.reasm_tail, > + &buf); This is not the correct way to indent a function call that spans multiple lines. In such a situation the arguments that appear on the second and subsequent lines must start at the first column after the openning parenthesis of the function call. Like this: func(a, b, c, d, e, f); Please audit this in your entire set of patches and resubmit, thanks. ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain 2013-10-28 5:07 ` David Miller @ 2013-10-28 10:24 ` Jon Maloy 2013-10-29 1:56 ` Ying Xue 0 siblings, 1 reply; 9+ messages in thread From: Jon Maloy @ 2013-10-28 10:24 UTC (permalink / raw) Cc: jon.maloy, paul.gortmaker, erik.hugne, ying.xue, tipc-discussion, netdev, David Miller On 10/28/2013 01:07 AM, David Miller wrote: > From: Jon Maloy <jon.maloy@ericsson.com> > Date: Sat, 26 Oct 2013 14:41:02 -0400 > >> + int ret = tipc_link_recv_fragment( >> + &node->bclink.reasm_head, >> + &node->bclink.reasm_tail, >> + &buf); > This is not the correct way to indent a function call that spans > multiple lines. In such a situation the arguments that appear > on the second and subsequent lines must start at the first column > after the openning parenthesis of the function call. > > Like this: > > func(a, b, c, > d, e, f); > > Please audit this in your entire set of patches and resubmit, > thanks. Doing as David says here means that some lines will be >80 chars. This was the reason for the somewhat strange indentation. I tried to rename the function to tipc_link_rcv_fragm(), but one line was still too long. The problem we have goes deeper. In Linus' coding style manual I read that the 80 char limit is a hard limit, a limit we violate in several places. One offender is that we have too many indentation levels, at least in tipc_recv_msg() and probably in some other places. This is sensitive code, that I don't feel for touching right now. A more low hanging fruit is our local variable names: names such as l_ptr, n_ptr, b_ptr is exactly what Linus characterizes as "brain dead Hungarian style", and I never liked that naming anyway. For me l, n, and b is good enough as long as the context is clear. But, doing so, at least in tipc_recv_msg(), would require another, separate, patch, and it would lead to style inconsistency. In brief, I am at loss about to proceed here, and I am not going to submit this patch again until I have some feedback from somebody who can tell me what is the right thing to do. Maybe > 80 chars is fine for now? ///jon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain 2013-10-28 10:24 ` Jon Maloy @ 2013-10-29 1:56 ` Ying Xue 2013-10-29 3:49 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Ying Xue @ 2013-10-29 1:56 UTC (permalink / raw) To: Jon Maloy; +Cc: jon.maloy, netdev, tipc-discussion, David Miller On 10/28/2013 06:24 PM, Jon Maloy wrote: > On 10/28/2013 01:07 AM, David Miller wrote: >> From: Jon Maloy <jon.maloy@ericsson.com> >> Date: Sat, 26 Oct 2013 14:41:02 -0400 >> >>> + int ret = tipc_link_recv_fragment( >>> + &node->bclink.reasm_head, >>> + &node->bclink.reasm_tail, >>> + &buf); >> This is not the correct way to indent a function call that spans >> multiple lines. In such a situation the arguments that appear >> on the second and subsequent lines must start at the first column >> after the openning parenthesis of the function call. >> >> Like this: >> >> func(a, b, c, >> d, e, f); >> >> Please audit this in your entire set of patches and resubmit, >> thanks. > > Doing as David says here means that some lines will be >80 chars. > This was the reason for the somewhat strange indentation. > I tried to rename the function to tipc_link_rcv_fragm(), but one > line was still too long. The problem we have goes deeper. > > In Linus' coding style manual I read that the 80 char limit is a hard limit, > a limit we violate in several places. One offender is that we have too > many indentation levels, at least in tipc_recv_msg() and probably in > some other places. This is sensitive code, that I don't feel for touching > right now. A more low hanging fruit is our local variable names: > names such as l_ptr, n_ptr, b_ptr is exactly what Linus characterizes > as "brain dead Hungarian style", and I never liked that naming anyway. > For me l, n, and b is good enough as long as the context is clear. > > But, doing so, at least in tipc_recv_msg(), would require another, separate, > patch, and it would lead to style inconsistency. > > In brief, I am at loss about to proceed here, and I am not going to submit > this patch again until I have some feedback from somebody who can tell me > what is the right thing to do. Maybe > 80 chars is fine for now? > It's hard completely resolve the issue by simply changing variable names. So in my opinion, this is not a simple code style problem any more. As tipc_recv_msg() is too complex, it includes at least three embedded loops so that it doesn't remain too much space for functions in the most inner loop. This is the key point. Therefore, the best method is to divide tipc_recv_msg() into several smaller functions to simplify the current implementation. But it's not an easy job. Actually I ever tried to do it, but I gave up lastly because I did not find one perfect solution about how to divide it. Regards, Ying > ///jon > > > > ------------------------------------------------------------------------------ Android is increasing in popularity, but the open development platform that developers love is also attractive to malware creators. Download this white paper to learn more about secure code signing practices that can help keep Android apps secure. http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain 2013-10-29 1:56 ` Ying Xue @ 2013-10-29 3:49 ` David Miller 2013-10-29 5:36 ` Ying Xue 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2013-10-29 3:49 UTC (permalink / raw) To: ying.xue Cc: maloy, jon.maloy, paul.gortmaker, erik.hugne, tipc-discussion, netdev From: Ying Xue <ying.xue@windriver.com> Date: Tue, 29 Oct 2013 09:56:56 +0800 > Therefore, the best method is to divide tipc_recv_msg() into several > smaller functions to simplify the current implementation. But it's not > an easy job. Actually I ever tried to do it, but I gave up lastly > because I did not find one perfect solution about how to divide it. But you're going to have to find a way, this indent level is out of control. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain 2013-10-29 3:49 ` David Miller @ 2013-10-29 5:36 ` Ying Xue 0 siblings, 0 replies; 9+ messages in thread From: Ying Xue @ 2013-10-29 5:36 UTC (permalink / raw) To: David Miller, maloy, jon.maloy Cc: paul.gortmaker, erik.hugne, tipc-discussion, netdev On 10/29/2013 11:49 AM, David Miller wrote: > From: Ying Xue <ying.xue@windriver.com> > Date: Tue, 29 Oct 2013 09:56:56 +0800 > >> Therefore, the best method is to divide tipc_recv_msg() into several >> smaller functions to simplify the current implementation. But it's not >> an easy job. Actually I ever tried to do it, but I gave up lastly >> because I did not find one perfect solution about how to divide it. > > But you're going to have to find a way, this indent level is out of > control. > > By simply adjusting the order of branch blocks, this is what I can figure out the simplest method to resolve the issue now :) Please review below change. If it's fine, I will send one formal patch. (Below change still follows original code logic) diff --git a/net/tipc/link.c b/net/tipc/link.c index 12c72d2..6c88dbc 100644 --- a/net/tipc/link.c +++ b/net/tipc/link.c @@ -1592,97 +1592,94 @@ void tipc_recv_msg(struct sk_buff *head, struct tipc_bearer *b_ptr) /* Now (finally!) process the incoming message */ protocol_check: - if (likely(link_working_working(l_ptr))) { - if (likely(seq_no == mod(l_ptr->next_in_no))) { - l_ptr->next_in_no++; - if (unlikely(l_ptr->oldest_deferred_in)) - head = link_insert_deferred_queue(l_ptr, - head); -deliver: - if (likely(msg_isdata(msg))) { - tipc_node_unlock(n_ptr); - tipc_port_recv_msg(buf); - continue; - } - switch (msg_user(msg)) { - int ret; - case MSG_BUNDLER: - l_ptr->stats.recv_bundles++; - l_ptr->stats.recv_bundled += - msg_msgcnt(msg); - tipc_node_unlock(n_ptr); - tipc_link_recv_bundle(buf); - continue; - case NAME_DISTRIBUTOR: - n_ptr->bclink.recv_permitted = true; - tipc_node_unlock(n_ptr); - tipc_named_recv(buf); - continue; - case BCAST_PROTOCOL: - tipc_link_recv_sync(n_ptr, buf); - tipc_node_unlock(n_ptr); - continue; - case CONN_MANAGER: - tipc_node_unlock(n_ptr); - tipc_port_recv_proto_msg(buf); - continue; - case MSG_FRAGMENTER: - l_ptr->stats.recv_fragments++; - ret = tipc_link_recv_fragment( - &l_ptr->defragm_buf, - &buf, &msg); - if (ret == 1) { - l_ptr->stats.recv_fragmented++; - goto deliver; - } - if (ret == -1) - l_ptr->next_in_no--; - break; - case CHANGEOVER_PROTOCOL: - type = msg_type(msg); - if (link_recv_changeover_msg(&l_ptr, - &buf)) { - msg = buf_msg(buf); - seq_no = msg_seqno(msg); - if (type == ORIGINAL_MSG) - goto deliver; - goto protocol_check; - } - break; - default: - kfree_skb(buf); - buf = NULL; - break; - } + if (unlikely(!link_working_working(l_ptr))) { + if (msg_user(msg) == LINK_PROTOCOL) { + link_recv_proto_msg(l_ptr, buf); + head = link_insert_deferred_queue(l_ptr, head); + tipc_node_unlock(n_ptr); + continue; + } + + /* Traffic message. Conditionally activate link */ + link_state_event(l_ptr, TRAFFIC_MSG_EVT); + + if (link_working_working(l_ptr)) { + /* Re-insert buffer in front of queue */ + buf->next = head; + head = buf; tipc_node_unlock(n_ptr); - tipc_net_route_msg(buf); continue; } + tipc_node_unlock(n_ptr); + goto cont; + } + + /* Link is now in state WORKING_WORKING */ + if (unlikely(seq_no != mod(l_ptr->next_in_no))) { link_handle_out_of_seq_msg(l_ptr, buf); head = link_insert_deferred_queue(l_ptr, head); tipc_node_unlock(n_ptr); continue; } - - /* Link is not in state WORKING_WORKING */ - if (msg_user(msg) == LINK_PROTOCOL) { - link_recv_proto_msg(l_ptr, buf); + l_ptr->next_in_no++; + if (unlikely(l_ptr->oldest_deferred_in)) head = link_insert_deferred_queue(l_ptr, head); +deliver: + if (likely(msg_isdata(msg))) { tipc_node_unlock(n_ptr); + tipc_port_recv_msg(buf); continue; } - - /* Traffic message. Conditionally activate link */ - link_state_event(l_ptr, TRAFFIC_MSG_EVT); - - if (link_working_working(l_ptr)) { - /* Re-insert buffer in front of queue */ - buf->next = head; - head = buf; + switch (msg_user(msg)) { + int ret; + case MSG_BUNDLER: + l_ptr->stats.recv_bundles++; + l_ptr->stats.recv_bundled += msg_msgcnt(msg); + tipc_node_unlock(n_ptr); + tipc_link_recv_bundle(buf); + continue; + case NAME_DISTRIBUTOR: + n_ptr->bclink.recv_permitted = true; + tipc_node_unlock(n_ptr); + tipc_named_recv(buf); + continue; + case BCAST_PROTOCOL: + tipc_link_recv_sync(n_ptr, buf); + tipc_node_unlock(n_ptr); + continue; + case CONN_MANAGER: tipc_node_unlock(n_ptr); + tipc_port_recv_proto_msg(buf); continue; + case MSG_FRAGMENTER: + l_ptr->stats.recv_fragments++; + ret = tipc_link_recv_fragment(&l_ptr->defragm_buf, + &buf, &msg); + if (ret == 1) { + l_ptr->stats.recv_fragmented++; + goto deliver; + } + if (ret == -1) + l_ptr->next_in_no--; + break; + case CHANGEOVER_PROTOCOL: + type = msg_type(msg); + if (link_recv_changeover_msg(&l_ptr, &buf)) { + msg = buf_msg(buf); + seq_no = msg_seqno(msg); + if (type == ORIGINAL_MSG) + goto deliver; + goto protocol_check; + } + break; + default: + kfree_skb(buf); + buf = NULL; + break; } tipc_node_unlock(n_ptr); + tipc_net_route_msg(buf); + continue; cont: kfree_skb(buf); } Regards, Ying ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 3/3] tipc: reassembly failures should cause link reset 2013-10-26 18:41 [PATCH net-next 0/3] message reassembly improvements Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 1/3] tipc: don't reroute message fragments Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 2/3] tipc: message reassembly using fragment chain Jon Maloy @ 2013-10-26 18:41 ` Jon Maloy 2 siblings, 0 replies; 9+ messages in thread From: Jon Maloy @ 2013-10-26 18:41 UTC (permalink / raw) To: davem; +Cc: Jon Maloy, netdev, tipc-discussion From: Erik Hugne <erik.hugne@ericsson.com> If appending a received fragment to the pending fragment chain in a unicast link fails, the current code tries to force a retransmission of the fragment by decrementing the 'next received sequence number' field in the link. This is done under the assumption that the failure is caused by an out-of-memory situation, an assumption that does not hold true after the previous patch in this series. A failure to append a fragment can now only be caused by a protocol violation by the sending peer, and it must hence be assumed that it is either malicious or buggy. Either way, the correct behavior is now to reset the link instead of trying to revert its sequence number. So, this is what we do in this commit. Signed-off-by: Erik Hugne <erik.hugne@ericsson.com> Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com> --- net/tipc/link.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/tipc/link.c b/net/tipc/link.c index 8e4942d..9cf75bb 100644 --- a/net/tipc/link.c +++ b/net/tipc/link.c @@ -1634,7 +1634,7 @@ deliver: goto deliver; } if (ret == LINK_REASM_ERROR) - l_ptr->next_in_no--; + tipc_link_reset(l_ptr); tipc_node_unlock(n_ptr); continue; case CHANGEOVER_PROTOCOL: -- 1.7.9.5 ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-10-29 5:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-26 18:41 [PATCH net-next 0/3] message reassembly improvements Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 1/3] tipc: don't reroute message fragments Jon Maloy 2013-10-26 18:41 ` [PATCH net-next 2/3] tipc: message reassembly using fragment chain Jon Maloy 2013-10-28 5:07 ` David Miller 2013-10-28 10:24 ` Jon Maloy 2013-10-29 1:56 ` Ying Xue 2013-10-29 3:49 ` David Miller 2013-10-29 5:36 ` Ying Xue 2013-10-26 18:41 ` [PATCH net-next 3/3] tipc: reassembly failures should cause link reset Jon Maloy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).