Re: [tipc-discussion] [net-next 3/3] tipc: introduce variable window congestion control

[Jon M. <jm...@re...>]

On 3/17/20 6:55 AM, Tuong Lien Tong wrote:
> Hi Jon,
>
> For the "variable window congestion control" patch, if I remember correctly,
> it is for unicast link only? Why did you apply it for broadcast link, a
> mistake or ...?
I did it so the code would be the same everywhere. Then, by setting both
min_win and max_win to the same value BC_LINK_WIN_DEFAULT (==50)
in the broadcast send link this window should never change.

> It now causes user messages disordered on the receiving side, because on the
> sending side, the broadcast link's window is suddenly increased to 300 (i.e.
> max_t(u16, l->window / 2, 300)) at a packet retransmission, leaving some
> gaps between the link's 'transmq' & 'backlogq' unexpectedly... Will we fix
> this by removing it?
That is clearly a bug that breaks the above stated limitation.
It should be sufficient to check that also l->ssthresh never exceeds
l->max_win to remedy this.

///jon

>
> @@ -1160,7 +1224,6 @@ static int tipc_link_bc_retrans(struct tipc_link *l,
> struct tipc_link *r,
>   			continue;
>   		if (more(msg_seqno(hdr), to))
>   			break;
> -
>   		if (time_before(jiffies, TIPC_SKB_CB(skb)->nxt_retr))
>   			continue;
>   		TIPC_SKB_CB(skb)->nxt_retr = TIPC_BC_RETR_LIM;
> @@ -1173,11 +1236,12 @@ static int tipc_link_bc_retrans(struct tipc_link *l,
> struct tipc_link *r,
>   		_skb->priority = TC_PRIO_CONTROL;
>   		__skb_queue_tail(xmitq, _skb);
>   		l->stats.retransmitted++;
> -
> +		retransmitted++;
>   		/* Increase actual retrans counter & mark first time */
>   		if (!TIPC_SKB_CB(skb)->retr_cnt++)
>   			TIPC_SKB_CB(skb)->retr_stamp = jiffies;
>   	}
> +	tipc_link_update_cwin(l, 0, retransmitted);             // ???
>   	return 0;
>   }
>
> +static void tipc_link_update_cwin(struct tipc_link *l, int released,
> +				  bool retransmitted)
> +{
> +	int bklog_len = skb_queue_len(&l->backlogq);
> +	struct sk_buff_head *txq = &l->transmq;
> +	int txq_len = skb_queue_len(txq);
> +	u16 cwin = l->window;
> +
> +	/* Enter fast recovery */
> +	if (unlikely(retransmitted)) {
> +		l->ssthresh = max_t(u16, l->window / 2, 300);
> +		l->window = l->ssthresh;
> +		return;
> +	}
>
> BR/Tuong
>
> -----Original Message-----
> From: Jon Maloy <jon...@er...>
> Sent: Monday, December 2, 2019 7:33 AM
> To: Jon Maloy <jon...@er...>; Jon Maloy <ma...@do...>
> Cc: moh...@er...;
> par...@gm...; tun...@de...;
> hoa...@de...; tuo...@de...;
> gor...@de...; yin...@wi...;
> tip...@li...
> Subject: [net-next 3/3] tipc: introduce variable window congestion control
>
> We introduce a simple variable window congestion control for links.
> The algorithm is inspired by the Reno algorithm, covering both 'slow
> start', 'congestion avoidance', and 'fast recovery' modes.
>
> - We introduce hard lower and upper window limits per link, still
>    different and configurable per bearer type.
>
> - We introduce as 'slow start theshold' variable, initially set to
>    the maximum window size.
>
> - We let a link start at the minimum congestion window, i.e. in slow
>    start mode, and then let is grow rapidly (+1 per rceived ACK) until
>    it reaches the slow start threshold and enters congestion avoidance
>    mode.
>
> - In congestion avoidance mode we increment the congestion window for
>    each window_size number of acked packets, up to a possible maximum
>    equal to the configured maximum window.
>
> - For each non-duplicate NACK received, we drop back to fast recovery
>    mode, by setting the both the slow start threshold to and the
>    congestion window to (current_congestion_window / 2).
>
> - If the timeout handler finds that the transmit queue has not moved
>    timeout, it drops the link back to slow start and forces a probe
>    containing the last sent sequence number to the sent to the peer.
>
> This change does in reality have effect only on unicast ethernet
> transport, as we have seen that there is no room whatsoever for
> increasing the window max size for the UDP bearer.
> For now, we also choose to keep the limits for the broadcast link
> unchanged and equal.
>
> This algorithm seems to give a 50-100% throughput improvement for
> messages larger than MTU.
>
> Suggested-by: Xin Long <luc...@gm...>
> Acked-by: Xin Long <luc...@gm...>
> Signed-off-by: Jon Maloy <jon...@er...>
> ---
>   net/tipc/bcast.c     |  11 ++--
>   net/tipc/bearer.c    |  11 ++--
>   net/tipc/bearer.h    |   6 +-
>   net/tipc/eth_media.c |   3 +-
>   net/tipc/ib_media.c  |   5 +-
>   net/tipc/link.c      | 175
> +++++++++++++++++++++++++++++++++++----------------
>   net/tipc/link.h      |   9 +--
>   net/tipc/node.c      |  16 ++---
>   net/tipc/udp_media.c |   3 +-
>   9 files changed, 160 insertions(+), 79 deletions(-)
>
>
>
>
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>