[PATCH net-next] net/smc: add sysctl for smc_limit_hs

[PATCH net-next] net/smc: add sysctl for smc_limit_hs

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake workqueue congested"),
we introduce a mechanism to put constraint on SMC connections visit according to
the pressure of SMC handshake process.

At that time, we believed that controlling the feature through netlink was sufficient,
However, most people have realized now that netlink is not convenient in
container scenarios, and sysctl is a more suitable approach.

In addition, it is not reasonable for us to initialize limit_smc_hs in
smc_pnet_net_init, we made a mistable before. It should be initialized
in smc_sysctl_net_init(), just like other systcl.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/smc_pnet.c   |  3 ---
 net/smc/smc_sysctl.c | 11 +++++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 2adb92b..1dd3623 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -887,9 +887,6 @@ int smc_pnet_net_init(struct net *net)
 
 	smc_pnet_create_pnetids_list(net);
 
-	/* disable handshake limitation by default */
-	net->smc.limit_smc_hs = 0;
-
 	return 0;
 }
 
diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
index 13f2bc0..2fab645 100644
--- a/net/smc/smc_sysctl.c
+++ b/net/smc/smc_sysctl.c
@@ -90,6 +90,15 @@
 		.extra1		= &conns_per_lgr_min,
 		.extra2		= &conns_per_lgr_max,
 	},
+	{
+		.procname	= "limit_smc_hs",
+		.data		= &init_net.smc.limit_smc_hs,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
 };
 
 int __net_init smc_sysctl_net_init(struct net *net)
@@ -121,6 +130,8 @@ int __net_init smc_sysctl_net_init(struct net *net)
 	WRITE_ONCE(net->smc.sysctl_rmem, net_smc_rmem_init);
 	net->smc.sysctl_max_links_per_lgr = SMC_LINKS_PER_LGR_MAX_PREFER;
 	net->smc.sysctl_max_conns_per_lgr = SMC_CONN_PER_LGR_PREFER;
+	/* disable handshake limitation by default */
+	net->smc.limit_smc_hs = 0;
 
 	return 0;
 
-- 
1.8.3.1

Re: [PATCH net-next] net/smc: add sysctl for smc_limit_hs

[Wen Gu]

On 2024/8/15 21:03, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake workqueue congested"),
> we introduce a mechanism to put constraint on SMC connections visit according to
> the pressure of SMC handshake process.
> 
> At that time, we believed that controlling the feature through netlink was sufficient,
> However, most people have realized now that netlink is not convenient in
> container scenarios, and sysctl is a more suitable approach.
> 
> In addition, it is not reasonable for us to initialize limit_smc_hs in
> smc_pnet_net_init, we made a mistable before. It should be initialized

nit: mistable -> mistake?

> in smc_sysctl_net_init(), just like other systcl.
> 
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>   net/smc/smc_pnet.c   |  3 ---
>   net/smc/smc_sysctl.c | 11 +++++++++++
>   2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 2adb92b..1dd3623 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -887,9 +887,6 @@ int smc_pnet_net_init(struct net *net)
>   
>   	smc_pnet_create_pnetids_list(net);
>   
> -	/* disable handshake limitation by default */
> -	net->smc.limit_smc_hs = 0;
> -
>   	return 0;
>   }
>   
> diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
> index 13f2bc0..2fab645 100644
> --- a/net/smc/smc_sysctl.c
> +++ b/net/smc/smc_sysctl.c
> @@ -90,6 +90,15 @@
>   		.extra1		= &conns_per_lgr_min,
>   		.extra2		= &conns_per_lgr_max,
>   	},
> +	{
> +		.procname	= "limit_smc_hs",
> +		.data		= &init_net.smc.limit_smc_hs,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ZERO,
> +		.extra2		= SYSCTL_ONE,
> +	},
>   };
>   
>   int __net_init smc_sysctl_net_init(struct net *net)
> @@ -121,6 +130,8 @@ int __net_init smc_sysctl_net_init(struct net *net)
>   	WRITE_ONCE(net->smc.sysctl_rmem, net_smc_rmem_init);
>   	net->smc.sysctl_max_links_per_lgr = SMC_LINKS_PER_LGR_MAX_PREFER;
>   	net->smc.sysctl_max_conns_per_lgr = SMC_CONN_PER_LGR_PREFER;
> +	/* disable handshake limitation by default */
> +	net->smc.limit_smc_hs = 0;
>   
>   	return 0;
>   

