Re: [PATCH mptcp-net v2] mptcp: fix the default value of scaling_ratio

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

(+cc Paolo who wrote the commit mentioned by the "Fixes" tag)

On 07/05/2024 05:46, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> BPF tests fail sometimes with "bytes != total_bytes" errors:
> 
>  test_default:PASS:sched_init:default 0 nsec
>  send_data:PASS:pthread_create 0 nsec
>  send_data:FAIL:recv 936000 != 10485760 nr_recv:-1 errno:11
>  default: 3041 ms
>  server:FAIL:send 7579500 != 10485760 nr_sent:-1 errno:11
>  send_data:FAIL:pthread_join thread_ret:-11 test_default:PASS: \
> 				has_bytes_sent addr_1 0 nsec
>  test_default:PASS:has_bytes_sent addr_2 0 nsec
>  close_netns:PASS:setns 0 nsec
> 
> In this case mptcp_recvmsg() gets EAGAIN errors. This issue introduces
> by commit b8dc6d6ce931 ("mptcp: fix rcv buffer auto-tuning"). The default
> value of scaling_ratio should be TCP_DEFAULT_SCALING_RATIO, not U8_MAX.

It looks like you are modifying the max value, not the default one.

Also, please always explain the reason: why should it be X, not Y?

> 
> Fixes: b8dc6d6ce931 ("mptcp: fix rcv buffer auto-tuning")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/487
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> v2:
>  - I finally found the root cause of this issue.
>  - cc Martin too.
> ---
>  net/mptcp/protocol.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 579031c60937..d00cd21e8d3f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1981,9 +1981,9 @@ static int __mptcp_recvmsg_mskq(struct mptcp_sock *msk,
>   */
>  static void mptcp_rcv_space_adjust(struct mptcp_sock *msk, int copied)
>  {
> +	u8 scaling_ratio = TCP_DEFAULT_SCALING_RATIO;

With this modification, you are limiting msk->scaling_ratio to
TCP_DEFAULT_SCALING_RATIO. That's probably not what you want. See below:

  (...)
  mptcp_for_each_subflow(msk, subflow)
      (...)
      scaling_ratio = min(tp->scaling_ratio, scaling_ratio);

  (...)
  msk->scaling_ratio = scaling_ratio;


Do you mean that in your case, msk->scaling_ratio was assigned to
U8_MAX? Or it was assigned to a too high value (>TCP_DEFAULT_SCALING_RATIO)?

Please add such info in the commit message as well.

>  	struct mptcp_subflow_context *subflow;
>  	struct sock *sk = (struct sock *)msk;
> -	u8 scaling_ratio = U8_MAX;
>  	u32 time, advmss = 1;
>  	u64 rtt_us, mstamp;
>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.