Re: [PATCH net 2/2] net/smc: Use correct buffer sizes when switching between TCP and SMC

[Gerd Bayer <gbayer@linux.ibm.com>]

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.
> > 
> > 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
> >   via setsockopt is active.
> > - When falling back to TCP use sk_sndbuf or sk_rcvbuf as specified
> > with
> >   setsockopt. Otherwise, use the sysctl value for TCP/IPv4.
> > - Likewise, use either values from setsockopt or from sysctl for
> > SMC
> >   (duplicated) on successful SMC connect.
> > 
> > In smc_tcp_listen_work() drop the explicit copy of buffer sizes as
> > that
> > is taken care of by the attribute copy.
> > 
> > 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>
> 
> Reviewed-by: Tony Lu <tonylu@linux.alibaba.com>
> 
> > 
> ^^^^ nit: a extra new line here.
I'll clean that up.

> > ---
> >  net/smc/af_smc.c | 76 ++++++++++++++++++++++++++++++++++----------
> > ----
> >  1 file changed, 54 insertions(+), 22 deletions(-)
> > 
> > 
[...]

> > +/* 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,
> > +                                    unsigned long mask)
> > +{
> > +       struct net *nnet;
> > +
> > +       nnet = nsk->sk_net.net;
> 
> Better to combine these two lines with existed helper.
> 
> struct net *net = sock_net(nsk);
Yes, looks much cleaner.

[...]
> 

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