On 3/18/20 12:50 AM, Tuong Lien Tong wrote:
Hi Jon,
Ok, that makes sense (but we should have covered the case a broadcast packet is
released too...).
However, I have another concern about the logic here:
+ /* Enter fast recovery */
+ if (unlikely(retransmitted)) {
+ l->ssthresh = max_t(u16, l->window / 2, 300);
+ l->window = l->ssthresh;
+ return;
+ }
What will if we have a retransmission when it's still in the slow-start phase?
For example:
l->ssthresh = 300
l-> window = 60
==> retransmitted = true, then: l->ssthresh = 300; l->window = 300???
This looks not correct?
Should it be:
+ /* Enter fast recovery */
+ if (unlikely(retransmitted)) {
+ l->ssthresh = max_t(u16, l->window / 2, 300);
- l->window = l->ssthresh;
+ l->window = min_t(u16, l->window, l->ssthresh);
+ return;
+ }
So will fix the issue with broadcast case as well?
Yes, this would fix both issues, so I think it is a good suggestion.
To my surprise I have realized that this code has not been released yet
(I only find it in 5.6-rc1 and later versions) but maybe that is just as
well ;-)
///jon
BR/Tuong
-----Original Message-----
From: Jon Maloy <[email protected]>
Sent: Wednesday, March 18, 2020 1:38 AM
To: Tuong Lien Tong <[email protected]>; 'Jon Maloy'
<[email protected]>; 'Jon Maloy' <[email protected]>
Cc: [email protected];
[email protected]
Subject: Re: [tipc-discussion] [net-next 3/3] tipc: introduce variable window
congestion control
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 <[email protected]>
Sent: Monday, December 2, 2019 7:33 AM
To: Jon Maloy <[email protected]>; Jon Maloy <[email protected]>
Cc: [email protected];
[email protected]; [email protected];
[email protected]; [email protected];
[email protected]; [email protected];
[email protected]
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 <[email protected]>
Acked-by: Xin Long <[email protected]>
Signed-off-by: Jon Maloy <[email protected]>
---
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
_______________________________________________
tipc-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
