> -----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:
> >
> >
> > 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.
> >
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 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.
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" 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...)?
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
