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

[PATCH net-next v2] 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>
---
v1 -> v2:

Modified the description in the commit and removed the incorrect
spelling.

 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 v2] net/smc: add sysctl for smc_limit_hs

[Wen Gu]

On 2024/8/21 10:36, 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, 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>

LGTM. Thanks!

Reviewed-by: Wen Gu <guwen@linux.alibaba.com>

> ---
> v1 -> v2:
> 
> Modified the description in the commit and removed the incorrect
> spelling.
> 
>   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 v2] net/smc: add sysctl for smc_limit_hs

[Jan Karcher]

On 21/08/2024 04:36, 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.

Hi D.

thanks for your contribution.
What i wonder is should we prefer the use of netlink > sysctl or not?
To the upstream maintainers: Is there a prefernce for the net tree?

My impression from past discussions is that netlink should be chosen 
over sysctl.
If so, why is it inconvenient to use netlink in containers?
Can this be changed?

Other then the general discussion the changhes look good to me.

Reviewed-by: Jan Karcher <jaka@linux.ibm.com>


> 
> 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>
> ---
> v1 -> v2:
> 
> Modified the description in the commit and removed the incorrect
> spelling.
> 
>   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 v2] net/smc: add sysctl for smc_limit_hs

[Jakub Kicinski]

On Wed, 21 Aug 2024 10:03:49 +0200 Jan Karcher wrote:
> If so, why is it inconvenient to use netlink in containers?
> Can this be changed?

Adding a YAML spec for the link to make it easily accessible from
C/C++/Python applications seems like a better direction.

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

[D. Wythe]

On 8/21/24 4:03 PM, Jan Karcher wrote:
>
>
> On 21/08/2024 04:36, 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.
>
> Hi D.
>
> thanks for your contribution.
> What i wonder is should we prefer the use of netlink > sysctl or not?
> To the upstream maintainers: Is there a prefernce for the net tree?
>
> My impression from past discussions is that netlink should be chosen 
> over sysctl.
> If so, why is it inconvenient to use netlink in containers?
> Can this be changed?
>
> Other then the general discussion the changhes look good to me.
>
> Reviewed-by: Jan Karcher <jaka@linux.ibm.com>
>

Hi Jan,

I noticed that there have been relevant discussions before, perhaps this 
will be helpful to you.

Link: https://lore.kernel.org/netdev/20220224020253.GF5443@linux.alibaba.com


Best wishes,
D. Wythe


>
>>
>> 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>
>> ---
>> v1 -> v2:
>>
>> Modified the description in the commit and removed the incorrect
>> spelling.
>>
>>   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 v2] net/smc: add sysctl for smc_limit_hs

[Jan Karcher]

On 26/08/2024 05:02, D. Wythe wrote:
> 
> 
> On 8/21/24 4:03 PM, Jan Karcher wrote:
>>
>>
>> On 21/08/2024 04:36, 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.
>>
>> Hi D.
>>
>> thanks for your contribution.
>> What i wonder is should we prefer the use of netlink > sysctl or not?
>> To the upstream maintainers: Is there a prefernce for the net tree?
>>
>> My impression from past discussions is that netlink should be chosen 
>> over sysctl.
>> If so, why is it inconvenient to use netlink in containers?
>> Can this be changed?
>>
>> Other then the general discussion the changhes look good to me.
>>
>> Reviewed-by: Jan Karcher <jaka@linux.ibm.com>
>>
> 
> Hi Jan,
> 
> I noticed that there have been relevant discussions before, perhaps this 
> will be helpful to you.
> 
> Link: 
> https://lore.kernel.org/netdev/20220224020253.GF5443@linux.alibaba.com

Hi D.,

thanks for the pointer! If i understood Jakub correct it should be 
possible to add a yaml definition for the SMC netlink anbd modify it vie 
a small program.

That said I'm not to familia with the use of them in containers. So if 
you say this is the better solution and everyone is fine with yet 
another sysctl it is fine by me.

Thanks
- Jan

> 
> 
> Best wishes,
> D. Wythe
> 
> 
>>
>>>
>>> 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>
>>> ---
>>> v1 -> v2:
>>>
>>> Modified the description in the commit and removed the incorrect
>>> spelling.
>>>
>>>   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 v2] net/smc: add sysctl for smc_limit_hs

[D. Wythe]

On 8/21/24 10:36 AM, 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.
>

Hi everyone,

Just a quick reminder regarding the patch that seems to have been 
overlooked, possibly dues to its status was
mistakenly updated to change request ? It seems that no further 
modifications are needed.

Best wishes,
D. Wythe

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

[Andrew Lunn]

> Hi everyone,
> 
> Just a quick reminder regarding the patch that seems to have been
> overlooked, possibly dues to its status was
> mistakenly updated to change request ?

Once it gets set to change request, it is effectively dead. Please
repost.

	Andrew

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

[D. Wythe]

On 9/6/24 5:12 AM, Andrew Lunn wrote:
>> Hi everyone,
>>
>> Just a quick reminder regarding the patch that seems to have been
>> overlooked, possibly dues to its status was
>> mistakenly updated to change request ?
> Once it gets set to change request, it is effectively dead. Please
> repost.
>
> 	Andrew

Thanks for the reminder,  I will repost it later.

Best wishes,
D. Wythe