Menu
▾
▴
Re: [tipc-discussion] [PATCH RFC] tipc: include retrans failure detection for unicast
|
From: Jon M. <jon...@er...> - 2019-06-12 15:19:43
|
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 <jon...@er...>
///jon
> -----Original Message-----
> From: Tuong Lien <tuo...@de...>
> Sent: 12-Jun-19 07:22
> To: tip...@li...; Jon Maloy
> <jon...@er...>; ma...@do...; yin...@wi...
> 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 <tuo...@de...>
> ---
> 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
|