Re: [PATCH net-next] net/smc: add sysctl for smc_limit_hs

[D. Wythe]

On 8/15/24 9:14 PM, Wen Gu wrote:
>
>
> On 2024/8/15 21:03, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake 
>> workqueue congested"),
>> we introduce a mechanism to put constraint on SMC connections visit 
>> according to
>> the pressure of SMC handshake process.
>>
>> At that time, we believed that controlling the feature through 
>> netlink was sufficient,
>> However, most people have realized now that netlink is not convenient in
>> container scenarios, and sysctl is a more suitable approach.
>>
>> In addition, it is not reasonable for us to initialize limit_smc_hs in
>> smc_pnet_net_init, we made a mistable before. It should be initialized
>
> nit: mistable -> mistake?

Take it. Also, I suddenly realized that the reason for initializing 
limit_smc_hs in smc_pnet_net_init before
was because there was no smc_sysctl_net_init at that time ...

D. Wythe

>
>> in smc_sysctl_net_init(), just like other systcl.
>>
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   net/smc/smc_pnet.c   |  3 ---
>>   net/smc/smc_sysctl.c | 11 +++++++++++
>>   2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
>> index 2adb92b..1dd3623 100644
>> --- a/net/smc/smc_pnet.c
>> +++ b/net/smc/smc_pnet.c
>> @@ -887,9 +887,6 @@ int smc_pnet_net_init(struct net *net)
>>         smc_pnet_create_pnetids_list(net);
>>   -    /* disable handshake limitation by default */
>> -    net->smc.limit_smc_hs = 0;
>> -
>>       return 0;
>>   }
>>   diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
>> index 13f2bc0..2fab645 100644
>> --- a/net/smc/smc_sysctl.c
>> +++ b/net/smc/smc_sysctl.c
>> @@ -90,6 +90,15 @@
>>           .extra1        = &conns_per_lgr_min,
>>           .extra2        = &conns_per_lgr_max,
>>       },
>> +    {
>> +        .procname    = "limit_smc_hs",
>> +        .data        = &init_net.smc.limit_smc_hs,
>> +        .maxlen        = sizeof(int),
>> +        .mode        = 0644,
>> +        .proc_handler    = proc_dointvec_minmax,
>> +        .extra1        = SYSCTL_ZERO,
>> +        .extra2        = SYSCTL_ONE,
>> +    },
>>   };
>>     int __net_init smc_sysctl_net_init(struct net *net)
>> @@ -121,6 +130,8 @@ int __net_init smc_sysctl_net_init(struct net *net)
>>       WRITE_ONCE(net->smc.sysctl_rmem, net_smc_rmem_init);
>>       net->smc.sysctl_max_links_per_lgr = SMC_LINKS_PER_LGR_MAX_PREFER;
>>       net->smc.sysctl_max_conns_per_lgr = SMC_CONN_PER_LGR_PREFER;
>> +    /* disable handshake limitation by default */
>> +    net->smc.limit_smc_hs = 0;
>>         return 0;

[PATCH net-next] net/smc: add sysctl for smc_limit_hs

[D. Wythe]

From: "D. Wythe" <alibuda@linux.alibaba.com>

In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake workqueue congested"),
we introduce a mechanism to put constraint on SMC connections visit
according to the pressure of SMC handshake process.

At that time, we believed that controlling the feature through netlink
was sufficient. However, most people have realized now that netlink is
not convenient in container scenarios, and sysctl is a more suitable
approach.

In addition, since commit 462791bbfa35 ("net/smc: add sysctl interface for SMC")
had introcuded smc_sysctl_net_init(), it is reasonable for us to
initialize limit_smc_hs in it instead of initializing it in
smc_pnet_net_int().

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-by: Jan Karcher <jaka@linux.ibm.com>
---
 net/smc/smc_pnet.c   |  3 ---
 net/smc/smc_sysctl.c | 11 +++++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 2adb92b..1dd3623 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -887,9 +887,6 @@ int smc_pnet_net_init(struct net *net)
 
 	smc_pnet_create_pnetids_list(net);
 
