sctp: make the max_burst min value to 1

PATCH In review Wang Weidong 6 participants 13 messages Dec 2, 2013 – Dec 6, 2013

Subsystems (3)

Networking Network drivers SCTP protocol

lore

Share & tools

lore

Wang Weidong

author

2013-12-02 06:51 UTC #0 lore raw

PATCH 1/1 sctp: make the max_burst min value to 1

From: Wang Weidong <[email protected]>

when I setted the max_burst to 0, do the lksctp-tools I got hang. I found sctp_transport_burst_limited would make the cwnd to 0. so I make the max_burst min value to 1.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sctp/sysctl.c +1 -1

index 7637e8e..46832d3 100644
@@ -135,7 +135,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &int_max
 	},
 	{

Signature · 1 line

1.7.12

Neil Horman

2013-12-02 11:56 UTC #1 lore raw

Quoted 27 lines · Wang Weidong

From: Wang Weidong <[email protected]>
when I setted the max_burst to 0, do the lksctp-tools I got hang. I found sctp_transport_burst_limited would make the cwnd to 0. so I make the max_burst min value to 1.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/sysctl.c +1 -1
index 7637e8e..46832d3 100644
@@ -135,7 +135,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &int_max
 	},
 	{
Signature · 1 line
1.7.12

This seems like a band-aid to me. There are a few things wrong:

1) You can also set the the max_burst via setsockopt, and so this would need to be checked in that path as well.

2) I don't see how having a cwnd of zero would cause a hang. It looks like a cwnd of zero would perpetually place the association in a slow start state, which is silly but not illegal.

Please investigate the acutally root cause of the problem before just avoiding it like this.

Thanks!
Neil

Wang Weidong

author

2013-12-02 12:03 UTC #2 lore raw

Quoted 32 lines · Neil Horman

Quoted 27 lines · Wang Weidong
From: Wang Weidong <[email protected]>
when I setted the max_burst to 0, do the lksctp-tools I got hang. I found sctp_transport_burst_limited would make the cwnd to 0. so I make the max_burst min value to 1.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/sysctl.c +1 -1
index 7637e8e..46832d3 100644
@@ -135,7 +135,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &int_max
 	},
 	{
Signature · 1 line
1.7.12
This seems like a band-aid to me. There are a few things wrong:

Quoted 7 lines

1) You can also set the the max_burst via setsockopt, and so this would need to be checked in that path as well.
2) I don't see how having a cwnd of zero would cause a hang. It looks like a cwnd of zero would perpetually place the association in a slow start state, which is silly but not illegal.

Hm, Good suggestions. Ok, I will try it again and find the root cause. Thanks!

Quoted 12 lines

Please investigate the acutally root cause of the problem before just avoiding it like this.
Thanks!
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vlad Yasevich

2013-12-02 14:51 UTC #3 lore raw

Quoted 36 lines · Wang Weidong

Quoted 35 lines · Neil Horman
Quoted 27 lines · Wang Weidong
From: Wang Weidong <[email protected]>
when I setted the max_burst to 0, do the lksctp-tools I got hang. I found sctp_transport_burst_limited would make the cwnd to 0. so I make the max_burst min value to 1.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/sysctl.c +1 -1
index 7637e8e..46832d3 100644
@@ -135,7 +135,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &int_max
 	},
 	{
Signature · 1 line
1.7.12
This seems like a band-aid to me. There are a few things wrong:
1) You can also set the the max_burst via setsockopt, and so this would need to be checked in that path as well.

Quoted 7 lines

Quoted 4 lines
2) I don't see how having a cwnd of zero would cause a hang. It looks like a cwnd of zero would perpetually place the association in a slow start state, which is silly but not illegal.
Hm, Good suggestions. Ok, I will try it again and find the root cause. Thanks!

It's really simple. sctp_transport_burst_limited() should simply do nothing if max_burst is 0, essentially allowing unlimited bursts.

-vlad

Quoted 15 lines

Quoted 13 lines
Please investigate the acutally root cause of the problem before just avoiding it like this.
Thanks!
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wang Weidong

author

2013-12-03 01:35 UTC #4 lore raw

Quoted 42 lines · Vlad Yasevich

Quoted 41 lines · Wang Weidong
Quoted 39 lines · Neil Horman
Quoted 27 lines · Wang Weidong
From: Wang Weidong <[email protected]>
when I setted the max_burst to 0, do the lksctp-tools I got hang. I found sctp_transport_burst_limited would make the cwnd to 0. so I make the max_burst min value to 1.
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/sysctl.c +1 -1
index 7637e8e..46832d3 100644
@@ -135,7 +135,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &int_max
 	},
 	{
Signature · 1 line
1.7.12
This seems like a band-aid to me. There are a few things wrong:
1) You can also set the the max_burst via setsockopt, and so this would need to be checked in that path as well.
2) I don't see how having a cwnd of zero would cause a hang. It looks like a cwnd of zero would perpetually place the association in a slow start state, which is silly but not illegal.

