[PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Xin Long]

when A sends a data to B, then A close() and enter into SHUTDOWN_PENDING state,
if B neither claim his rwnd is 0 nor send SACK for this data, A will keep
retransmitting this data util t5 timeout, Max.Retrans times can't work anymore,
which is bad.

if B's rwnd is not 0, it should send abord after Max.Retrans times, only when
B's rwnd == 0 and A's retransmitting beyonds Max.Retrans times, A will start
t5 timer, which is also commit f8d960524 means, but it lacks the condition
peer.rwnd == 0.

Fixes: f8d960524 ("sctp: Enforce retransmission limit during shutdown")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_statefuns.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 3ee27b7..deb9eab 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -5412,7 +5412,8 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(struct net *net,
 	SCTP_INC_STATS(net, SCTP_MIB_T3_RTX_EXPIREDS);
 
 	if (asoc->overall_error_count >= asoc->max_retrans) {
-		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
+		if (!q->asoc->peer.rwnd &&
+		    asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
 			/*
 			 * We are here likely because the receiver had its rwnd
 			 * closed for a while and we have not been able to
-- 
2.1.0

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Marcelo Ricardo Leitner]

On Sun, Aug 23, 2015 at 07:30:40PM +0800, Xin Long wrote:
> when A sends a data to B, then A close() and enter into SHUTDOWN_PENDING state,
> if B neither claim his rwnd is 0 nor send SACK for this data, A will keep
> retransmitting this data util t5 timeout, Max.Retrans times can't work anymore,
> which is bad.
> 
> if B's rwnd is not 0, it should send abord after Max.Retrans times, only when
> B's rwnd == 0 and A's retransmitting beyonds Max.Retrans times, A will start
> t5 timer, which is also commit f8d960524 means, but it lacks the condition
> peer.rwnd == 0.
> 
> Fixes: f8d960524 ("sctp: Enforce retransmission limit during shutdown")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

You still have typos on changelog, but at least not on keywords.
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/sm_statefuns.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 3ee27b7..deb9eab 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -5412,7 +5412,8 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(struct net *net,
>  	SCTP_INC_STATS(net, SCTP_MIB_T3_RTX_EXPIREDS);
>  
>  	if (asoc->overall_error_count >= asoc->max_retrans) {
> -		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> +		if (!q->asoc->peer.rwnd &&
> +		    asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
>  			/*
>  			 * We are here likely because the receiver had its rwnd
>  			 * closed for a while and we have not been able to
> -- 
> 2.1.0
> 

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Vlad Yasevich]

On 08/23/2015 07:30 AM, Xin Long wrote:
> when A sends a data to B, then A close() and enter into SHUTDOWN_PENDING state,
> if B neither claim his rwnd is 0 nor send SACK for this data, A will keep
> retransmitting this data util t5 timeout, Max.Retrans times can't work anymore,
> which is bad.
> 
> if B's rwnd is not 0, it should send abord after Max.Retrans times, only when
> B's rwnd == 0 and A's retransmitting beyonds Max.Retrans times, A will start
> t5 timer, which is also commit f8d960524 means, but it lacks the condition
> peer.rwnd == 0.
> 
> Fixes: f8d960524 ("sctp: Enforce retransmission limit during shutdown")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/sm_statefuns.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 3ee27b7..deb9eab 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -5412,7 +5412,8 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(struct net *net,
>  	SCTP_INC_STATS(net, SCTP_MIB_T3_RTX_EXPIREDS);
>  
>  	if (asoc->overall_error_count >= asoc->max_retrans) {
> -		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> +		if (!q->asoc->peer.rwnd &&
> +		    asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
>  			/*
>  			 * We are here likely because the receiver had its rwnd
>  			 * closed for a while and we have not been able to
> 

This may not work as expected.  peer.rwnd is the calculated peer window, but it
also gets updated when we receive sacks.  So there is no way to tell that
the current windows is 0 because peer told us, or because we sent data to make 0
and the peer hasn't responded.

-vlad

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Marcelo Ricardo Leitner]

On Mon, Aug 24, 2015 at 02:13:38PM -0400, Vlad Yasevich wrote:
> On 08/23/2015 07:30 AM, Xin Long wrote:
> > when A sends a data to B, then A close() and enter into SHUTDOWN_PENDING state,
> > if B neither claim his rwnd is 0 nor send SACK for this data, A will keep
> > retransmitting this data util t5 timeout, Max.Retrans times can't work anymore,
> > which is bad.
> > 
> > if B's rwnd is not 0, it should send abord after Max.Retrans times, only when
> > B's rwnd == 0 and A's retransmitting beyonds Max.Retrans times, A will start
> > t5 timer, which is also commit f8d960524 means, but it lacks the condition
> > peer.rwnd == 0.
> > 
> > Fixes: f8d960524 ("sctp: Enforce retransmission limit during shutdown")
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/sm_statefuns.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> > index 3ee27b7..deb9eab 100644
> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
> > @@ -5412,7 +5412,8 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(struct net *net,
> >  	SCTP_INC_STATS(net, SCTP_MIB_T3_RTX_EXPIREDS);
> >  
> >  	if (asoc->overall_error_count >= asoc->max_retrans) {
> > -		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> > +		if (!q->asoc->peer.rwnd &&
> > +		    asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> >  			/*
> >  			 * We are here likely because the receiver had its rwnd
> >  			 * closed for a while and we have not been able to
> > 
> 
> This may not work as expected.  peer.rwnd is the calculated peer window, but it
> also gets updated when we receive sacks.  So there is no way to tell that
> the current windows is 0 because peer told us, or because we sent data to make 0
> and the peer hasn't responded.

I'm not sure I follow you, Vlad. I don't think we care on why we have
zero-window in there, just that if we are at it on that stage. Either
one, if it's zero window, we will go through T5 and give it more time to
recover, but if it's not zero window, I don't see a reason to enable T5..

  Marcelo

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Vlad Yasevich]

On 08/24/2015 02:31 PM, Marcelo Ricardo Leitner wrote:
> On Mon, Aug 24, 2015 at 02:13:38PM -0400, Vlad Yasevich wrote:
>> On 08/23/2015 07:30 AM, Xin Long wrote:
>>> when A sends a data to B, then A close() and enter into SHUTDOWN_PENDING state,
>>> if B neither claim his rwnd is 0 nor send SACK for this data, A will keep
>>> retransmitting this data util t5 timeout, Max.Retrans times can't work anymore,
>>> which is bad.
>>>
>>> if B's rwnd is not 0, it should send abord after Max.Retrans times, only when
>>> B's rwnd == 0 and A's retransmitting beyonds Max.Retrans times, A will start
>>> t5 timer, which is also commit f8d960524 means, but it lacks the condition
>>> peer.rwnd == 0.
>>>
>>> Fixes: f8d960524 ("sctp: Enforce retransmission limit during shutdown")
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>  net/sctp/sm_statefuns.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
>>> index 3ee27b7..deb9eab 100644
>>> --- a/net/sctp/sm_statefuns.c
>>> +++ b/net/sctp/sm_statefuns.c
>>> @@ -5412,7 +5412,8 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(struct net *net,
>>>  	SCTP_INC_STATS(net, SCTP_MIB_T3_RTX_EXPIREDS);
>>>  
>>>  	if (asoc->overall_error_count >= asoc->max_retrans) {
>>> -		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
>>> +		if (!q->asoc->peer.rwnd &&
>>> +		    asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
>>>  			/*
>>>  			 * We are here likely because the receiver had its rwnd
>>>  			 * closed for a while and we have not been able to
>>>
>>
>> This may not work as expected.  peer.rwnd is the calculated peer window, but it
>> also gets updated when we receive sacks.  So there is no way to tell that
>> the current windows is 0 because peer told us, or because we sent data to make 0
>> and the peer hasn't responded.
> 
> I'm not sure I follow you, Vlad. I don't think we care on why we have
> zero-window in there, just that if we are at it on that stage. Either
> one, if it's zero window, we will go through T5 and give it more time to
> recover, but if it's not zero window, I don't see a reason to enable T5..

No, these are 2 distinct instances.  In one instance, the peer is reachable and
is able to communication 0 rwnd state to us.  Thus we are being nice and granting
the peer more time to exit the 0 window state.

In the other state, the peer is unreachable and we just happen to hit the 0-window
condition based on some estimations of the peer window.  In this case, we should
be subject to the Max.RTX and terminate the association sooner.

-vlad

> 
>   Marcelo
> 

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Marcelo Ricardo Leitner]

On Mon, Aug 24, 2015 at 02:36:59PM -0400, Vlad Yasevich wrote:
> On 08/24/2015 02:31 PM, Marcelo Ricardo Leitner wrote:
> > On Mon, Aug 24, 2015 at 02:13:38PM -0400, Vlad Yasevich wrote:
> >> On 08/23/2015 07:30 AM, Xin Long wrote:
> >>> when A sends a data to B, then A close() and enter into SHUTDOWN_PENDING state,
> >>> if B neither claim his rwnd is 0 nor send SACK for this data, A will keep
> >>> retransmitting this data util t5 timeout, Max.Retrans times can't work anymore,
> >>> which is bad.
> >>>
> >>> if B's rwnd is not 0, it should send abord after Max.Retrans times, only when
> >>> B's rwnd == 0 and A's retransmitting beyonds Max.Retrans times, A will start
> >>> t5 timer, which is also commit f8d960524 means, but it lacks the condition
> >>> peer.rwnd == 0.
> >>>
> >>> Fixes: f8d960524 ("sctp: Enforce retransmission limit during shutdown")
> >>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >>> ---
> >>>  net/sctp/sm_statefuns.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> >>> index 3ee27b7..deb9eab 100644
> >>> --- a/net/sctp/sm_statefuns.c
> >>> +++ b/net/sctp/sm_statefuns.c
> >>> @@ -5412,7 +5412,8 @@ sctp_disposition_t sctp_sf_do_6_3_3_rtx(struct net *net,
> >>>  	SCTP_INC_STATS(net, SCTP_MIB_T3_RTX_EXPIREDS);
> >>>  
> >>>  	if (asoc->overall_error_count >= asoc->max_retrans) {
> >>> -		if (asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> >>> +		if (!q->asoc->peer.rwnd &&
> >>> +		    asoc->state == SCTP_STATE_SHUTDOWN_PENDING) {
> >>>  			/*
> >>>  			 * We are here likely because the receiver had its rwnd
> >>>  			 * closed for a while and we have not been able to
> >>>
> >>
> >> This may not work as expected.  peer.rwnd is the calculated peer window, but it
> >> also gets updated when we receive sacks.  So there is no way to tell that
> >> the current windows is 0 because peer told us, or because we sent data to make 0
> >> and the peer hasn't responded.
> > 
> > I'm not sure I follow you, Vlad. I don't think we care on why we have
> > zero-window in there, just that if we are at it on that stage. Either
> > one, if it's zero window, we will go through T5 and give it more time to
> > recover, but if it's not zero window, I don't see a reason to enable T5..
> 
> No, these are 2 distinct instances.  In one instance, the peer is reachable and
> is able to communication 0 rwnd state to us.  Thus we are being nice and granting
> the peer more time to exit the 0 window state.
> 
> In the other state, the peer is unreachable and we just happen to hit the 0-window
> condition based on some estimations of the peer window.  In this case, we should
> be subject to the Max.RTX and terminate the association sooner.

Makes sense, we can do better in there. Thanks Vlad.

  Marcelo

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[lucien xin]

>
> No, these are 2 distinct instances.  In one instance, the peer is reachable and
> is able to communication 0 rwnd state to us.  Thus we are being nice and granting
> the peer more time to exit the 0 window state.
>
> In the other state, the peer is unreachable and we just happen to hit the 0-window
> condition based on some estimations of the peer window.  In this case, we should
> be subject to the Max.RTX and terminate the association sooner.
>
> -vlad
>
okay, I got you,

we can see that local update their peer.rwnd in sctp_packet_append_data() and
sctp_retransmit_mark(), it do that according to a_rwnd and outstanding, so the
root reason is that it's hard to know that peer really closed it's window, maybe
just so many outstanding lead to that.

what we can do is to trust peer.rwnd is the real window in peer.
from another angle,  even though it's not real, at least we can reduce the
* the other state* you mentioned by doing this. especially, if there is only one
small packet keep retransmitting in SHUTDOWN_PENDING state, the
peer.rwnd is more believable to be the real peer window.

I saw bsd code didnot care about Max.Retrans in SHUTDOWN_PENDING,
instead it just start T5 timer. but now that we choose Max.Retrans + T5, it's
better to process more unreachable by using Max.Retrans. I also hope we can
do it better there as Marcelo said, but by now I cannot see it. :)

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Vlad Yasevich]

On 08/27/2015 09:19 AM, lucien xin wrote:
>>
>> No, these are 2 distinct instances.  In one instance, the peer is reachable and
>> is able to communication 0 rwnd state to us.  Thus we are being nice and granting
>> the peer more time to exit the 0 window state.
>>
>> In the other state, the peer is unreachable and we just happen to hit the 0-window
>> condition based on some estimations of the peer window.  In this case, we should
>> be subject to the Max.RTX and terminate the association sooner.
>>
>> -vlad
>>
> okay, I got you,
> 
> we can see that local update their peer.rwnd in sctp_packet_append_data() and
> sctp_retransmit_mark(), it do that according to a_rwnd and outstanding, so the
> root reason is that it's hard to know that peer really closed it's window, maybe
> just so many outstanding lead to that.
> 
> what we can do is to trust peer.rwnd is the real window in peer.
> from another angle,  even though it's not real, at least we can reduce the
> * the other state* you mentioned by doing this. especially, if there is only one
> small packet keep retransmitting in SHUTDOWN_PENDING state, the
> peer.rwnd is more believable to be the real peer window.
> 
> I saw bsd code didnot care about Max.Retrans in SHUTDOWN_PENDING,
> instead it just start T5 timer. but now that we choose Max.Retrans + T5, it's
> better to process more unreachable by using Max.Retrans. I also hope we can
> do it better there as Marcelo said, but by now I cannot see it. :)
> 

So one potential way is to have peer.rwnd and peer.a_rwnd, where peer.a_rwnd is
the window advertised by peer and peer.rwnd and our estimation based on peer.a_rwnd.
This way we will always know where we stand.

Although I am not sure yet if we want to grow the peer structure any more.

Another way is to have an estimate or 0-window probe bit/flags one the send side
and set it when we do 0-window probe.  This way we'd know that when 0-window probe
bit is set, peer returned 0 window.

Just some thoughts.
-vlad

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[lucien xin]

>
> So one potential way is to have peer.rwnd and peer.a_rwnd, where peer.a_rwnd is
> the window advertised by peer and peer.rwnd and our estimation based on peer.a_rwnd.
> This way we will always know where we stand.
>
> Although I am not sure yet if we want to grow the peer structure any more.
>
> Another way is to have an estimate or 0-window probe bit/flags one the send side
> and set it when we do 0-window probe.  This way we'd know that when 0-window probe
> bit is set, peer returned 0 window.
>
I think updating 0-window may happen in sctp_process_init() and
sctp_outq_sack(),
I don't think 0-window can be probed, cause unreachable and closing
window both has
no reply from peer. but we can update the 0-window bit in those two
functions. I just do
not know where there is a available bit we can use if won't change the
peer structure.

> Just some thoughts.
> -vlad

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[Vlad Yasevich]

On 08/27/2015 10:49 AM, lucien xin wrote:
>>
>> So one potential way is to have peer.rwnd and peer.a_rwnd, where peer.a_rwnd is
>> the window advertised by peer and peer.rwnd and our estimation based on peer.a_rwnd.
>> This way we will always know where we stand.
>>
>> Although I am not sure yet if we want to grow the peer structure any more.
>>
>> Another way is to have an estimate or 0-window probe bit/flags one the send side
>> and set it when we do 0-window probe.  This way we'd know that when 0-window probe
>> bit is set, peer returned 0 window.
>>
> I think updating 0-window may happen in sctp_process_init() and
> sctp_outq_sack(),
> I don't think 0-window can be probed, cause unreachable and closing
> window both has
> no reply from peer. but we can update the 0-window bit in those two
> functions. I just do
> not know where there is a available bit we can use if won't change the
> peer structure.

You can ignore INIT as the window will never be 0 (not allowed).

The updates could happen at the end of sctp_outq_sack().   There some spare
bits in peer if you want to go that way.

-vlad


> 
>> Just some thoughts.
>> -vlad

Re: [PATCH net v2] sctp: start t5 timer only when peer.rwnd is 0 and local.state is SHUTDOWN_PENDING

[lucien xin]

>>>
>>> So one potential way is to have peer.rwnd and peer.a_rwnd, where peer.a_rwnd is
>>> the window advertised by peer and peer.rwnd and our estimation based on peer.a_rwnd.
>>> This way we will always know where we stand.
>>>
>>> Although I am not sure yet if we want to grow the peer structure any more.
>>>
>>> Another way is to have an estimate or 0-window probe bit/flags one the send side
>>> and set it when we do 0-window probe.  This way we'd know that when 0-window probe
>>> bit is set, peer returned 0 window.
>>>
>> I think updating 0-window may happen in sctp_process_init() and
>> sctp_outq_sack(),
>> I don't think 0-window can be probed, cause unreachable and closing
>> window both has
>> no reply from peer. but we can update the 0-window bit in those two
>> functions. I just do
>> not know where there is a available bit we can use if won't change the
>> peer structure.
>
> You can ignore INIT as the window will never be 0 (not allowed).
>
> The updates could happen at the end of sctp_outq_sack().   There some spare
> bits in peer if you want to go that way.
>
I find this one *addip_disabled_mask*, but it's really bad to use it.
we should put a extensible mask or point there before.

as to a_rwnd, it's a good idea, but if we just use it here, it may not be worth
doing that. if there are a lot places need it, we can add it.