Hi Tuong, I have only minor comments to this. See below. Post this to netdev (net-next) first, and I will post my modified patch on top of it later.
Acked-by: Jon Maloy <[email protected]> ///jon > -----Original Message----- > From: Tuong Lien <[email protected]> > Sent: 12-Jun-19 07:22 > To: [email protected]; Jon Maloy > <[email protected]>; [email protected]; [email protected] > Subject: [PATCH RFC] tipc: include retrans failure detection for unicast > > In patch series, commit 9195948fbf34 ("tipc: improve TIPC throughput by > Gap ACK blocks"), as for simplicity, the repeated retransmit failures' > detection in the function - "tipc_link_retrans()" was kept there for broadcast > retransmissions only. > > This commit now reapplies this feature for link unicast retransmissions that > has > been done via the function - "tipc_link_advance_transmq()". > > Signed-off-by: Tuong Lien <[email protected]> > --- > net/tipc/link.c | 96 ++++++++++++++++++++++++++++++++++++++----------- > -------- > 1 file changed, 65 insertions(+), 31 deletions(-) > > diff --git a/net/tipc/link.c b/net/tipc/link.c index > f5cd986e1e50..ffe3880a7b79 > 100644 > --- a/net/tipc/link.c > +++ b/net/tipc/link.c > @@ -249,9 +249,9 @@ static void tipc_link_build_bc_init_msg(struct > tipc_link *l, > struct sk_buff_head *xmitq); > static bool tipc_link_release_pkts(struct tipc_link *l, u16 to); static u16 > tipc_build_gap_ack_blks(struct tipc_link *l, void *data); -static void > tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 gap, > - struct tipc_gap_ack_blks *ga, > - struct sk_buff_head *xmitq); > +static int tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 gap, > + struct tipc_gap_ack_blks *ga, > + struct sk_buff_head *xmitq); > > /* > * Simple non-static link routines (i.e. referenced outside this file) @@ - > 1044,16 +1044,53 @@ static void tipc_link_advance_backlog(struct tipc_link > *l, > l->snd_nxt = seqno; > } > > -static void link_retransmit_failure(struct tipc_link *l, struct sk_buff *skb) > +/** > + * link_retransmit_failure() - Detect if retransmit failures repeated "Detect repeated retransmit failures" > + * @l: tipc link sender > + * @r: tipc link receiver (= l in case of unicast) > + * @from: seqno of the 1st packet in the retransmit request > + * @rc: returned code > + * > + * Return: true if the repeated retransmit failures happen, otherwise s/happen/happens/ > + * false > + */ > +static bool link_retransmit_failure(struct tipc_link *l, struct tipc_link *r, > + u16 from, int *rc) > { > - struct tipc_msg *hdr = buf_msg(skb); > + struct sk_buff *skb = skb_peek(&l->transmq); > + struct tipc_msg *hdr; > > - pr_warn("Retransmission failure on link <%s>\n", l->name); > - link_print(l, "State of link "); > - pr_info("Failed msg: usr %u, typ %u, len %u, err %u\n", > - msg_user(hdr), msg_type(hdr), msg_size(hdr), > msg_errcode(hdr)); > - pr_info("sqno %u, prev: %x, src: %x\n", > - msg_seqno(hdr), msg_prevnode(hdr), msg_orignode(hdr)); > + if (!skb) > + return false; > + > + hdr = buf_msg(skb); > + > + /* Detect repeated retransmit failures on same packet */ > + if (r->prev_from != from) { > + r->prev_from = from; > + r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance); > + r->stale_cnt = 0; > + } else if (++r->stale_cnt > 99 && time_after(jiffies, r->stale_limit)) { > + pr_warn("Retransmission failure on link <%s>\n", l->name); > + link_print(l, "State of link "); > + pr_info("Failed msg: usr %u, typ %u, len %u, err %u\n", > + msg_user(hdr), msg_type(hdr), > + msg_size(hdr), msg_errcode(hdr)); > + pr_info("sqno %u, prev: %x, src: %x\n", > + msg_seqno(hdr), msg_prevnode(hdr), > msg_orignode(hdr)); > + > + trace_tipc_list_dump(&l->transmq, true, "retrans failure!"); > + trace_tipc_link_dump(l, TIPC_DUMP_NONE, "retrans failure!"); > + trace_tipc_link_dump(r, TIPC_DUMP_NONE, "retrans failure!"); > + > + if (link_is_bc_sndlink(l)) > + *rc = TIPC_LINK_DOWN_EVT; > + > + *rc = tipc_link_fsm_evt(l, LINK_FAILURE_EVT); > + return true; > + } > + > + return false; > } > > /* tipc_link_retrans() - retransmit one or more packets @@ -1062,6 +1099,8 "zero ore more" is more correct here. > @@ static void link_retransmit_failure(struct tipc_link *l, struct sk_buff > *skb) > * @from: retransmit from (inclusive) this sequence number > * @to: retransmit to (inclusive) this sequence number > * xmitq: queue for accumulating the retransmitted packets > + * > + * Note: this retrans is for broadcast only! > */ > static int tipc_link_retrans(struct tipc_link *l, struct tipc_link *r, > u16 from, u16 to, struct sk_buff_head *xmitq) @@ - Since this one now is used only for broadcast, I suggest we rename it to tipc_link_bc_retrans(), as per convention elsewhere. > 1070,6 +1109,7 @@ static int tipc_link_retrans(struct tipc_link *l, struct > tipc_link *r, > u16 bc_ack = l->bc_rcvlink->rcv_nxt - 1; > u16 ack = l->rcv_nxt - 1; > struct tipc_msg *hdr; > + int rc = 0; > > if (!skb) > return 0; > @@ -1077,20 +1117,9 @@ static int tipc_link_retrans(struct tipc_link *l, > struct tipc_link *r, > return 0; > > trace_tipc_link_retrans(r, from, to, &l->transmq); > - /* Detect repeated retransmit failures on same packet */ > - if (r->prev_from != from) { > - r->prev_from = from; > - r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance); > - r->stale_cnt = 0; > - } else if (++r->stale_cnt > 99 && time_after(jiffies, r->stale_limit)) { > - link_retransmit_failure(l, skb); > - trace_tipc_list_dump(&l->transmq, true, "retrans failure!"); > - trace_tipc_link_dump(l, TIPC_DUMP_NONE, "retrans failure!"); > - trace_tipc_link_dump(r, TIPC_DUMP_NONE, "retrans failure!"); > - if (link_is_bc_sndlink(l)) > - return TIPC_LINK_DOWN_EVT; > - return tipc_link_fsm_evt(l, LINK_FAILURE_EVT); > - } > + > + if (link_retransmit_failure(l, r, from, &rc)) > + return rc; > > skb_queue_walk(&l->transmq, skb) { > hdr = buf_msg(skb); > @@ -1325,16 +1354,19 @@ static u16 tipc_build_gap_ack_blks(struct > tipc_link *l, void *data) > * @ga: buffer pointer to Gap ACK blocks from peer > * @xmitq: queue for accumulating the retransmitted packets if any > */ > -static void tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 > gap, > - struct tipc_gap_ack_blks *ga, > - struct sk_buff_head *xmitq) > +static int tipc_link_advance_transmq(struct tipc_link *l, u16 acked, u16 gap, > + struct tipc_gap_ack_blks *ga, > + struct sk_buff_head *xmitq) > { > struct sk_buff *skb, *_skb, *tmp; > struct tipc_msg *hdr; > u16 bc_ack = l->bc_rcvlink->rcv_nxt - 1; > u16 ack = l->rcv_nxt - 1; > - u16 seqno; > - u16 n = 0; > + u16 seqno, n = 0; > + int rc = 0; > + > + if (gap && link_retransmit_failure(l, l, acked + 1, &rc)) > + return rc; > > skb_queue_walk_safe(&l->transmq, skb, tmp) { > seqno = buf_seqno(skb); > @@ -1369,6 +1401,8 @@ static void tipc_link_advance_transmq(struct > tipc_link *l, u16 acked, u16 gap, > goto next_gap_ack; > } > } > + > + return 0; > } > > /* tipc_link_build_state_msg: prepare link state message for transmission > @@ -1919,7 +1953,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, > struct sk_buff *skb, > tipc_link_build_proto_msg(l, STATE_MSG, 0, reply, > rcvgap, 0, 0, xmitq); > > - tipc_link_advance_transmq(l, ack, gap, ga, xmitq); > + rc = tipc_link_advance_transmq(l, ack, gap, ga, xmitq); > > /* If NACK, retransmit will now start at right position */ > if (gap) > -- > 2.13.7 _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
