RE: [PATCH net-next,v2] tcp: Set pingpong threshold via sysctl

[Haiyang Zhang]

> -----Original Message-----
> From: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> Sent: Tuesday, October 10, 2023 4:12 PM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Cc: corbet@xxxxxxx; davem@xxxxxxxxxxxxx; dsahern@xxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; kuniyu@xxxxxxxxxx; KY
> Srinivasan <kys@xxxxxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; linux-
> hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> mfreemon@xxxxxxxxxxxxxx; morleyd@xxxxxxxxxx; mubashirq@xxxxxxxxxx;
> ncardwell@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; pabeni@xxxxxxxxxx;
> weiwan@xxxxxxxxxx; ycheng@xxxxxxxxxx
> Subject: Re: [PATCH net-next,v2] tcp: Set pingpong threshold via sysctl
> 
> [You don't often get email from kuniyu@xxxxxxxxxx. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Date: Tue, 10 Oct 2023 12:23:30 -0700
> > TCP pingpong threshold is 1 by default. But some applications, like SQL DB
> > may prefer a higher pingpong threshold to activate delayed acks in quick
> > ack mode for better performance.
> >
> > The pingpong threshold and related code were changed to 3 in the year
> > 2019 in:
> >   commit 4a41f453bedf ("tcp: change pingpong threshold to 3")
> > And reverted to 1 in the year 2022 in:
> >   commit 4d8f24eeedc5 ("Revert "tcp: change pingpong threshold to 3"")
> >
> > There is no single value that fits all applications.
> > Add net.ipv4.tcp_pingpong_thresh sysctl tunable, so it can be tuned for
> > optimal performance based on the application needs.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > ---
> > v2: Make it per-namesapce setting, and other updates suggested by Neal
> Cardwell,
> > and Kuniyuki Iwashima.
> >
> > ---
> >  Documentation/networking/ip-sysctl.rst |  8 ++++++++
> >  include/net/inet_connection_sock.h     | 16 ++++++++++++----
> >  include/net/netns/ipv4.h               |  1 +
> >  net/ipv4/sysctl_net_ipv4.c             |  8 ++++++++
> >  net/ipv4/tcp_ipv4.c                    |  2 ++
> >  net/ipv4/tcp_output.c                  |  4 ++--
> >  6 files changed, 33 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/networking/ip-sysctl.rst
> b/Documentation/networking/ip-sysctl.rst
> > index 5bfa1837968c..c0308b65dc2f 100644
> > --- a/Documentation/networking/ip-sysctl.rst
> > +++ b/Documentation/networking/ip-sysctl.rst
> > @@ -1183,6 +1183,14 @@ tcp_plb_cong_thresh - INTEGER
> >
> >       Default: 128
> >
> > +tcp_pingpong_thresh - INTEGER
> > +     TCP pingpong threshold is 1 by default, but some application may need a
> > +     higher threshold for optimal performance.
> > +
> > +     Possible Values: 1 - 255
> > +
> > +     Default: 1
> > +
> >  UDP variables
> >  =============
> >
> > diff --git a/include/net/inet_connection_sock.h
> b/include/net/inet_connection_sock.h
> > index 5d2fcc137b88..0182f27bce40 100644
> > --- a/include/net/inet_connection_sock.h
> > +++ b/include/net/inet_connection_sock.h
> > @@ -325,11 +325,10 @@ void inet_csk_update_fastreuse(struct
> inet_bind_bucket *tb,
> >
> >  struct dst_entry *inet_csk_update_pmtu(struct sock *sk, u32 mtu);
> >
> > -#define TCP_PINGPONG_THRESH  1
> > -
> >  static inline void inet_csk_enter_pingpong_mode(struct sock *sk)
> >  {
> > -     inet_csk(sk)->icsk_ack.pingpong = TCP_PINGPONG_THRESH;
> > +     inet_csk(sk)->icsk_ack.pingpong =
> > +             READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_pingpong_thresh);
> >  }
> >
> >  static inline void inet_csk_exit_pingpong_mode(struct sock *sk)
> > @@ -339,7 +338,16 @@ static inline void
> inet_csk_exit_pingpong_mode(struct sock *sk)
> >
> >  static inline bool inet_csk_in_pingpong_mode(struct sock *sk)
> >  {
> > -     return inet_csk(sk)->icsk_ack.pingpong >= TCP_PINGPONG_THRESH;
> > +     return inet_csk(sk)->icsk_ack.pingpong >=
> > +            READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_pingpong_thresh);
> > +}
> > +
> > +static inline void inet_csk_inc_pingpong_cnt(struct sock *sk)
> > +{
> > +     struct inet_connection_sock *icsk = inet_csk(sk);
> > +
> > +     if (icsk->icsk_ack.pingpong < U8_MAX)
> > +             icsk->icsk_ack.pingpong++;
> >  }
> >
> >  static inline bool inet_csk_has_ulp(const struct sock *sk)
> > diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
> > index d96d05b08819..9f1b3eb9473e 100644
> > --- a/include/net/netns/ipv4.h
> > +++ b/include/net/netns/ipv4.h
> > @@ -191,6 +191,7 @@ struct netns_ipv4 {
> >       u8 sysctl_tcp_plb_rehash_rounds;
> >       u8 sysctl_tcp_plb_suspend_rto_sec;
> >       int sysctl_tcp_plb_cong_thresh;
> > +     u8 sysctl_tcp_pingpong_thresh;
> >
> >       int sysctl_udp_wmem_min;
> >       int sysctl_udp_rmem_min;
> 
> Maybe a hole after sysctl_tcp_backlog_ack_defer is a good place
> to put a new TCP knob.
> 
> After sysctl_tcp_plb_cong_thresh, we can fill 1-byte hole but the
> cacheline seems cold for TCP.
> 
> $ pahole -C netns_ipv4 vmlinux
> struct netns_ipv4 {
> ...
>         u8                         sysctl_tcp_backlog_ack_defer; /*   402     1 */
> 
>         /* XXX 1 byte hole, try to pack */
> 
>         int                        sysctl_tcp_reordering; /*   404     4 */
> ...
>         int                        sysctl_tcp_plb_cong_thresh; /*   572     4 */
>         /* --- cacheline 9 boundary (576 bytes) --- */
>         int                        sysctl_udp_wmem_min;  /*   576     4 */
>         int                        sysctl_udp_rmem_min;  /*   580     4 */
>         u8                         sysctl_fib_notify_on_flag_change; /*   584     1 */
>         u8                         sysctl_tcp_syn_linear_timeouts; /*   585     1 */
>         u8                         sysctl_igmp_llm_reports; /*   586     1 */
> 
>         /* XXX 1 byte hole, try to pack */
> ...
> 

Will do.

Thanks,
- Haiyang