Patchwork [v2,4/8] Add the no SACK route option feature

login
register
mail settings
Submitter Gilad Ben-Yossef
Date Oct. 21, 2009, 8:56 a.m.
Message ID <1256115421-12714-5-git-send-email-gilad@codefidence.com>
Download mbox | patch
Permalink /patch/36502/
State Rejected
Delegated to: David Miller
Headers show

Comments

Gilad Ben-Yossef - Oct. 21, 2009, 8:56 a.m.
Implement querying and acting upon the no sack bit in the features
field.

Signed-off-by: Gilad Ben-Yossef <gilad@codefidence.com>
Sigend-off-by: Ori Finkelman <ori@comsleep.com>
Sigend-off-by: Yony Amit <yony@comsleep.com>

---
 include/linux/rtnetlink.h |    2 +-
 net/ipv4/tcp_input.c      |    3 ++-
 net/ipv4/tcp_output.c     |    4 +++-
 3 files changed, 6 insertions(+), 3 deletions(-)
William Allen Simpson - Oct. 21, 2009, 7:22 p.m.
Gilad Ben-Yossef wrote:
> Implement querying and acting upon the no sack bit in the features
> field.
> 
>  #define RTAX_FEATURE_ECN	0x00000001
> -#define RTAX_FEATURE_SACK	0x00000002
> +#define RTAX_FEATURE_NO_SACK	0x00000002
>  #define RTAX_FEATURE_TIMESTAMP	0x00000004
>  #define RTAX_FEATURE_ALLFRAG	0x00000008
>  
I just realized that unlike NO_DSACK, this change assumes removing the
sysctl and defaulting on.  I'm opposed to removing this sysctl, so I'm
opposed to this change.

I'd prefer the ability to both turn on for global default off, and
turn off for global default on.  Shouldn't that be 2 different bits?

Or should this be a toggle?  How do other systems handle 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
Gilad Ben-Yossef - Oct. 25, 2009, 8:44 a.m.
William Allen Simpson wrote:

> Gilad Ben-Yossef wrote:
>> Implement querying and acting upon the no sack bit in the features
>> field.
>>
>>  #define RTAX_FEATURE_ECN    0x00000001
>> -#define RTAX_FEATURE_SACK    0x00000002
>> +#define RTAX_FEATURE_NO_SACK    0x00000002
>>  #define RTAX_FEATURE_TIMESTAMP    0x00000004
>>  #define RTAX_FEATURE_ALLFRAG    0x00000008
>>  
> I just realized that unlike NO_DSACK, this change assumes removing the
> sysctl and defaulting on.
Once again, no it does not do no such thing. The sysctl and semantics 
stays here just the same.

The RTAX_FEATURE_SACK is not sued AFAIK in current code at all.

Gilad

Patch

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index adf2068..9c802a6 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -377,7 +377,7 @@  enum
 #define RTAX_MAX (__RTAX_MAX - 1)
 
 #define RTAX_FEATURE_ECN	0x00000001
-#define RTAX_FEATURE_SACK	0x00000002
+#define RTAX_FEATURE_NO_SACK	0x00000002
 #define RTAX_FEATURE_TIMESTAMP	0x00000004
 #define RTAX_FEATURE_ALLFRAG	0x00000008
 
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index d502f49..b14f780 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3763,7 +3763,8 @@  void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
 				break;
 			case TCPOPT_SACK_PERM:
 				if (opsize == TCPOLEN_SACK_PERM && th->syn &&
-				    !estab && sysctl_tcp_sack) {
+				    !estab && sysctl_tcp_sack &&
+				    !dst_feature(dst, RTAX_FEATURE_NO_SACK)) {
 					opt_rx->sack_ok = 1;
 					tcp_sack_reset(opt_rx);
 				}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index fcd278a..64db8dd 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -464,6 +464,7 @@  static unsigned tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 				struct tcp_md5sig_key **md5) {
 	struct tcp_sock *tp = tcp_sk(sk);
 	unsigned size = 0;
+	struct dst_entry *dst = __sk_dst_get(sk);
 
 #ifdef CONFIG_TCP_MD5SIG
 	*md5 = tp->af_specific->md5_lookup(sk, sk);
@@ -498,7 +499,8 @@  static unsigned tcp_syn_options(struct sock *sk, struct sk_buff *skb,
 		opts->options |= OPTION_WSCALE;
 		size += TCPOLEN_WSCALE_ALIGNED;
 	}
-	if (likely(sysctl_tcp_sack)) {
+	if (likely(sysctl_tcp_sack &&
+		   !dst_feature(dst, RTAX_FEATURE_NO_SACK))) {
 		opts->options |= OPTION_SACK_ADVERTISE;
 		if (unlikely(!(OPTION_TS & opts->options)))
 			size += TCPOLEN_SACKPERM_ALIGNED;