Re: [PATCH net-next] net/smc: Unbind smc control from tcp control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 24, 2022 at 03:06:50PM +0100, Alexandra Winter wrote:
> 
> 
> On 24.11.22 14:00, Alexandra Winter wrote:
> > 
> > 
> > On 23.11.22 12:25, Tony Lu wrote:
> >> On Wed, Nov 23, 2022 at 12:19:19PM +0100, Jan Karcher wrote:
> >>>
> >>>
> >>> On 23/11/2022 12:13, Tony Lu wrote:
> >>>> On Wed, Nov 23, 2022 at 11:58:30AM +0100, Jan Karcher wrote:
> >>>>> In the past SMC used the values of tcp_{w|r}mem to create the send
> >>>>> buffer and RMB. We now have our own sysctl knobs to tune them without
> >>>>> influencing the TCP default.
> >>>>>
> >>>>> This patch removes the dependency on the TCP control by providing our
> >>>>> own initial values which aim for a low memory footprint.
> >>>>
> >>>> +1, before introducing sysctl knobs of SMC, we were going to get rid of
> >>>> TCP and have SMC own values. Now this does it, So I very much agree with
> >>>> this.
> >>>>
> > Iiuc you are changing the default values in this a patch and your other patch:
> > Default values for real_buf for send and receive:
> > 
> > before 0227f058aa29 ("net/smc: Unbind r/w buffer size from clcsock and make them tunable")
> >     real_buf=net.ipv4.tcp_{w|r}mem[1]/2   send: 8k  recv: 64k 
> Jan, you explained to me: "the minima is 16Kib. This is enforced in smc_compress_bufsize 
> which would move any value <= 16Kib into bucket 0 - which is 16KiB "
> so 					send  16k   recv: 64k
> >     
> > after 0227f058aa29 ("net/smc: Unbind r/w buffer size from clcsock and make them tunable")
> > real_buf=net.ipv4.tcp_{w|r}mem[1]   send: 16k (16*1024) recv: 128k (131072) 
> > 
> > after net/smc: Fix expected buffersizes and sync logic
> > real_buf=net.ipv4.tcp_{w|r}mem[1]   send: 16k (16*1024) recv: 128k (131072) 
> > 
> > after net/smc: Unbind smc control from tcp control
> > real_buf=SMC_*BUF_INIT_SIZE   send: 16k (16384) recv: 64k (65536)
> > 
> > If my understanding is correct, then I nack this. 
> > Defaults should be restored to the values before 0227f058aa29.
> > Otherwise users will notice a change in memory usage that needs to
> > be avoided or announced more explicitely. (and don't change them twice)
> See above, I stand corrected. This does restore the default receive buffer size.
> It should got to net with a Fixes: 0227f058aa29 tag.
> And the descrition should clarify that this restores the default recv buffer size
> (and uncouples from tcp control)
> Maybe the users and the distros would thank you, if you merge them into 1 patch?

AFAIK, updating kernel to the version of upstream will take a long time
for distros and users. Since we have sysctls, maybe we could let
applications and distros use sysctl to set their desired and default
values with documentation. It's a more common way to tweak kernel. 

Cheers,
Tony Lu

> 
> 
> > 
> >>>>>
> >>>>> Signed-off-by: Jan Karcher <jaka@xxxxxxxxxxxxx>
> >>>>> Reviewed-by: Wenjia Zhang <wenjia@xxxxxxxxxxxxx>
> >>>>> ---
> >>>>>   Documentation/networking/smc-sysctl.rst |  4 ++--
> >>>>>   net/smc/smc_core.h                      |  6 ++++--
> >>>>>   net/smc/smc_sysctl.c                    | 10 ++++++----
> >>>>>   3 files changed, 12 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/networking/smc-sysctl.rst b/Documentation/networking/smc-sysctl.rst
> >>>>> index 6d8acdbe9be1..a1c634d3690a 100644
> >>>>> --- a/Documentation/networking/smc-sysctl.rst
> >>>>> +++ b/Documentation/networking/smc-sysctl.rst
> >>>>> @@ -44,7 +44,7 @@ smcr_testlink_time - INTEGER
> >>>>>   wmem - INTEGER
> >>>>>   	Initial size of send buffer used by SMC sockets.
> >>>>> -	The default value inherits from net.ipv4.tcp_wmem[1].
> >>>>> +	The default value aims for a small memory footprint and is set to 16KiB.
> >>>>>   	The minimum value is 16KiB and there is no hard limit for max value, but
> >>>>>   	only allowed 512KiB for SMC-R and 1MiB for SMC-D.
> >>>>> @@ -53,7 +53,7 @@ wmem - INTEGER
> >>>>>   rmem - INTEGER
> >>>>>   	Initial size of receive buffer (RMB) used by SMC sockets.
> >>>>> -	The default value inherits from net.ipv4.tcp_rmem[1].
> >>>>> +	The default value aims for a small memory footprint and is set to 64KiB.
> >>>>>   	The minimum value is 16KiB and there is no hard limit for max value, but
> >>>>>   	only allowed 512KiB for SMC-R and 1MiB for SMC-D.
> >>>>> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
> >>>>> index 285f9bd8e232..67c3937f341d 100644
> >>>>> --- a/net/smc/smc_core.h
> >>>>> +++ b/net/smc/smc_core.h
> >>>>> @@ -206,8 +206,10 @@ struct smc_rtoken {				/* address/key of remote RMB */
> >>>>>   	u32			rkey;
> >>>>>   };
> >>>>> -#define SMC_BUF_MIN_SIZE	16384	/* minimum size of an RMB */
> >>>>> -#define SMC_RMBE_SIZES		16	/* number of distinct RMBE sizes */
> >>>>> +#define SMC_SNDBUF_INIT_SIZE 16384 /* initial size of send buffer */
> >>>>> +#define SMC_RCVBUF_INIT_SIZE 65536 /* initial size of receive buffer */
> >>>>> +#define SMC_BUF_MIN_SIZE	 16384	/* minimum size of an RMB */
> >>>>> +#define SMC_RMBE_SIZES		 16	/* number of distinct RMBE sizes */
> There is one blank added to these lines? Why? They still don't align.
> >>>>>   /* theoretically, the RFC states that largest size would be 512K,
> >>>>>    * i.e. compressed 5 and thus 6 sizes (0..5), despite
> >>>>>    * struct smc_clc_msg_accept_confirm.rmbe_size being a 4 bit value (0..15)
> >>>>> diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
> >>>>> index b6f79fabb9d3..a63aa79d4856 100644
> >>>>> --- a/net/smc/smc_sysctl.c
> >>>>> +++ b/net/smc/smc_sysctl.c
> >>>>> @@ -19,8 +19,10 @@
> >>>>>   #include "smc_llc.h"
> >>>>>   #include "smc_sysctl.h"
> >>>>> -static int min_sndbuf = SMC_BUF_MIN_SIZE;
> >>>>> -static int min_rcvbuf = SMC_BUF_MIN_SIZE;
> >>>>> +static int initial_sndbuf	= SMC_SNDBUF_INIT_SIZE;
> >>>>> +static int initial_rcvbuf	= SMC_RCVBUF_INIT_SIZE;
> >>>>> +static int min_sndbuf		= SMC_BUF_MIN_SIZE;
> >>>>> +static int min_rcvbuf		= SMC_BUF_MIN_SIZE;
> > Broken formatting
> >>>>>   static struct ctl_table smc_table[] = {
> >>>>>   	{
> >>>>> @@ -88,8 +90,8 @@ int __net_init smc_sysctl_net_init(struct net *net)
> >>>>>   	net->smc.sysctl_autocorking_size = SMC_AUTOCORKING_DEFAULT_SIZE;
> >>>>>   	net->smc.sysctl_smcr_buf_type = SMCR_PHYS_CONT_BUFS;
> >>>>>   	net->smc.sysctl_smcr_testlink_time = SMC_LLC_TESTLINK_DEFAULT_TIME;
> >>>>> -	WRITE_ONCE(net->smc.sysctl_wmem, READ_ONCE(net->ipv4.sysctl_tcp_wmem[1]));
> >>>>> -	WRITE_ONCE(net->smc.sysctl_rmem, READ_ONCE(net->ipv4.sysctl_tcp_rmem[1]));
> >>>>> +	WRITE_ONCE(net->smc.sysctl_wmem, initial_sndbuf);
> >>>>> +	WRITE_ONCE(net->smc.sysctl_rmem, initial_rcvbuf);
> >>>>
> >>>> Maybe we can use SMC_{SND|RCV}BUF_INIT_SIZE macro directly, instead of
> >>>> new variables.
> >>>
> >>> The reason i created the new variables is that min_{snd|rcv}buf also have
> >>> their own variables. I know it is not needed but thought it was cleaner.
> >>> If you have a strong opinion on using the value directly i can change it.
> >>> Please let me know if you want it changed.
> >>
> >> Yep, it's okay for me to use variables or macros. Just let it be.
> > I think it's better coding style to use the macros instead of unneccessary variables.
> > At least the variables could be defined as const.
> >>
> >> Reviewed-by: Tony Lu <tonylu@xxxxxxxxxxxxxxxxx>
> >>
> >> Cheers,
> >> Tony Lu
> >>
> >>>
> >>> - Jan
> >>>>
> >>>> Cheers,
> >>>> Tony Lu
> >>>>
> >>>>>   	return 0;
> >>>>> -- 
> >>>>> 2.34.1



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux