On 7/2/20 6:28 PM, Jon Maloy wrote:
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.
This has to be verified of course.
///jon
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
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
