On 6/30/20 9:39 PM, Tuong Tong Lien wrote:
-----Original Message-----
From: Jon Maloy <jma...@redhat.com>
Sent: Tuesday, June 30, 2020 11:26 PM
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: [net] tipc: fix incorrect unicast link window
On 6/30/20 12:56 AM, Tuong Lien wrote:
In commit 16ad3f4022bb ("tipc: introduce variable window congestion
control"), we enable user to set the link 'max_win' value which is used
as the upper threshold for the variable link window algorithm later.
However, since it is done by the same netlink command property
'TIPC_NLA_PROP_WIN' that was used to set the actual link window before,
it appears to be a non backward compatible issue when user tries to get
the value back but finds a different value (i.e. the variable window at
that moment).
Besides, there is another flaw with the 'max_win' where it is set to be
'TIPC_MAX_LINK_WIN' (i.e. 8191) by default that is obviously unexpected
(the variable link window will take place and go beyond that might harm
the underlying device...). The value is actually derived from the lower
bearer & media layers (i.e. 'eth' media) at the initializing time where
the default value should be 'TIPC_DEF_LINK_WIN' (i.e. 50) instead.
We fix the issue #1 by returning the 'max_win' which is exactly the one
set by user, while setting back the 'max_win' for the 'eth' media to be
the said value for the #2.
Note: the changes do not affect the variable link window mechanism, but
make the right thing i.e. it will work only when user really wants.
If understand this right variable window flow control will now in
practice be disabled by default, and only be activated if the user
explicitly sets max_win to something >> 50. Right?
Yes, I suppose this is the right way, the default value should be at 50 as
before and user has its control fully. Or, do you want it to be 8181, but I
think it is too big by default and we will need to explain it somehow because
one will certainly be surprised to see that value in TIPC printouts... ?
One important property of TIPC is to keep all need for configuration at
a minimum. If a user explicitly has to set a configuration value to make
TIPC performance optimal (and we know now that the difference between a
fix 50 buffer window and a variable one is huge) I don't find that very
satisfactory.
But I admit we have a dilemma. What we have done is to change the
semantic meaning of setting/getting the window from "fix sliding
protocol window" to "max allowable window". I can quite well understand
that someone within /// found it confusing to discover that a value that
was previously 50 now is 8181, and wants it back to how it used to be.
However, I don't think this kind of "user expectation" about a deeply
internal setting of TIPC is a valid reason to cripple protocol
performance. In the past the users would set this value to achieve
better performance, while it in reality is irrelevant now, -it can only
make performance poorer.
In my view we have two options:
1) We could add a dummy value that sets and reads the user's value just
to make him happy.
2) We could set the default value to e.g. 1000 to make it look more
reasonable to the user, but with no practical effect on the protocol.
Then, if the user has his own legacy setting at e.g. 300 he will
probably discover poorer performance, and choose to keep the default
value. And if he doesn't then it is his own problem.
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.
///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
