On Mon, Apr 15, 2024 at 04:12:10PM -0700, Kuniyuki Iwashima wrote: > From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@xxxxxxxxxx> > Date: Fri, 12 Apr 2024 16:48:29 +0200 > > From: Joel Granados <j.granados@xxxxxxxxxxx> ... > > Signed-off-by: Joel Granados <j.granados@xxxxxxxxxxx> > > --- > > net/core/neighbour.c | 5 +---- > > net/core/sysctl_net_core.c | 9 ++++----- > > net/dccp/sysctl.c | 2 -- > > net/ieee802154/6lowpan/reassembly.c | 6 +----- > > net/ipv4/devinet.c | 5 ++--- > > net/ipv4/ip_fragment.c | 2 -- > > net/ipv4/route.c | 8 ++------ > > net/ipv4/sysctl_net_ipv4.c | 7 +++---- > > net/ipv4/xfrm4_policy.c | 1 - > > net/ipv6/addrconf.c | 5 +---- > > net/ipv6/icmp.c | 1 - > > net/ipv6/reassembly.c | 2 -- > > net/ipv6/route.c | 5 ----- > > net/ipv6/sysctl_net_ipv6.c | 4 +--- > > net/ipv6/xfrm6_policy.c | 1 - > > net/llc/sysctl_net_llc.c | 8 ++------ > > net/mpls/af_mpls.c | 3 +-- > > net/mptcp/ctrl.c | 1 - > > net/netrom/sysctl_net_netrom.c | 1 - > > net/phonet/sysctl.c | 1 - > > net/rds/ib_sysctl.c | 1 - > > net/rds/sysctl.c | 1 - > > net/rds/tcp.c | 1 - > > net/rose/sysctl_net_rose.c | 1 - > > net/rxrpc/sysctl.c | 1 - > > net/sctp/sysctl.c | 6 +----- > > net/smc/smc_sysctl.c | 1 - > > net/sunrpc/sysctl.c | 1 - > > net/sunrpc/xprtrdma/svc_rdma.c | 1 - > > net/sunrpc/xprtrdma/transport.c | 1 - > > net/sunrpc/xprtsock.c | 1 - > > net/tipc/sysctl.c | 1 - > > net/unix/sysctl_net_unix.c | 1 - > > net/x25/sysctl_net_x25.c | 1 - > > net/xfrm/xfrm_sysctl.c | 5 +---- > > 35 files changed, 20 insertions(+), 81 deletions(-) > > You may want to split patch based on subsystem or the type of changes > to make review easier. That is fair. It is a big chunk:). I'll put the trivial patches together to avoid having an 18 commits to instead have 8. This is my proposal based on MAINTAINERS file: ### Not in MAINTAINERS / Orphaned net/core/neighbour.c net/core/sysctl_net_core.c net/ieee802154/6lowpan/reassembly.c net/mpls/af_mpls.c net/unix/sysctl_net_unix.c net/dccp/sysctl.c ### NETWORKING net/ipv4/devinet.c net/ipv4/ip_fragment.c net/ipv4/route.c net/ipv4/sysctl_net_ipv4.c net/ipv4/xfrm4_policy.c net/ipv6/addrconf.c net/ipv6/icmp.c net/ipv6/reassembly.c net/ipv6/route.c net/ipv6/sysctl_net_ipv6.c net/ipv6/xfrm6_policy.c ### RDS net/rds/ib_sysctl.c net/rds/sysctl.c net/rds/tcp.c ### SUNRPC net/sunrpc/sysctl.c net/sunrpc/xprtrdma/svc_rdma.c net/sunrpc/xprtrdma/transport.c net/sunrpc/xprtsock.c ### LLC/MTCP/NETROM/PHONET/ROSE/RXRPC/SCTP/SMC/TIPC/x.25/IPSEC net/llc/sysctl_net_llc.c net/mptcp/ctrl.c net/netrom/sysctl_net_netrom.c net/phonet/sysctl.c net/rose/sysctl_net_rose.c net/rxrpc/sysctl.c net/sctp/sysctl.c net/smc/smc_sysctl.c net/tipc/sysctl.c net/x25/sysctl_net_x25.c net/xfrm/xfrm_sysctl.c > > > > > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > > index 552719c3bbc3..b0327402b3e6 100644 > > --- a/net/core/neighbour.c > > +++ b/net/core/neighbour.c > > @@ -3728,7 +3728,7 @@ static int neigh_proc_base_reachable_time(struct ctl_table *ctl, int write, > > > > static struct neigh_sysctl_table { > > struct ctl_table_header *sysctl_header; > > - struct ctl_table neigh_vars[NEIGH_VAR_MAX + 1]; > > + struct ctl_table neigh_vars[NEIGH_VAR_MAX]; > > } neigh_sysctl_template __read_mostly = { > > .neigh_vars = { > > NEIGH_SYSCTL_ZERO_INTMAX_ENTRY(MCAST_PROBES, "mcast_solicit"), > > @@ -3779,7 +3779,6 @@ static struct neigh_sysctl_table { > > .extra2 = SYSCTL_INT_MAX, > > .proc_handler = proc_dointvec_minmax, > > }, > > - {}, > > }, > > }; > > > > @@ -3807,8 +3806,6 @@ int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p, > > if (dev) { > > dev_name_source = dev->name; > > /* Terminate the table early */ > > You can remove this comment. Why? I do not think we should remove it because it is what the change to neigh_vars_size is doing. > > > > - memset(&t->neigh_vars[NEIGH_VAR_GC_INTERVAL], 0, > > - sizeof(t->neigh_vars[NEIGH_VAR_GC_INTERVAL])); > > neigh_vars_size = NEIGH_VAR_BASE_REACHABLE_TIME_MS + 1; > > } else { > > struct neigh_table *tbl = p->tbl; > > diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c > > index 6973dda3abda..46f5143e86be 100644 > > --- a/net/core/sysctl_net_core.c > > +++ b/net/core/sysctl_net_core.c > > @@ -660,7 +660,6 @@ static struct ctl_table net_core_table[] = { > > .proc_handler = proc_dointvec_minmax, > > .extra1 = SYSCTL_ZERO, > > }, > > - { } > > }; > > > > static struct ctl_table netns_core_table[] = { > > @@ -697,7 +696,6 @@ static struct ctl_table netns_core_table[] = { > > .extra2 = SYSCTL_ONE, > > .proc_handler = proc_dou8vec_minmax, > > }, > > - { } > > }; > > > > static int __init fb_tunnels_only_for_init_net_sysctl_setup(char *str) > > @@ -715,7 +713,8 @@ __setup("fb_tunnels=", fb_tunnels_only_for_init_net_sysctl_setup); > > > > static __net_init int sysctl_core_net_init(struct net *net) > > { > > - struct ctl_table *tbl, *tmp; > > + struct ctl_table *tbl; > > + size_t table_size = ARRAY_SIZE(netns_core_table); > > When you add a new variable, please keep reverse xmas tree. Thx for pointing this out. Was not aware of this quirk in net code. Will include it for my next version. > > Also, you can reuse this variable for the following > register_net_sysctl_sz(), but it's inconsistent in the > this patch.. > > table_size > * sysctl_route_net_init > * ipv4_sysctl_init_net > > ARRAY_SIZE > * __addrconf_sysctl_register > * ipv6_sysctl_net_init > * mpls_dev_sysctl_register > * sctp_sysctl_net_register > > > > > > tbl = netns_core_table; > > if (!net_eq(net, &init_net)) { > > @@ -723,8 +722,8 @@ static __net_init int sysctl_core_net_init(struct net *net) > > if (tbl == NULL) > > goto err_dup; > > > > - for (tmp = tbl; tmp->procname; tmp++) > > - tmp->data += (char *)net - (char *)&init_net; > > + for (int i = 0; i < table_size; ++i) > > .devinet_vars = { ... > > .extra1 = SYSCTL_ONE, > > }, > > - { } > > }; > > > > static __net_init int ipv4_sysctl_init_net(struct net *net) > > { > > struct ctl_table *table; > > + size_t table_size = ARRAY_SIZE(ipv4_net_table); > > nit: keep reverse xmax tree order. Ok. > > > > > > table = ipv4_net_table; > > if (!net_eq(net, &init_net)) { > > @@ -1517,7 +1516,7 @@ static __net_init int ipv4_sysctl_init_net(struct net *net) > > if (!table) > > goto err_alloc; > > > > - for (i = 0; i < ARRAY_SIZE(ipv4_net_table) - 1; i++) { ... > > > > static int __addrconf_sysctl_register(struct net *net, char *dev_name, > > @@ -7197,7 +7194,7 @@ static int __addrconf_sysctl_register(struct net *net, char *dev_name, > > if (!table) > > goto out; > > > > - for (i = 0; table[i].data; i++) { > > + for (i = 0; i < ARRAY_SIZE(addrconf_sysctl); i++) { > > ^^^ Did you mean reuse variable here? What does this mean? > > > > table[i].data += (char *)p - (char *)&ipv6_devconf; > > /* If one of these is already set, then it is not safe to > > * overwrite either of them: this makes proc_dointvec_minmax > > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > > index 1635da07285f..91cbf8e8009f 100644 > > --- a/net/ipv6/icmp.c > > +++ b/net/ipv6/icmp.c > > @@ -1206,7 +1206,6 @@ static struct ctl_table ipv6_icmp_table_template[] = { > > .extra1 = SYSCTL_ZERO, ... > > - { } > > }; > > > > static int __net_init ipv6_sysctl_net_init(struct net *net) > > @@ -264,7 +262,7 @@ static int __net_init ipv6_sysctl_net_init(struct net *net) > > if (!ipv6_table) > > goto out; > > /* Update the variables to point into the current struct net */ > > - for (i = 0; i < ARRAY_SIZE(ipv6_table_template) - 1; i++) > > + for (i = 0; i < ARRAY_SIZE(ipv6_table_template); i++) > > ^^^ Did you mean reuse variable here? What does this mean? > > > > ipv6_table[i].data += (void *)net - (void *)&init_net; > > > > ipv6_route_table = ipv6_route_sysctl_init(net); > > diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c > > index 42fb6996b077..499b5f5c19fc 100644 > > --- a/net/ipv6/xfrm6_policy.c > > +++ b/net/ipv6/xfrm6_policy.c > > @@ -184,7 +184,6 @@ static struct ctl_table xfrm6_policy_table[] = { > > .mode = 0644, > > .proc_handler = proc_dointvec, > > }, > > - { } > > }; > > > > static int __net_init xfrm6_net_sysctl_init(struct net *net) > > diff --git a/net/llc/sysctl_net_llc.c b/net/llc/sysctl_net_llc.c > > index 8443a6d841b0..72e101135f8c 100644 > > --- a/net/llc/sysctl_net_llc.c > > +++ b/net/llc/sysctl_net_llc.c > > @@ -44,11 +44,6 @@ static struct ctl_table llc2_timeout_table[] = { > > .mode = 0644, > > .proc_handler = proc_dointvec_jiffies, > > }, > > - { }, > > -}; > > - > > -static struct ctl_table llc_station_table[] = { > > - { }, > > }; > > > > static struct ctl_table_header *llc2_timeout_header; > > @@ -56,8 +51,9 @@ static struct ctl_table_header *llc_station_header; > > > > int __init llc_sysctl_init(void) > > { > > + struct ctl_table empty[1] = {}; > > llc2_timeout_header = register_net_sysctl(&init_net, "net/llc/llc2/timeout", llc2_timeout_table); > > - llc_station_header = register_net_sysctl(&init_net, "net/llc/station", llc_station_table); > > + llc_station_header = register_net_sysctl_sz(&init_net, "net/llc/station", empty, 0); > > Do we really need this ... ?? That is a good question, but its something that needs to be address outside this patchset. I'm just keeping current behaviour. > > > > > > if (!llc2_timeout_header || !llc_station_header) { > > llc_sysctl_exit(); > > diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c > > index 6dab883a08dd..e163fac55ffa 100644 > > --- a/net/mpls/af_mpls.c > > +++ b/net/mpls/af_mpls.c > > @@ -1393,7 +1393,6 @@ static const struct ctl_table mpls_dev_table[] = { > > .proc_handler = mpls_conf_proc, > > .data = MPLS_PERDEV_SYSCTL_OFFSET(input_enabled), > > }, > > - { } > > }; > > > > static int mpls_dev_sysctl_register(struct net_device *dev, > > @@ -2689,7 +2688,7 @@ static int mpls_net_init(struct net *net) > > /* Table data contains only offsets relative to the base of > > * the mdev at this point, so make them absolute. > > */ > > - for (i = 0; i < ARRAY_SIZE(mpls_table) - 1; i++) > > + for (i = 0; i < ARRAY_SIZE(mpls_table); i++) > > ^^^ > > > > table[i].data = (char *)net + (uintptr_t)table[i].data; > > > > net->mpls.ctl = register_net_sysctl_sz(net, "net/mpls", table, > > diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c > > index 13fe0748dde8..8bf7c26a0878 100644 > > --- a/net/mptcp/ctrl.c > > +++ b/net/mptcp/ctrl.c ... > > > > net->xfrm.sysctl_hdr = register_net_sysctl_sz(net, "net/core", table, > > table_size); > > > > -- > > 2.43.0 > B Thx for the review Best -- Joel Granados
Attachment:
signature.asc
Description: PGP signature