Re: [net-next,1/9] ipvs: make ip_vs_svc_table and ip_vs_svc_fwm_table per netns

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "FYI"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ipvs: make ip_vs_svc_table and ip_vs_svc_fwm_table per netns

This commit converts the global IPVS service tables and their
associated mutex from global to per-netns, improving lookup
performance in large-scale deployments with multiple network
namespaces.

> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 29a36709e7f3..074a204ec6db 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -33,6 +33,12 @@
>
>  #define IP_VS_HDR_INVERSE	1
>  #define IP_VS_HDR_ICMP		2
> +/*
> + *	Hash table: for virtual service lookups
> + */
> +#define IP_VS_SVC_TAB_BITS 8
> +#define IP_VS_SVC_TAB_SIZE BIT(IP_VS_SVC_TAB_BITS)
> +#define IP_VS_SVC_TAB_MASK (IP_VS_SVC_TAB_SIZE - 1)

The patch removes the global mutex definition DEFINE_MUTEX(__ip_vs_mutex)
from net/netfilter/ipvs/ip_vs_ctl.c and replaces all usage sites with the
per-netns ipvs->service_mutex. However, the extern declaration at line 52
of include/net/ip_vs.h is still present:

    extern struct mutex __ip_vs_mutex;

Since no in-tree code references __ip_vs_mutex after this conversion,
should this declaration be removed for consistency?

[ ... ]

> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index 068702894377..d871273ce917 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c

[ ... ]

> @@ -2306,9 +2282,9 @@ static struct ip_vs_service *ip_vs_info_array(struct seq_file *seq, loff_t pos)
>
>  	/* look in hash by protocol */
>  	for (idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
> -		hlist_for_each_entry_rcu(svc, &ip_vs_svc_table[idx], s_list) {
> -			if ((svc->ipvs == ipvs) && pos-- == 0) {
> -				iter->table = ip_vs_svc_table;
> +		hlist_for_each_entry_rcu(svc, &ipvs->svc_table[idx], s_list) {
> +			if (pos-- == 0) {
> +				iter->table = ipvs->svc_table;
>  				iter->bucket = idx;
>  				return svc;
>  			}
> @@ -2317,10 +2293,10 @@ static struct ip_vs_service *ip_vs_info_array(struct seq_file *seq, loff_t pos)
>
>  	/* keep looking in fwmark */
>  	for (idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
> -		hlist_for_each_entry_rcu(svc, &ip_vs_svc_fwm_table[idx],
> +		hlist_for_each_entry_rcu(svc, &ipvs->svc_fwm_table[idx],
>  					 f_list) {
> -			if ((svc->ipvs == ipvs) && pos-- == 0) {
> -				iter->table = ip_vs_svc_fwm_table;
> +			if (pos-- == 0) {
> +				iter->table = ipvs->svc_fwm_table;
>  				iter->bucket = idx;
>  				return svc;
>  			}

[ ... ]

> @@ -2414,7 +2392,7 @@ static int ip_vs_info_seq_show(struct seq_file *seq, void *v)
>
>  		if (svc->ipvs != ipvs)
>  			return 0;
> -		if (iter->table == ip_vs_svc_table) {
> +		if (iter->table == ipvs->svc_table) {
>  #ifdef CONFIG_IP_VS_IPV6
>  			if (svc->af == AF_INET6)
>  				seq_printf(seq, "%s  [%pI6]:%04X %s ",

The conversion to per-netns tables makes 'svc->ipvs != ipvs' always false
here in ip_vs_info_seq_show(). Every service in ipvs->svc_table belongs
to that netns by construction.

Looking at the patch, similar checks were removed everywhere else:
- ip_vs_flush(): removed 'svc->ipvs == ipvs' check
- ip_vs_dst_event(): removed 'svc->ipvs == ipvs' check
- ip_vs_zero_all(): removed 'svc->ipvs == ipvs' check
- __ip_vs_service_find(): removed 'svc->ipvs == ipvs' check
- __ip_vs_svc_fwm_find(): removed 'svc->ipvs == ipvs' check
- ip_vs_genl_dump_services(): removed 'svc->ipvs != ipvs' check
- __ip_vs_get_service_entries(): removed 'svc->ipvs != ipvs' check
- ip_vs_info_array(): removed 'svc->ipvs == ipvs' check

But this one in ip_vs_info_seq_show() was left behind. Should it also
be removed for consistency?