[net-next,1/5] tcp: fix SO_RCVLOWAT and RCVBUF autotuning

Message ID 20180416173339.6310-2-edumazet@google.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • tcp: add zero copy receive
Related show

Commit Message

Eric Dumazet April 16, 2018, 5:33 p.m.
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(-)

Comments

Marcelo Ricardo Leitner April 20, 2018, 2:02 a.m. | #1
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);
> +
...
Eric Dumazet April 20, 2018, 2:36 a.m. | #2
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.
Marcelo Ricardo Leitner April 20, 2018, 3:04 a.m. | #3
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.

Patch

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 = {