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

Jan Karcher
2022-11-23T11:19:36Z

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.
>
>>
>> Signed-off-by: Jan Karcher <jaka@linux.ibm.com>
>> Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
>> ---
>> 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 */
>> /* 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;
>>
>> 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.

- Jan
>
> Cheers,
> Tony Lu
>
>>
>> return 0;
>>
>> --
>> 2.34.1