Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <603d2672855d74e9bfc2619156f4ffe7976de4f5.camel@linux.ibm.com>
Subject: Re: [PATCH net 2/2] net/smc: Use correct buffer sizes when
 switching between TCP and SMC
From:   Gerd Bayer <gbayer@linux.ibm.com>
To:     Tony Lu <tonylu@linux.alibaba.com>
Cc:     Wenjia Zhang <wenjia@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Karsten Graul <kgraul@linux.ibm.com>,
        "D . Wythe" <alibuda@linux.alibaba.com>,
        Wen Gu <guwen@linux.alibaba.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>, linux-s390@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Wed, 02 Aug 2023 18:47:58 +0200
In-Reply-To: <ZMpPjAaRzSRy-Vo_@TONYMAC-ALIBABA.local>
References: <20230802093313.1501605-1-gbayer@linux.ibm.com>
         <20230802093313.1501605-3-gbayer@linux.ibm.com>
         <ZMpPjAaRzSRy-Vo_@TONYMAC-ALIBABA.local>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.module_f38+17164+63eeee4a) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-08-02 at 20:43 +0800, Tony Lu wrote:
> On Wed, Aug 02, 2023 at 11:33:13AM +0200, Gerd Bayer wrote:
> > Tuning of the effective buffer size through setsockopts was working
> > for
> > SMC traffic only but not for TCP fall-back connections even before
> > commit 0227f058aa29 ("net/smc: Unbind r/w buffer size from clcsock
> > and
> > make them tunable"). That change made it apparent that TCP fall-
> > back
> > connections would use net.smc.[rw]mem as buffer size instead of
> > net.ipv4_tcp_[rw]mem.
> >=20
> > Amend the code that copies attributes between the (TCP) clcsock and
> > the
> > SMC socket and adjust buffer sizes appropriately:
> > - Copy over sk_userlocks so that both sockets agree on whether
> > tuning
> > =C2=A0 via setsockopt is active.
> > - When falling back to TCP use sk_sndbuf or sk_rcvbuf as specified
> > with
> > =C2=A0 setsockopt. Otherwise, use the sysctl value for TCP/IPv4.
> > - Likewise, use either values from setsockopt or from sysctl for
> > SMC
> > =C2=A0 (duplicated) on successful SMC connect.
> >=20
> > In smc_tcp_listen_work() drop the explicit copy of buffer sizes as
> > that
> > is taken care of by the attribute copy.
> >=20
> > Fixes: 0227f058aa29 ("net/smc: Unbind r/w buffer size from clcsock
> > and make them tunable")
> > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> > Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> > Reviewed-by: Jan Karcher <jaka@linux.ibm.com>
>=20
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
>=20
> >=20
> ^^^^ nit: a extra new line here.
I'll clean that up.

> > ---
> > =C2=A0net/smc/af_smc.c | 76 ++++++++++++++++++++++++++++++++++---------=
-
> > ----
> > =C2=A01 file changed, 54 insertions(+), 22 deletions(-)
> >=20
> >=20
[...]

> > +/* if set, use value set by setsockopt() - else use IPv4 or SMC
> > sysctl value */
> > +static void smc_adjust_sock_bufsizes(struct sock *nsk, struct sock
> > *osk,
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned=
 long mask)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct net *nnet;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0nnet =3D nsk->sk_net.net;
>=20
> Better to combine these two lines with existed helper.
>=20
> struct net *net =3D sock_net(nsk);
Yes, looks much cleaner.

[...]
>=20

Thank you Tony for your review and comments.
I'll be sending out a v2 with your recommendations - but give people a
little more time to look at this version.

Thanks,
Gerd