From: Joel Granados <j.granados@samsung.com>
To: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: <devnull+j.granados.samsung.com@kernel.org>, <Dai.Ngo@oracle.com>,
<alex.aring@gmail.com>, <alibuda@linux.alibaba.com>,
<allison.henderson@oracle.com>, <anna@kernel.org>,
<bridge@lists.linux.dev>, <chuck.lever@oracle.com>,
<coreteam@netfilter.org>, <courmisch@gmail.com>,
<davem@davemloft.net>, <dccp@vger.kernel.org>,
<dhowells@redhat.com>, <dsahern@kernel.org>,
<edumazet@google.com>, <fw@strlen.de>, <geliang@kernel.org>,
<guwen@linux.alibaba.com>, <herbert@gondor.apana.org.au>,
<horms@verge.net.au>, <ja@ssi.bg>, <jaka@linux.ibm.com>,
<jlayton@kernel.org>, <jmaloy@redhat.com>, <jreuter@yaina.de>,
<kadlec@netfilter.org>, <keescook@chromium.org>,
<kolga@netapp.com>, <kuba@kernel.org>,
<linux-afs@lists.infradead.org>, <linux-hams@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-nfs@vger.kernel.org>,
<linux-rdma@vger.kernel.org>, <linux-s390@vger.kernel.org>,
<linux-sctp@vger.kernel.org>, <linux-wpan@vger.kernel.org>,
<linux-x25@vger.kernel.org>, <lucien.xin@gmail.com>,
<lvs-devel@vger.kernel.org>, <marc.dionne@auristor.com>,
<marcelo.leitner@gmail.com>, <martineau@kernel.org>,
<matttbe@kernel.org>, <mcgrof@kernel.org>,
<miquel.raynal@bootlin.com>, <mptcp@lists.linux.dev>,
<ms@dev.tdt.de>, <neilb@suse.de>, <netdev@vger.kernel.org>,
<netfilter-devel@vger.kernel.org>, <pabeni@redhat.com>,
<pablo@netfilter.org>, <ralf@linux-mips.org>,
<razor@blackwall.org>, <rds-devel@oss.oracle.com>,
<roopa@nvidia.com>, <stefan@datenfreihafen.org>,
<steffen.klassert@secunet.com>,
<tipc-discussion@lists.sourceforge.net>, <tom@talpey.com>,
<tonylu@linux.alibaba.com>, <trond.myklebust@hammerspace.com>,
<wenjia@linux.ibm.com>, <ying.xue@windriver.com>
Subject: Re: [PATCH v3 1/4] networking: Remove the now superfluous sentinel elements from ctl_table array
Date: Tue, 16 Apr 2024 11:45:56 +0200 [thread overview]
Message-ID: <20240416094556.cv4k3uqthqqpln3h@joelS2.panther.com> (raw)
In-Reply-To: <20240415231210.22785-1-kuniyu@amazon.com>
[-- Attachment #1: Type: text/plain, Size: 11758 bytes --]
On Mon, Apr 15, 2024 at 04:12:10PM -0700, Kuniyuki Iwashima wrote:
> From: Joel Granados via B4 Relay <devnull+j.granados.samsung.com@kernel.org>
> Date: Fri, 12 Apr 2024 16:48:29 +0200
> > From: Joel Granados <j.granados@samsung.com>
...
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
next prev parent reply other threads:[~2024-04-16 9:46 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 14:48 [PATCH v3 0/4] sysctl: Remove sentinel elements from networking Joel Granados via B4 Relay
2024-04-12 14:48 ` [PATCH v3 1/4] networking: Remove the now superfluous sentinel elements from ctl_table array Joel Granados via B4 Relay
2024-04-15 23:12 ` Kuniyuki Iwashima
2024-04-16 8:18 ` Paolo Abeni
2024-04-16 12:32 ` Joel Granados
2024-04-16 9:45 ` Joel Granados [this message]
2024-04-12 14:48 ` [PATCH v3 2/4] netfilter: " Joel Granados via B4 Relay
2024-04-12 14:48 ` [PATCH v3 3/4] appletalk: " Joel Granados via B4 Relay
2024-04-12 14:48 ` [PATCH v3 4/4] ax.25: " Joel Granados via B4 Relay
2024-04-15 23:18 ` Kuniyuki Iwashima
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240416094556.cv4k3uqthqqpln3h@joelS2.panther.com \
--to=j.granados@samsung.com \
--cc=Dai.Ngo@oracle.com \
--cc=alex.aring@gmail.com \
--cc=alibuda@linux.alibaba.com \
--cc=allison.henderson@oracle.com \
--cc=anna@kernel.org \
--cc=bridge@lists.linux.dev \
--cc=chuck.lever@oracle.com \
--cc=coreteam@netfilter.org \
--cc=courmisch@gmail.com \
--cc=davem@davemloft.net \
--cc=dccp@vger.kernel.org \
--cc=devnull+j.granados.samsung.com@kernel.org \
--cc=dhowells@redhat.com \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=fw@strlen.de \
--cc=geliang@kernel.org \
--cc=guwen@linux.alibaba.com \
--cc=herbert@gondor.apana.org.au \
--cc=horms@verge.net.au \
--cc=ja@ssi.bg \
--cc=jaka@linux.ibm.com \
--cc=jlayton@kernel.org \
--cc=jmaloy@redhat.com \
--cc=jreuter@yaina.de \
--cc=kadlec@netfilter.org \
--cc=keescook@chromium.org \
--cc=kolga@netapp.com \
--cc=kuba@kernel.org \
--cc=kuniyu@amazon.com \
--cc=linux-afs@lists.infradead.org \
--cc=linux-hams@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=linux-wpan@vger.kernel.org \
--cc=linux-x25@vger.kernel.org \
--cc=lucien.xin@gmail.com \
--cc=lvs-devel@vger.kernel.org \
--cc=marc.dionne@auristor.com \
--cc=marcelo.leitner@gmail.com \
--cc=martineau@kernel.org \
--cc=matttbe@kernel.org \
--cc=mcgrof@kernel.org \
--cc=miquel.raynal@bootlin.com \
--cc=mptcp@lists.linux.dev \
--cc=ms@dev.tdt.de \
--cc=neilb@suse.de \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pablo@netfilter.org \
--cc=ralf@linux-mips.org \
--cc=razor@blackwall.org \
--cc=rds-devel@oss.oracle.com \
--cc=roopa@nvidia.com \
--cc=stefan@datenfreihafen.org \
--cc=steffen.klassert@secunet.com \
--cc=tipc-discussion@lists.sourceforge.net \
--cc=tom@talpey.com \
--cc=tonylu@linux.alibaba.com \
--cc=trond.myklebust@hammerspace.com \
--cc=wenjia@linux.ibm.com \
--cc=ying.xue@windriver.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).