> -----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... ?
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
