On 7/3/20 12:48 AM, Tuong Tong Lien wrote:
-----Original Message-----
From: Jon Maloy <jma...@redhat.com>
Sent: Friday, July 3, 2020 5:36 AM
To: Tuong Tong Lien <tuong.t.l...@dektech.com.au>; ma...@donjonn.com;
ying....@windriver.com; tipc-
discuss...@lists.sourceforge.net
Cc: tipc-dek <tipc-...@dektech.com.au>
Subject: Re: [tipc-discussion] [net] tipc: fix incorrect unicast link window
On 7/2/20 6:28 PM, Jon Maloy wrote:
[...]
Thinking more about this, I would go for the second option.
Either way we will have to be ready to explain to the users why we
have made this change, and that there is no reason for them to touch
this value in the future.
An alternative is to introduce an additional property for user to set/get the
link max window which will be 8191 by default and keep the legacy one but for
the link min window instead. This way, we have no backward compatible issues
while still can go beyond that up to the max value which is fully under control
as well (if he/she wants to limit the threshold, e.g. for other traffic like
TCP, etc., he can consider to lower the max window...).
In addition, we will still retain the true meaning of the window setting
Changing the meaning of this setting from "maximum window" to "minimum
window" is for me a far bigger semantic change that changing it from
"maximum window" to "upper limit for maximum window" as we have done. I
fear this will be even more confusing for the user.
And we would have to set a limit "maximum minimum window" to avoid that
the user screws up performance. What should that be? Should we make even
that limit configurable? And some users might still see their current
setting being rejected, which I suspect is exactly what they want to
avoid, and the reason for this issue being raised at all.
from user that increasing the min window will help increase performance
somewhat (i.e. shorten the slow start phase) and vice versa, the same happens
for the max window setting as well.
I think the risk is obvious that the user anyway will run into
surprises. If he has set his window to e.g. 300 he will end up with no
slow start at all, or his setting being rejected. So he would be better
off removing this setting anyway.
We can also display the max value in the link statistics (for example, "Window: 50 packets, up to
8191" or "Window: 123 packets, range 50 - 8191") which acts like a "statement"
or maybe Window (min..cur..max): [50..123..1000] packets??
This is not a bad idea, whatever solution we choose.
saying that we currently support variable window and the actual link window can
be in that range instead of a fixed one as before... so, user will be
completely aware of this.
There is however a concern for the media & bearer layer's window setting
commands, do we need to apply the same change(s) for them as well (current it sets
and gets the max window value...)?
We would definitely have to change here too.
I would still suggest we go for my second proposal above. We select a
default value, e.g., 1000, where we see that there is no improvement in
increasing it further. Then we let the user set it differently if he
wants, any value between 50 and 8191.
This would be easy to explain to the user: "we have improved the
protocol so that we now can set the default window much larger than
before". How can he object to that, especially if he sees what can be
perceived as a sane value (1000 instead of 8191) ? The users should
*not* be allowed to dictate our internal default settings just because
they are unwilling to change their own. As long as we don't break
anything or reduce their current performance we remain backwards
compatible, in my view.
Finally, I apologize for not attending the meeting this morning. It is
at 6 AM for me, and easy to miss since it is bi-weekly. I will see if I
can set up Bluejens so that a moderator is not needed, or if we can use
some other tool. Any suggestions?
///jon
What do you think?
///jon
BR/Tuong
///jon
Fixes: 16ad3f4022bb ("tipc: introduce variable window congestion
control")
Reported-by: Hoang Le <hoang.h...@dektech.com.au>
Reported-by: Thang Ngo <thang.h....@dektech.com.au>
Signed-off-by: Tuong Lien <tuong.t.l...@dektech.com.au>
---
net/tipc/eth_media.c | 2 +-
net/tipc/link.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 8b0bb600602d..675b947eab89 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -93,7 +93,7 @@ struct tipc_media eth_media_info = {
.priority = TIPC_DEF_LINK_PRI,
.tolerance = TIPC_DEF_LINK_TOL,
.min_win = TIPC_DEF_LINK_WIN,
- .max_win = TIPC_MAX_LINK_WIN,
+ .max_win = TIPC_DEF_LINK_WIN,
.type_id = TIPC_MEDIA_TYPE_ETH,
.hwaddr_len = ETH_ALEN,
.name = "eth"
diff --git a/net/tipc/link.c b/net/tipc/link.c
index ee3b8d0576b8..28834dafdc98 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -2662,7 +2662,7 @@ int __tipc_nl_add_link(struct net *net,
struct tipc_nl_msg *msg,
if (nla_put_u32(msg->skb, TIPC_NLA_PROP_TOL, link->tolerance))
goto prop_msg_full;
if (nla_put_u32(msg->skb, TIPC_NLA_PROP_WIN,
- link->window))
+ link->max_win))
goto prop_msg_full;
if (nla_put_u32(msg->skb, TIPC_NLA_PROP_PRIO, link->priority))
goto prop_msg_full;
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