-	/* disable handshake limitation by default */
-	net->smc.limit_smc_hs = 0;
-
 	return 0;
 }
 
diff --git a/net/smc/smc_sysctl.c b/net/smc/smc_sysctl.c
index 13f2bc0..2fab645 100644
--- a/net/smc/smc_sysctl.c
+++ b/net/smc/smc_sysctl.c
@@ -90,6 +90,15 @@
 		.extra1		= &conns_per_lgr_min,
 		.extra2		= &conns_per_lgr_max,
 	},
+	{
+		.procname	= "limit_smc_hs",
+		.data		= &init_net.smc.limit_smc_hs,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
 };
 
 int __net_init smc_sysctl_net_init(struct net *net)
@@ -121,6 +130,8 @@ int __net_init smc_sysctl_net_init(struct net *net)
 	WRITE_ONCE(net->smc.sysctl_rmem, net_smc_rmem_init);
 	net->smc.sysctl_max_links_per_lgr = SMC_LINKS_PER_LGR_MAX_PREFER;
 	net->smc.sysctl_max_conns_per_lgr = SMC_CONN_PER_LGR_PREFER;
+	/* disable handshake limitation by default */
+	net->smc.limit_smc_hs = 0;
 
 	return 0;
 
-- 
1.8.3.1

Re: [PATCH net-next] net/smc: add sysctl for smc_limit_hs

[Paolo Abeni]

On 9/6/24 04:35, D. Wythe wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake workqueue congested"),
> we introduce a mechanism to put constraint on SMC connections visit
> according to the pressure of SMC handshake process.
> 
> At that time, we believed that controlling the feature through netlink
> was sufficient. However, most people have realized now that netlink is
> not convenient in container scenarios, and sysctl is a more suitable
> approach.

Not blocking this patch, but could you please describe why/how NL is 
less convenient? is possibly just a matter of lack of command line tool 
to operate on NL? yaml to the rescue ;)

Cheers,

Paolo

Re: [PATCH net-next] net/smc: add sysctl for smc_limit_hs

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri,  6 Sep 2024 10:35:35 +0800 you wrote:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake workqueue congested"),
> we introduce a mechanism to put constraint on SMC connections visit
> according to the pressure of SMC handshake process.
> 
> At that time, we believed that controlling the feature through netlink
> was sufficient. However, most people have realized now that netlink is
> not convenient in container scenarios, and sysctl is a more suitable
> approach.
> 
> [...]

Here is the summary with links:
  - [net-next] net/smc: add sysctl for smc_limit_hs
    https://git.kernel.org/netdev/net-next/c/f8406a2fd279

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net/smc: add sysctl for smc_limit_hs

[D. Wythe]

On 9/10/24 6:10 PM, Paolo Abeni wrote:
> On 9/6/24 04:35, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> In commit 48b6190a0042 ("net/smc: Limit SMC visits when handshake 
>> workqueue congested"),
>> we introduce a mechanism to put constraint on SMC connections visit
>> according to the pressure of SMC handshake process.
>>
>> At that time, we believed that controlling the feature through netlink
>> was sufficient. However, most people have realized now that netlink is
>> not convenient in container scenarios, and sysctl is a more suitable
>> approach.
>
> Not blocking this patch, but could you please describe why/how NL is 
> less convenient? is possibly just a matter of lack of command line 
> tool to operate on NL? yaml to the rescue ;)
>
> Cheers,
>
> Paolo

Hi Paolo,

Based on the information I've gathered, there are several aspects:

1. There is a lack of support for YAML on NL in popular products.

2. The infrastructure related to NLwas not yet sound enough. For 
example, how to disable
settings for certain NL in container, while we can do it within sysctls. 
Also, regular synchronization.
NL may be able to implement it (maybe via Yaml ), but it still be waiting
for support in popular products.

Best wishes,
D. Wythe