| Message ID | 1324351929-2588-1-git-send-email-subramanian.vijay@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 19 December 2011 19:32, Vijay Subramanian <subramanian.vijay@gmail.com> wrote: > to record the state of SACK/FACK and DSACK for better readability and maintenance. > > Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com> > --- > include/linux/tcp.h | 5 +++++ > include/net/tcp.h | 4 ++-- > net/ipv4/syncookies.c | 2 +- > net/ipv4/tcp_input.c | 6 +++--- > 4 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h > index 7f59ee9..874b043 100644 > --- a/include/linux/tcp.h > +++ b/include/linux/tcp.h > @@ -238,6 +238,11 @@ struct tcp_sack_block { > u32 end_seq; > }; > > +/*These are used to set the sack_ok field in struct tcp_options_received */ > +#define SACK_SEEN (1 << 0) /*1 = peer is SACK capable, */ > +#define FACK_ENABLED (1 << 1) /*1 = FACK is enabled locally*/ > +#define DSACK_SEEN (1 << 2) /*1 = DSACK was received from peer*/ > + > struct tcp_options_received { > /* PAWS/RTTM data */ > long ts_recent_stamp;/* Time we stored ts_recent (for aging) */ > diff --git a/include/net/tcp.h b/include/net/tcp.h > index a4f52e1..cbb943e 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -773,12 +773,12 @@ static inline int tcp_is_reno(const struct tcp_sock *tp) > > static inline int tcp_is_fack(const struct tcp_sock *tp) > { > - return tp->rx_opt.sack_ok & 2; > + return tp->rx_opt.sack_ok & FACK_ENABLED; > } > > static inline void tcp_enable_fack(struct tcp_sock *tp) > { > - tp->rx_opt.sack_ok |= 2; > + tp->rx_opt.sack_ok |= FACK_ENABLED; > } > > static inline unsigned int tcp_left_out(const struct tcp_sock *tp) > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c > index 90f6544..dfba6a5 100644 > --- a/net/ipv4/syncookies.c > +++ b/net/ipv4/syncookies.c > @@ -245,7 +245,7 @@ bool cookie_check_timestamp(struct tcp_options_received *tcp_opt, bool *ecn_ok) > if (!sysctl_tcp_timestamps) > return false; > > - tcp_opt->sack_ok = (options >> 4) & 0x1; > + tcp_opt->sack_ok = (options >> 4) & SACK_SEEN; > *ecn_ok = (options >> 5) & 1; > if (*ecn_ok && !sysctl_tcp_ecn) > return false; > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index f131d92..5cbc788 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -865,13 +865,13 @@ static void tcp_disable_fack(struct tcp_sock *tp) > /* RFC3517 uses different metric in lost marker => reset on change */ > if (tcp_is_fack(tp)) > tp->lost_skb_hint = NULL; > - tp->rx_opt.sack_ok &= ~2; > + tp->rx_opt.sack_ok &= ~FACK_ENABLED; > } > > /* Take a notice that peer is sending D-SACKs */ > static void tcp_dsack_seen(struct tcp_sock *tp) > { > - tp->rx_opt.sack_ok |= 4; > + tp->rx_opt.sack_ok |= DSACK_SEEN; > } > > /* Initialize metrics on socket. */ > @@ -3878,7 +3878,7 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o > case TCPOPT_SACK_PERM: > if (opsize == TCPOLEN_SACK_PERM && th->syn && > !estab && sysctl_tcp_sack) { > - opt_rx->sack_ok = 1; > + opt_rx->sack_ok = SACK_SEEN; > tcp_sack_reset(opt_rx); > } > break; > -- > 1.7.0.4 > This patch was created against net-next tree and not -net tree. Apologies for the error. Should I resend it? Regards, Vijay -- 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/include/linux/tcp.h b/include/linux/tcp.h > index 7f59ee9..874b043 100644 > --- a/include/linux/tcp.h > +++ b/include/linux/tcp.h > @@ -238,6 +238,11 @@ struct tcp_sack_block { > u32 end_seq; > }; > > +/*These are used to set the sack_ok field in struct > tcp_options_received */ > +#define SACK_SEEN (1 << 0) /*1 = peer is SACK capable, */ > +#define FACK_ENABLED (1 << 1) /*1 = FACK is enabled locally*/ > +#define DSACK_SEEN (1 << 2) /*1 = DSACK was received from peer*/ > + Since that is a fairly public header, some namespace protection might be sensible. > - tcp_opt->sack_ok = (options >> 4) & 0x1; > + tcp_opt->sack_ok = (options >> 4) & SACK_SEEN; Looks to me like that 0x1 isn't SAC_SEEK! So this is now misleading. David -- 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
Thanks a lot for reviewing the patch. Please see responses below. On 20 December 2011 02:27, David Laight <David.Laight@aculab.com> wrote: > >> +/*These are used to set the sack_ok field in struct >> tcp_options_received */ >> +#define SACK_SEEN (1 << 0) /*1 = peer is SACK capable, */ >> +#define FACK_ENABLED (1 << 1) /*1 = FACK is enabled locally*/ >> +#define DSACK_SEEN (1 << 2) /*1 = DSACK was received from peer*/ >> + > > Since that is a fairly public header, some namespace protection > might be sensible. I assume you are talking about collision with userspace. These definitions are already protected by #ifdef __KERNEL__. That should be enough to protect against collisions with userspace I think. > >> - tcp_opt->sack_ok = (options >> 4) & 0x1; >> + tcp_opt->sack_ok = (options >> 4) & SACK_SEEN; > > Looks to me like that 0x1 isn't SAC_SEEK! So this is now misleading. Can you please elaborate? Are you saying this is wrong or that it could be done better? Thanks for your time! Vijay -- 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
From: Vijay Subramanian <subramanian.vijay@gmail.com> Date: Tue, 20 Dec 2011 12:02:52 -0800 > I assume you are talking about collision with userspace. These > definitions are already protected by #ifdef __KERNEL__. > That should be enough to protect against collisions with userspace I think. Within the kernel he means, please put a TCP_* prefix onto these values. >> >>> - tcp_opt->sack_ok = (options >> 4) & 0x1; >>> + tcp_opt->sack_ok = (options >> 4) & SACK_SEEN; >> >> Looks to me like that 0x1 isn't SAC_SEEK! So this is now misleading. > > Can you please elaborate? Are you saying this is wrong or that it > could be done better? I think he's saying it could be done better. Probably something like: tcp_opt->sack_ok = (options & (1 << 4)) ? SACK_SEEN : 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
> > I assume you are talking about collision with userspace. These > > definitions are already protected by #ifdef __KERNEL__. > > That should be enough to protect against collisions with > userspace I think. > > Within the kernel he means, please put a TCP_* prefix onto these > values. Given the values are currently: > > +#define SACK_SEEN (1 << 0) /*1 = peer is SACK capable, */ > > +#define FACK_ENABLED (1 << 1) /*1 = FACK is enabled locally*/ > > +#define DSACK_SEEN (1 << 2) /*1 = DSACK was received from peer*/ But the structure field member is 'sack_ok', unless it generates very silly long lines - which in this case it probably doesn't - it might even be worth prefixing with TCP_SACK_OK_ although that might be deemed excessive! David -- 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
> > Given the values are currently: > >> > +#define SACK_SEEN (1 << 0) /*1 = peer is SACK capable, */ >> > +#define FACK_ENABLED (1 << 1) /*1 = FACK is enabled locally*/ >> > +#define DSACK_SEEN (1 << 2) /*1 = DSACK was received from > peer*/ > > But the structure field member is 'sack_ok', unless it generates > very silly long lines - which in this case it probably doesn't - > it might even be worth prefixing with TCP_SACK_OK_ although > that might be deemed excessive! > Thanks for all the feedback. I will follow these suggestions in future. Vijay -- 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/include/linux/tcp.h b/include/linux/tcp.h index 7f59ee9..874b043 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -238,6 +238,11 @@ struct tcp_sack_block { u32 end_seq; }; +/*These are used to set the sack_ok field in struct tcp_options_received */ +#define SACK_SEEN (1 << 0) /*1 = peer is SACK capable, */ +#define FACK_ENABLED (1 << 1) /*1 = FACK is enabled locally*/ +#define DSACK_SEEN (1 << 2) /*1 = DSACK was received from peer*/ + struct tcp_options_received { /* PAWS/RTTM data */ long ts_recent_stamp;/* Time we stored ts_recent (for aging) */ diff --git a/include/net/tcp.h b/include/net/tcp.h index a4f52e1..cbb943e 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -773,12 +773,12 @@ static inline int tcp_is_reno(const struct tcp_sock *tp) static inline int tcp_is_fack(const struct tcp_sock *tp) { - return tp->rx_opt.sack_ok & 2; + return tp->rx_opt.sack_ok & FACK_ENABLED; } static inline void tcp_enable_fack(struct tcp_sock *tp) { - tp->rx_opt.sack_ok |= 2; + tp->rx_opt.sack_ok |= FACK_ENABLED; } static inline unsigned int tcp_left_out(const struct tcp_sock *tp) diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index 90f6544..dfba6a5 100644 --- a/net/ipv4/syncookies.c +++ b/net/ipv4/syncookies.c @@ -245,7 +245,7 @@ bool cookie_check_timestamp(struct tcp_options_received *tcp_opt, bool *ecn_ok) if (!sysctl_tcp_timestamps) return false; - tcp_opt->sack_ok = (options >> 4) & 0x1; + tcp_opt->sack_ok = (options >> 4) & SACK_SEEN; *ecn_ok = (options >> 5) & 1; if (*ecn_ok && !sysctl_tcp_ecn) return false; diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index f131d92..5cbc788 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -865,13 +865,13 @@ static void tcp_disable_fack(struct tcp_sock *tp) /* RFC3517 uses different metric in lost marker => reset on change */ if (tcp_is_fack(tp)) tp->lost_skb_hint = NULL; - tp->rx_opt.sack_ok &= ~2; + tp->rx_opt.sack_ok &= ~FACK_ENABLED; } /* Take a notice that peer is sending D-SACKs */ static void tcp_dsack_seen(struct tcp_sock *tp) { - tp->rx_opt.sack_ok |= 4; + tp->rx_opt.sack_ok |= DSACK_SEEN; } /* Initialize metrics on socket. */ @@ -3878,7 +3878,7 @@ void tcp_parse_options(const struct sk_buff *skb, struct tcp_options_received *o case TCPOPT_SACK_PERM: if (opsize == TCPOLEN_SACK_PERM && th->syn && !estab && sysctl_tcp_sack) { - opt_rx->sack_ok = 1; + opt_rx->sack_ok = SACK_SEEN; tcp_sack_reset(opt_rx); } break;
to record the state of SACK/FACK and DSACK for better readability and maintenance. Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com> --- include/linux/tcp.h | 5 +++++ include/net/tcp.h | 4 ++-- net/ipv4/syncookies.c | 2 +- net/ipv4/tcp_input.c | 6 +++--- 4 files changed, 11 insertions(+), 6 deletions(-)