From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Subject: Re: [PATCH net-next 1/5] tcp: fix SO_RCVLOWAT and RCVBUF autotuning
Date: Thu, 19 Apr 2018 23:02:21 -0300
Message-ID: <20180420020221.GC3710@localhost.localdomain>
References: <20180416173339.6310-1-edumazet@google.com>
 <20180416173339.6310-2-edumazet@google.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: "David S . Miller" <davem@davemloft.net>,
        netdev <netdev@vger.kernel.org>,
        Neal Cardwell <ncardwell@google.com>,
        Yuchung Cheng <ycheng@google.com>,
        Soheil Hassas Yeganeh <soheil@google.com>,
        Eric Dumazet <eric.dumazet@gmail.com>
To: Eric Dumazet <edumazet@google.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-qk0-f175.google.com ([209.85.220.175]:45433 "EHLO
        mail-qk0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753784AbeDTCC0 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 19 Apr 2018 22:02:26 -0400
Received: by mail-qk0-f175.google.com with SMTP id c136so7439167qkb.12
        for <netdev@vger.kernel.org>; Thu, 19 Apr 2018 19:02:26 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20180416173339.6310-2-edumazet@google.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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);
> +
...