| Message ID | e5d77a8d7c30ccbcc113506837ae7b8e90625046.1440329440.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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. :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> >>> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)