Message ID | 20180416173339.6310-2-edumazet@google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | tcp: add zero copy receive | expand |
On Mon, Apr 16, 2018 at 10:33:35AM -0700, Eric Dumazet wrote: > Applications might use SO_RCVLOWAT on TCP socket hoping to receive > one [E]POLLIN event only when a given amount of bytes are ready in socket > receive queue. > > Problem is that receive autotuning is not aware of this constraint, > meaning sk_rcvbuf might be too small to allow all bytes to be stored. > > Add a new (struct proto_ops)->set_rcvlowat method so that a protocol > can override the default setsockopt(SO_RCVLOWAT) behavior. > ... > +/* Make sure sk_rcvbuf is big enough to satisfy SO_RCVLOWAT hint */ > +int tcp_set_rcvlowat(struct sock *sk, int val) > +{ > + sk->sk_rcvlowat = val ? : 1; > + if (sk->sk_userlocks & SOCK_RCVBUF_LOCK) > + return 0; > + > + /* val comes from user space and might be close to INT_MAX */ > + val <<= 1; > + if (val < 0) > + val = INT_MAX; > + > + val = min(val, sock_net(sk)->ipv4.sysctl_tcp_rmem[2]); Hi Eric, As val may be changed to a smaller value by the line above, shouldn't it assign sk->sk_rcvlowat again? Otherwise it may still be bigger than sk_rcvbuf. Say val = 512k, sysctl_tcp_rmem[2] = 256k val <<= 1 , val = 1M val = min() , val = 256k val > sk_rcvbuf sk_rcvbuf = 256k , at most, which is smaller than sk_rcvlowat Without reassigning the application has to check how big is tcp_rmem[2] and be sure to not go above /2 of it to not trip on this again. Or, as you have added a return value here, it could return -EINVAL in such cases. Probably better, as then the application will not get a smaller buffer than wanted later. > + if (val > sk->sk_rcvbuf) { > + sk->sk_rcvbuf = val; > + tcp_sk(sk)->window_clamp = tcp_win_from_space(sk, val); > + } > + return 0; > +} > +EXPORT_SYMBOL(tcp_set_rcvlowat); > + ...
On Thu, Apr 19, 2018 at 7:02 PM Marcelo Ricardo Leitner < marcelo.leitner@gmail.com> wrote: > Hi Eric, > As val may be changed to a smaller value by the line above, shouldn't > it assign sk->sk_rcvlowat again? Otherwise it may still be bigger > than sk_rcvbuf. > Say val = 512k, sysctl_tcp_rmem[2] = 256k > val <<= 1 , val = 1M > val = min() , val = 256k > val > sk_rcvbuf > sk_rcvbuf = 256k , at most, which is smaller than sk_rcvlowat > Without reassigning the application has to check how big is > tcp_rmem[2] and be sure to not go above /2 of it to not trip on this > again. I am not sure about that : Reporting an error might break existing applications that were not expecting setsockopt() to return an error, even if the value was 'probably too big to be okay' > Or, as you have added a return value here, it could return -EINVAL in > such cases. Probably better, as then the application will not get a > smaller buffer than wanted later. Note that maybe some applications might first set SO_RCVLOWAT, then SO_RCVBUF, we do not want to break them. My patch really covers the case were autotuning should immediately grow the sk_rcvbuf for reasonable SO_RCVLOWAT values.
On Fri, Apr 20, 2018 at 02:36:52AM +0000, Eric Dumazet wrote: > On Thu, Apr 19, 2018 at 7:02 PM Marcelo Ricardo Leitner < > marcelo.leitner@gmail.com> wrote: > > > Hi Eric, > > > As val may be changed to a smaller value by the line above, shouldn't > > it assign sk->sk_rcvlowat again? Otherwise it may still be bigger > > than sk_rcvbuf. > > > Say val = 512k, sysctl_tcp_rmem[2] = 256k > > val <<= 1 , val = 1M > > val = min() , val = 256k > > val > sk_rcvbuf > > sk_rcvbuf = 256k , at most, which is smaller than sk_rcvlowat > > > Without reassigning the application has to check how big is > > tcp_rmem[2] and be sure to not go above /2 of it to not trip on this > > again. > > I am not sure about that : > > Reporting an error might break existing applications that were not > expecting setsockopt() > to return an error, even if the value was 'probably too big to be okay' I would argue that they are already broken but... > > > > Or, as you have added a return value here, it could return -EINVAL in > > such cases. Probably better, as then the application will not get a > > smaller buffer than wanted later. > > Note that maybe some applications might first set SO_RCVLOWAT, then > SO_RCVBUF, > we do not want to break them. ... yeah.. if they do it this way, they work today. Good point. > > > My patch really covers the case were autotuning should immediately grow the > sk_rcvbuf > for reasonable SO_RCVLOWAT values. That's not exactly what the comment above the function says, thus why my comments. Thanks.
diff --git a/include/linux/net.h b/include/linux/net.h index 2248a052061d8aeb0ae08d233f181f09cba6384b..6554d3ba4396b3df49acac934ad16eeb71a695f4 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -197,6 +197,7 @@ struct proto_ops { int offset, size_t size, int flags); int (*sendmsg_locked)(struct sock *sk, struct msghdr *msg, size_t size); + int (*set_rcvlowat)(struct sock *sk, int val); }; #define DECLARE_SOCKADDR(type, dst, src) \ diff --git a/include/net/tcp.h b/include/net/tcp.h index 9c9b3768b350abfd51776563d220d5e97ca9da69..b2318242cad89176d3c2c027affd4db3c2549ff4 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -402,6 +402,7 @@ void tcp_set_keepalive(struct sock *sk, int val); void tcp_syn_ack_timeout(const struct request_sock *req); int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock, int flags, int *addr_len); +int tcp_set_rcvlowat(struct sock *sk, int val); void tcp_parse_options(const struct net *net, const struct sk_buff *skb, struct tcp_options_received *opt_rx, int estab, struct tcp_fastopen_cookie *foc); diff --git a/net/core/sock.c b/net/core/sock.c index 6444525f610cf8039516744ad26aec58485b9b8a..b2c3db169ca1892c4d624fc5e30af12f4eed0adb 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -905,7 +905,10 @@ int sock_setsockopt(struct socket *sock, int level, int optname, case SO_RCVLOWAT: if (val < 0) val = INT_MAX; - sk->sk_rcvlowat = val ? : 1; + if (sock->ops->set_rcvlowat) + ret = sock->ops->set_rcvlowat(sk, val); + else + sk->sk_rcvlowat = val ? : 1; break; case SO_RCVTIMEO: diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index eaed0367e669aec7635b3cc41de4ece63bb018ec..f5c562aaef3522519bcf1ae37782a7e14e278723 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1006,6 +1006,7 @@ const struct proto_ops inet_stream_ops = { .compat_getsockopt = compat_sock_common_getsockopt, .compat_ioctl = inet_compat_ioctl, #endif + .set_rcvlowat = tcp_set_rcvlowat, }; EXPORT_SYMBOL(inet_stream_ops); diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index bccc4c2700870b8c7ff592a6bd27acebd9bc6471..0abd8d1d3d1d4f0bd6e2762c8a2b862ecf31e4ae 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -1701,6 +1701,27 @@ int tcp_peek_len(struct socket *sock) } EXPORT_SYMBOL(tcp_peek_len); +/* Make sure sk_rcvbuf is big enough to satisfy SO_RCVLOWAT hint */ +int tcp_set_rcvlowat(struct sock *sk, int val) +{ + sk->sk_rcvlowat = val ? : 1; + if (sk->sk_userlocks & SOCK_RCVBUF_LOCK) + return 0; + + /* val comes from user space and might be close to INT_MAX */ + val <<= 1; + if (val < 0) + val = INT_MAX; + + val = min(val, sock_net(sk)->ipv4.sysctl_tcp_rmem[2]); + if (val > sk->sk_rcvbuf) { + sk->sk_rcvbuf = val; + tcp_sk(sk)->window_clamp = tcp_win_from_space(sk, val); + } + return 0; +} +EXPORT_SYMBOL(tcp_set_rcvlowat); + static void tcp_update_recv_tstamps(struct sk_buff *skb, struct scm_timestamping *tss) { diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 8da0b513f1882b39be4fa72a8233d702ae9ec53b..e70d59fb26e16ace1eb484d23964946092a2cd57 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -590,6 +590,7 @@ const struct proto_ops inet6_stream_ops = { .compat_setsockopt = compat_sock_common_setsockopt, .compat_getsockopt = compat_sock_common_getsockopt, #endif + .set_rcvlowat = tcp_set_rcvlowat, }; const struct proto_ops inet6_dgram_ops = {
Applications might use SO_RCVLOWAT on TCP socket hoping to receive one [E]POLLIN event only when a given amount of bytes are ready in socket receive queue. Problem is that receive autotuning is not aware of this constraint, meaning sk_rcvbuf might be too small to allow all bytes to be stored. Add a new (struct proto_ops)->set_rcvlowat method so that a protocol can override the default setsockopt(SO_RCVLOWAT) behavior. Signed-off-by: Eric Dumazet <edumazet@google.com> --- include/linux/net.h | 1 + include/net/tcp.h | 1 + net/core/sock.c | 5 ++++- net/ipv4/af_inet.c | 1 + net/ipv4/tcp.c | 21 +++++++++++++++++++++ net/ipv6/af_inet6.c | 1 + 6 files changed, 29 insertions(+), 1 deletion(-)