Quoted 8 lines

Quoted 2 lines
Hm, Good suggestions. Ok, I will try it again and find the root cause. Thanks!
It's really simple. sctp_transport_burst_limited() should simply do nothing if max_burst is 0, essentially allowing unlimited bursts.
-vlad

I will add the a check of max_burst, if max_burst is 0, just do nothing. Michael point out that it just disable max_burst which declared in rfc6458#section-8.1.24 as well.

Thanks.

Quoted 22 lines

Quoted 7 lines
Quoted 7 lines
Please investigate the acutally root cause of the problem before just avoiding it like this.
Thanks!
Neil
Quoted 8 lines
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michael Tuexen

2013-12-02 13:08 UTC #5 lore raw

In reply to Neil Horman

Quoted 35 lines · Neil Horman

Quoted 27 lines · Wang Weidong
From: Wang Weidong <[email protected]>
when I setted the max_burst to 0, do the lksctp-tools I got hang. I found sctp_transport_burst_limited would make the cwnd to 0. so I make the max_burst min value to 1.
Signed-off-by Wang Weidong <[email protected]>
---
net/sctp/sysctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/sysctl.c +1 -1
index 7637e8e..46832d3 100644
@@ -135,7 +135,7 @@ static struct ctl_table sctp_net_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
+		.extra1		= &one,
 		.extra2		= &int_max
 	},
 	{
Signature · 1 line
1.7.12
This seems like a band-aid to me. There are a few things wrong:
1) You can also set the the max_burst via setsockopt, and so this would need to be checked in that path as well.

Quoted 6 lines

2) I don't see how having a cwnd of zero would cause a hang. It looks like a cwnd of zero would perpetually place the association in a slow start state, which is silly but not illegal.
Please investigate the acutally root cause of the problem before just avoiding it like this.

I second this.

Please note the using max_burst of in the socket option means that you deactivate max_burst, not that you actually use the value 0. See
http://tools.ietf.org/html/rfc6458#section-8.1.24

I FreeBSD we use the sysctl variable as the default for sockets. Therefore max_burst is a valid value, it just disables max_burst.

Best regards
Michael

Quoted 9 lines

Thanks!
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wang Weidong

author

2013-12-03 06:30 UTC #6 lore raw

PATCH 1/1 sctp: disable max_burst when the max_burst is 0

As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/transport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/sctp/transport.c +2 -1

index e332efb..e580e34 100644
@@ -573,7 +573,8 @@ void sctp_transport_burst_limited(struct sctp_transport *t)
 	u32 old_cwnd = t->cwnd;
 	u32 max_burst_bytes;
 
-	if (t->burst_limited)
+	/* if the max_burst is 0, do notihing */
+	if (t->burst_limited || !asoc->max_burst)
 		return;
 
 	max_burst_bytes = t->flight_size + (asoc->max_burst * asoc->pathmtu);

Signature · 1 line

1.7.12

Daniel Borkmann

2013-12-04 08:57 UTC #7 lore raw

Re: [PATCH] sctp: disable max_burst when the max_burst is 0

Quoted 16 lines · Wang Weidong

As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.
Suggested-by Vlad Yasevich <[email protected]>
Suggested-by Michael Tuexen <[email protected]>
Signed-off-by Wang Weidong <[email protected]>
---
  net/sctp/transport.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
net/sctp/transport.c +0 -0
index e332efb..e580e34 100644

net/sctp/transport.c void sctp_transport_burst_limited(struct sctp_transport *t) · line 578

Quoted 7 lines

net/sctp/transport.c +2 -1

@@ -573,7 +573,8 @@ void sctp_transport_burst_limited(struct sctp_transport *t)
  	u32 old_cwnd = t->cwnd;
  	u32 max_burst_bytes;
 
-	if (t->burst_limited)
+	/* if the max_burst is 0, do notihing */
+	if (t->burst_limited || !asoc->max_burst)
  		return;

Minor nitpick ... you can remove the comment as it contains a typo and basically says what the code is doing anyway, so you could instead make the code a bit more clear for readers e.g. "asoc->max_burst = 0".

Quoted 2 lines

net/sctp/transport.c +0 -0

  	max_burst_bytes = t->flight_size + (asoc->max_burst * asoc->pathmtu);

Wang Weidong

author

2013-12-04 09:16 UTC #8 lore raw

Re: [PATCH] sctp: disable max_burst when the max_burst is 0

Quoted 20 lines · Daniel Borkmann

Quoted 19 lines · Wang Weidong
As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.
Suggested-by Vlad Yasevich <[email protected]>
Suggested-by Michael Tuexen <[email protected]>
Signed-off-by Wang Weidong <[email protected]>
---
  net/sctp/transport.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
net/sctp/transport.c +0 -0
index e332efb..e580e34 100644
@@ -573,7 +573,8 @@ void sctp_transport_burst_limited(struct sctp_transport *t)
      u32 old_cwnd = t->cwnd;
      u32 max_burst_bytes;
 

Quoted 9 lines

net/sctp/transport.c void sctp_transport_burst_limited(struct sctp_transport *t) · line 578
Quoted 4 lines
net/sctp/transport.c +2 -1
-    if (t->burst_limited)
+    /* if the max_burst is 0, do notihing */
+    if (t->burst_limited || !asoc->max_burst)
          return;
Minor nitpick ... you can remove the comment as it contains a typo and basically says what the code is doing anyway, so you could instead make the code a bit more clear for readers e.g. "asoc->max_burst = 0".

Nice, Thanks for pointing it out for me. I will fix it in v2.

Regards.

Quoted 5 lines

net/sctp/transport.c void sctp_transport_burst_limited(struct sctp_transport *t) · line 580
Quoted 2 lines
net/sctp/transport.c +0 -0
      max_burst_bytes = t->flight_size + (asoc->max_burst * asoc->pathmtu);
 
.

Wang Weidong

author

2013-12-04 09:32 UTC #9 lore raw

PATCH v2 1/1 sctp: disable max_burst when the max_burst is 0

In reply to Wang Weidong

As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.

Reviewed-by Daniel Borkmann <[email protected]>

Signed-off-by Wang Weidong <[email protected]>

---
 net/sctp/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sctp/transport.c +1 -1

index e332efb..efc46ff 100644
@@ -573,7 +573,7 @@ void sctp_transport_burst_limited(struct sctp_transport *t)
 	u32 old_cwnd = t->cwnd;
 	u32 max_burst_bytes;
 
-	if (t->burst_limited)
+	if (t->burst_limited || asoc->max_burst = 0)
 		return;
 
 	max_burst_bytes = t->flight_size + (asoc->max_burst * asoc->pathmtu);

Signature · 1 line

1.7.12

Neil Horman

Acked-by on v2 1/1

2013-12-04 11:45 UTC #10 lore raw

Re: [PATCH v2] sctp: disable max_burst when the max_burst is 0

Quoted 31 lines · Wang Weidong

As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.
Reviewed-by Daniel Borkmann <[email protected]>
Suggested-by Vlad Yasevich <[email protected]>
Suggested-by Michael Tuexen <[email protected]>
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/transport.c +1 -1
index e332efb..efc46ff 100644
@@ -573,7 +573,7 @@ void sctp_transport_burst_limited(struct sctp_transport *t)
 	u32 old_cwnd = t->cwnd;
 	u32 max_burst_bytes;
 
-	if (t->burst_limited)
+	if (t->burst_limited || asoc->max_burst = 0)
 		return;
 
 	max_burst_bytes = t->flight_size + (asoc->max_burst * asoc->pathmtu);
Signature · 1 line
1.7.12

Acked-by Neil Horman <[email protected]>

Vlad Yasevich

Acked-by on v2 1/1

2013-12-04 14:28 UTC #11 lore raw

Re: [PATCH v2] sctp: disable max_burst when the max_burst is 0

In reply to Wang Weidong

Quoted 18 lines · Wang Weidong

As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.
Reviewed-by Daniel Borkmann <[email protected]>
Suggested-by Vlad Yasevich <[email protected]>
Suggested-by Michael Tuexen <[email protected]>
Signed-off-by Wang Weidong <[email protected]>
---
 net/sctp/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/sctp/transport.c +0 -0
index e332efb..efc46ff 100644

net/sctp/transport.c void sctp_transport_burst_limited(struct sctp_transport *t) · line 579

Quoted 8 lines

net/sctp/transport.c +1 -1

@@ -573,7 +573,7 @@ void sctp_transport_burst_limited(struct sctp_transport *t)
 	u32 old_cwnd = t->cwnd;
 	u32 max_burst_bytes;
 
-	if (t->burst_limited)
+	if (t->burst_limited || asoc->max_burst = 0)
 		return;
 
 	max_burst_bytes = t->flight_size + (asoc->max_burst * asoc->pathmtu);

Acked-by Vlad Yasevich <[email protected]>

Thanks
-vlad

David Miller

maintainer Applied

2013-12-06 01:56 UTC #12 lore raw

Re: [PATCH v2] sctp: disable max_burst when the max_burst is 0

In reply to Wang Weidong

From: Wang Weidong <[email protected]>
Date: Wed, 4 Dec 2013 17:32:39 +0800

Quoted 8 lines

As Michael pointed out that when max_burst is 0, it just disable max_burst. It declared in rfc6458#section-8.1.24. so add the check in sctp_transport_burst_limited, when it 0, just do nothing.
Reviewed-by Daniel Borkmann <[email protected]>
Suggested-by Vlad Yasevich <[email protected]>
Suggested-by Michael Tuexen <[email protected]>
Signed-off-by Wang Weidong <[email protected]>

Applied, thank you.