[PATCH 00/14] sysctl: Add a size argument to register functions in sysctl

[PATCH 00/14] sysctl: Add a size argument to register functions in sysctl

[Joel Granados]

What?
These commits set things up so we can start removing the sentinel elements.
They modify sysctl and net_sysctl internals so that registering a ctl_table
that contains a sentinel gives the same result as passing a table_size
calculated from the ctl_table array without a sentinel. We accomplish this by
introducing a table_size argument in the same place where procname is checked
for NULL. The idea is for it to keep stopping when it hits ->procname == NULL,
while the sentinel is still present. And when the sentinel is removed, it will
stop on the table_size (thx to jani.nikula@linux.intel.com for the discussion
that led to this). This allows us to remove sentinels from one (or several)
files at a time.

These commits are part of a bigger set containing the removal of ctl_table sentinel
(https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V1).
The idea is to make the review process easier by chunking the 65+ commits into
manageable pieces. Even though I had already sent a V1 with the full set, I'll
restart the count as this is the first version of this chunk.

My idea is to send out one chunk at a time so it can be reviewed separately
from the others without the noise from parallel related sets. After this first
chunk will come 6 that remove the sentinel element from "arch/*, drivers/*,
fs/*, kernel/*, net/* and miscellaneous. And then a final one that removes the
->procname == NULL check other miscellaneous details. You can see all commits here
(https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V1).

Commits in this chunk:
* Preparation commits:
    start : sysctl: Prefer ctl_table_header in proc_sysct
    end   : sysctl: Add size argument to init_header
  These are preparation commits that make sure that we have the
  ctl_table_header where we need the array size.

* Add size to __register_sysctl_table, __register_sysctl_init and register_sysctl
    start : sysctl: Add a size arg to __register_sysctl_table
    end   : sysctl: Add size arg to __register_sysctl_init
  Here we replace the existing register functions with macros that add the
  ARRAY_SIZE automatically. Unfortunately these macros cannot be used for the
  register calls that pass a pointer; in this situation we add register
  functions with an table_size argument (thx to greg@kroah.com for bringing
  this to my attention)

* Add size to register_net_sysctl
    start : sysctl: Add size to register_net_sysctl function
    end   : sysctl: SIZE_MAX->ARRAY_SIZE in register_net_sysctl
  register_net_sysctl is an indirection function to the sysctl registrations
  and needed a several commits to add table_size to all its callers. We
  temporarily use SIZE_MAX to avoid compiler warnings while we change to
  register_net_sysctl to register_net_sysctl_sz; we remove it with the
  penultimate patch of this set. Finally, we make sure to adjust the calculated
  size every time there is a check for unprivileged users.

* Add size as additional stopping criteria
    commit : sysctl: Use size as stopping criteria for list macro
  We add table_size check in the main macro within proc_sysctl.c. This commit
  allows the removal of the sentinel element by chunks.

Why?
This is part of the push to trim down kernel/sysctl.c by moving the large array
that was causing merge conflicts. Most of the work is already done and what is
left is to remove the now unneeded empty last (sentinel) element in the
ctl_table arrays and plumb everything within the sysctl infrastructure so it
understands sizes instead of sentinels.

Here are some related threads to give more context:
* This is a patch set that replaces register_sysctl_table with register_sysctl
  https://lore.kernel.org/all/20230302204612.782387-1-mcgrof@kernel.org/
* Patch set to deprecate register_sysctl_paths()
  https://lore.kernel.org/all/20230302202826.776286-1-mcgrof@kernel.org/
* Here there is an explicit expectation for the removal of the sentinel element.
  https://lore.kernel.org/all/20230321130908.6972-1-frank.li@vivo.com
* The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread
  https://lore.kernel.org/all/20220220060626.15885-1-tangmeng@uniontech.com

Testing:
* Ran sysctl selftests (./tools/testing/selftests/sysctl/sysctl.sh)
* Successfully ran this through 0-day

Misc:
A consequence of eventually removing all the sentinels (64 bytes per sentinel)
is the bytes we save. Here I include numbers for when all sentinels are removed
to contextualize this chunk
  * bloat-o-meter:
    The "yesall" configuration results save 9158 bytes (you can see the output here
    https://lore.kernel.org/all/20230621091000.424843-1-j.granados@samsung.com/.
    The "tiny" configuration + CONFIG_SYSCTL save 1215 bytes (you can see the
    output here [2])
  * memory usage:
    As we no longer need the sentinel element within proc_sysctl.c, we save some
    bytes in main memory as well. In my testing kernel I measured a difference of
    6720 bytes. I include the way to measure this in [1]

Comments/feedback greatly appreciated

Best
Joel

[1]
To measure the in memory savings apply this patch on top of
https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V1
"
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5f413bfd6271..9aa8374c0ef1 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -975,6 +975,7 @@ static struct ctl_dir *new_dir(struct ctl_table_set *set,
        table[0].procname = new_name;
        table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO;
        init_header(&new->header, set->dir.header.root, set, node, table, 1);
+       printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_table));

        return new;
 }
@@ -1202,6 +1203,7 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_
                    head->ctl_table_size);
        links->nreg = head->ctl_table_size;

+       printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_table));
        return links;
 }

"
and then run the following bash script in the kernel:

accum=0
for n in $(dmesg | grep kzalloc | awk '{print $3}') ; do
    echo $n
    accum=$(calc "$accum + $n")
done
echo $accum

[2]
bloat-o-meter with "tiny" config:
add/remove: 0/2 grow/shrink: 33/24 up/down: 470/-1685 (-1215)
Function                                     old     new   delta
insert_header                                831     966    +135
__register_sysctl_table                      971    1092    +121
get_links                                    177     226     +49
put_links                                    167     186     +19
erase_header                                  55      66     +11
sysctl_init_bases                             59      69     +10
setup_sysctl_set                              65      73      +8
utsname_sysctl_init                           26      31      +5
sld_mitigate_sysctl_init                      33      38      +5
setup_userns_sysctls                         158     163      +5
sched_rt_sysctl_init                          33      38      +5
sched_fair_sysctl_init                        33      38      +5
sched_dl_sysctl_init                          33      38      +5
random_sysctls_init                           33      38      +5
page_writeback_init                          122     127      +5
oom_init                                      73      78      +5
kernel_panic_sysctls_init                     33      38      +5
kernel_exit_sysctls_init                      33      38      +5
init_umh_sysctls                              33      38      +5
init_signal_sysctls                           33      38      +5
init_pipe_fs                                  94      99      +5
init_fs_sysctls                               33      38      +5
init_fs_stat_sysctls                          33      38      +5
init_fs_namespace_sysctls                     33      38      +5
init_fs_namei_sysctls                         33      38      +5
init_fs_inode_sysctls                         33      38      +5
init_fs_exec_sysctls                          33      38      +5
init_fs_dcache_sysctls                        33      38      +5
register_sysctl                               22      25      +3
__register_sysctl_init                         9      12      +3
user_namespace_sysctl_init                   149     151      +2
sched_core_sysctl_init                        38      40      +2
register_sysctl_mount_point                   13      15      +2
vm_table                                    1344    1280     -64
vm_page_writeback_sysctls                    512     448     -64
vm_oom_kill_table                            256     192     -64
uts_kern_table                               448     384     -64
usermodehelper_table                         192     128     -64
user_table                                   576     512     -64
sld_sysctls                                  128      64     -64
signal_debug_table                           128      64     -64
sched_rt_sysctls                             256     192     -64
sched_fair_sysctls                           128      64     -64
sched_dl_sysctls                             192     128     -64
sched_core_sysctls                            64       -     -64
root_table                                   128      64     -64
random_table                                 448     384     -64
namei_sysctls                                320     256     -64
kern_table                                  1792    1728     -64
kern_panic_table                             128      64     -64
kern_exit_table                              128      64     -64
inodes_sysctls                               192     128     -64
fs_stat_sysctls                              256     192     -64
fs_shared_sysctls                            192     128     -64
fs_pipe_sysctls                              256     192     -64
fs_namespace_sysctls                         128      64     -64
fs_exec_sysctls                              128      64     -64
fs_dcache_sysctls                            128      64     -64
init_header                                   85       -     -85
Total: Before=1877669, After=1876454, chg -0.06%

base:  fdf0eaf11452

Joel Granados (14):
  sysctl: Prefer ctl_table_header in proc_sysctl
  sysctl: Use ctl_table_header in list_for_each_table_entry
  sysctl: Add ctl_table_size to ctl_table_header
  sysctl: Add size argument to init_header
  sysctl: Add a size arg to __register_sysctl_table
  sysctl: Add size to register_sysctl
  sysctl: Add size arg to __register_sysctl_init
  sysctl: Add size to register_net_sysctl function
  ax.25: Update to register_net_sysctl_sz
  netfilter: Update to register_net_sysctl_sz
  networking: Update to register_net_sysctl_sz
  vrf: Update to register_net_sysctl_sz
  sysctl: SIZE_MAX->ARRAY_SIZE in register_net_sysctl
  sysctl: Use size as stopping criteria for list macro

 arch/arm64/kernel/armv8_deprecated.c    |  2 +-
 arch/s390/appldata/appldata_base.c      |  2 +-
 drivers/net/vrf.c                       |  3 +-
 fs/proc/proc_sysctl.c                   | 88 ++++++++++++-------------
 include/linux/sysctl.h                  | 31 +++++++--
 include/net/ipv6.h                      |  2 +
 include/net/net_namespace.h             | 10 +--
 ipc/ipc_sysctl.c                        |  4 +-
 ipc/mq_sysctl.c                         |  4 +-
 kernel/ucount.c                         |  5 +-
 net/ax25/sysctl_net_ax25.c              |  3 +-
 net/bridge/br_netfilter_hooks.c         |  3 +-
 net/core/neighbour.c                    |  8 ++-
 net/core/sysctl_net_core.c              |  3 +-
 net/ieee802154/6lowpan/reassembly.c     |  8 ++-
 net/ipv4/devinet.c                      |  3 +-
 net/ipv4/ip_fragment.c                  |  3 +-
 net/ipv4/route.c                        |  8 ++-
 net/ipv4/sysctl_net_ipv4.c              |  3 +-
 net/ipv4/xfrm4_policy.c                 |  3 +-
 net/ipv6/addrconf.c                     |  3 +-
 net/ipv6/icmp.c                         |  5 ++
 net/ipv6/netfilter/nf_conntrack_reasm.c |  3 +-
 net/ipv6/reassembly.c                   |  3 +-
 net/ipv6/route.c                        | 13 ++--
 net/ipv6/sysctl_net_ipv6.c              | 16 +++--
 net/ipv6/xfrm6_policy.c                 |  3 +-
 net/mpls/af_mpls.c                      | 72 ++++++++++----------
 net/mptcp/ctrl.c                        |  3 +-
 net/netfilter/ipvs/ip_vs_ctl.c          |  8 ++-
 net/netfilter/ipvs/ip_vs_lblc.c         | 10 ++-
 net/netfilter/ipvs/ip_vs_lblcr.c        | 10 ++-
 net/netfilter/nf_conntrack_standalone.c |  4 +-
 net/netfilter/nf_log.c                  |  7 +-
 net/rds/tcp.c                           |  3 +-
 net/sctp/sysctl.c                       |  4 +-
 net/smc/smc_sysctl.c                    |  3 +-
 net/sysctl_net.c                        | 26 +++++---
 net/unix/sysctl_net_unix.c              |  3 +-
 net/xfrm/xfrm_sysctl.c                  |  8 ++-
 40 files changed, 254 insertions(+), 149 deletions(-)

-- 
2.30.2

[PATCH 01/14] sysctl: Prefer ctl_table_header in proc_sysctl

[Joel Granados]

This is a preparation commit to use ctl_table_header instead of
ctl_table as the element that is passed around in proc_sysctl.c.
This will become necessary when the size can no longer be calculated
by searching for an empty sentinel in the ctl_table array and
will live in the ctl_table_header.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 fs/proc/proc_sysctl.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5ea42653126e..94d71446da39 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1125,11 +1125,11 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 	return err;
 }
 
-static int sysctl_check_table(const char *path, struct ctl_table *table)
+static int sysctl_check_table(const char *path, struct ctl_table_header *header)
 {
 	struct ctl_table *entry;
 	int err = 0;
-	list_for_each_table_entry(entry, table) {
+	list_for_each_table_entry(entry, header->ctl_table) {
 		if ((entry->proc_handler == proc_dostring) ||
 		    (entry->proc_handler == proc_dobool) ||
 		    (entry->proc_handler == proc_dointvec) ||
@@ -1159,8 +1159,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 	return err;
 }
 
-static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table *table,
-	struct ctl_table_root *link_root)
+static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_header *head)
 {
 	struct ctl_table *link_table, *entry, *link;
 	struct ctl_table_header *links;
@@ -1170,7 +1169,7 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table
 
 	name_bytes = 0;
 	nr_entries = 0;
-	list_for_each_table_entry(entry, table) {
+	list_for_each_table_entry(entry, head->ctl_table) {
 		nr_entries++;
 		name_bytes += strlen(entry->procname) + 1;
 	}
@@ -1189,12 +1188,12 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table
 	link_name = (char *)&link_table[nr_entries + 1];
 	link = link_table;
 
-	list_for_each_table_entry(entry, table) {
+	list_for_each_table_entry(entry, head->ctl_table) {
 		int len = strlen(entry->procname) + 1;
 		memcpy(link_name, entry->procname, len);
 		link->procname = link_name;
 		link->mode = S_IFLNK|S_IRWXUGO;
-		link->data = link_root;
+		link->data = head->root;
 		link_name += len;
 		link++;
 	}
@@ -1205,15 +1204,16 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table
 }
 
 static bool get_links(struct ctl_dir *dir,
-	struct ctl_table *table, struct ctl_table_root *link_root)
+		      struct ctl_table_header *header,
+		      struct ctl_table_root *link_root)
 {
-	struct ctl_table_header *head;
+	struct ctl_table_header *tmp_head;
 	struct ctl_table *entry, *link;
 
 	/* Are there links available for every entry in table? */
-	list_for_each_table_entry(entry, table) {
+	list_for_each_table_entry(entry, header->ctl_table) {
 		const char *procname = entry->procname;
-		link = find_entry(&head, dir, procname, strlen(procname));
+		link = find_entry(&tmp_head, dir, procname, strlen(procname));
 		if (!link)
 			return false;
 		if (S_ISDIR(link->mode) && S_ISDIR(entry->mode))
@@ -1224,10 +1224,10 @@ static bool get_links(struct ctl_dir *dir,
 	}
 
 	/* The checks passed.  Increase the registration count on the links */
-	list_for_each_table_entry(entry, table) {
+	list_for_each_table_entry(entry, header->ctl_table) {
 		const char *procname = entry->procname;
-		link = find_entry(&head, dir, procname, strlen(procname));
-		head->nreg++;
+		link = find_entry(&tmp_head, dir, procname, strlen(procname));
+		tmp_head->nreg++;
 	}
 	return true;
 }
@@ -1246,13 +1246,13 @@ static int insert_links(struct ctl_table_header *head)
 	if (IS_ERR(core_parent))
 		return 0;
 
-	if (get_links(core_parent, head->ctl_table, head->root))
+	if (get_links(core_parent, head, head->root))
 		return 0;
 
 	core_parent->header.nreg++;
 	spin_unlock(&sysctl_lock);
 
-	links = new_links(core_parent, head->ctl_table, head->root);
+	links = new_links(core_parent, head);
 
 	spin_lock(&sysctl_lock);
 	err = -ENOMEM;
@@ -1260,7 +1260,7 @@ static int insert_links(struct ctl_table_header *head)
 		goto out;
 
 	err = 0;
-	if (get_links(core_parent, head->ctl_table, head->root)) {
+	if (get_links(core_parent, head, head->root)) {
 		kfree(links);
 		goto out;
 	}
@@ -1371,7 +1371,7 @@ struct ctl_table_header *__register_sysctl_table(
 
 	node = (struct ctl_node *)(header + 1);
 	init_header(header, root, set, node, table);
-	if (sysctl_check_table(path, table))
+	if (sysctl_check_table(path, header))
 		goto fail;
 
 	spin_lock(&sysctl_lock);
-- 
2.30.2

[PATCH 02/14] sysctl: Use ctl_table_header in list_for_each_table_entry

[Joel Granados]

Now that the ctl_header is the preferred way of passing ctl_table
elements around, we use it (the header) in the list_for_each_table_entry
macro.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 fs/proc/proc_sysctl.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 94d71446da39..884460b0385b 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -19,8 +19,8 @@
 #include <linux/kmemleak.h>
 #include "internal.h"
 
-#define list_for_each_table_entry(entry, table) \
-	for ((entry) = (table); (entry)->procname; (entry)++)
+#define list_for_each_table_entry(entry, header) \
+	for ((entry) = (header->ctl_table); (entry)->procname; (entry)++)
 
 static const struct dentry_operations proc_sys_dentry_operations;
 static const struct file_operations proc_sys_file_operations;
@@ -204,7 +204,7 @@ static void init_header(struct ctl_table_header *head,
 	if (node) {
 		struct ctl_table *entry;
 
-		list_for_each_table_entry(entry, table) {
+		list_for_each_table_entry(entry, head) {
 			node->header = head;
 			node++;
 		}
@@ -215,7 +215,7 @@ static void erase_header(struct ctl_table_header *head)
 {
 	struct ctl_table *entry;
 
-	list_for_each_table_entry(entry, head->ctl_table)
+	list_for_each_table_entry(entry, head)
 		erase_entry(head, entry);
 }
 
@@ -242,7 +242,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
 	err = insert_links(header);
 	if (err)
 		goto fail_links;
-	list_for_each_table_entry(entry, header->ctl_table) {
+	list_for_each_table_entry(entry, header) {
 		err = insert_entry(header, entry);
 		if (err)
 			goto fail;
@@ -1129,7 +1129,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header)
 {
 	struct ctl_table *entry;
 	int err = 0;
-	list_for_each_table_entry(entry, header->ctl_table) {
+	list_for_each_table_entry(entry, header) {
 		if ((entry->proc_handler == proc_dostring) ||
 		    (entry->proc_handler == proc_dobool) ||
 		    (entry->proc_handler == proc_dointvec) ||
@@ -1169,7 +1169,7 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_
 
 	name_bytes = 0;
 	nr_entries = 0;
-	list_for_each_table_entry(entry, head->ctl_table) {
+	list_for_each_table_entry(entry, head) {
 		nr_entries++;
 		name_bytes += strlen(entry->procname) + 1;
 	}
@@ -1188,7 +1188,7 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_
 	link_name = (char *)&link_table[nr_entries + 1];
 	link = link_table;
 
-	list_for_each_table_entry(entry, head->ctl_table) {
+	list_for_each_table_entry(entry, head) {
 		int len = strlen(entry->procname) + 1;
 		memcpy(link_name, entry->procname, len);
 		link->procname = link_name;
@@ -1211,7 +1211,7 @@ static bool get_links(struct ctl_dir *dir,
 	struct ctl_table *entry, *link;
 
 	/* Are there links available for every entry in table? */
-	list_for_each_table_entry(entry, header->ctl_table) {
+	list_for_each_table_entry(entry, header) {
 		const char *procname = entry->procname;
 		link = find_entry(&tmp_head, dir, procname, strlen(procname));
 		if (!link)
@@ -1224,7 +1224,7 @@ static bool get_links(struct ctl_dir *dir,
 	}
 
 	/* The checks passed.  Increase the registration count on the links */
-	list_for_each_table_entry(entry, header->ctl_table) {
+	list_for_each_table_entry(entry, header) {
 		const char *procname = entry->procname;
 		link = find_entry(&tmp_head, dir, procname, strlen(procname));
 		tmp_head->nreg++;
@@ -1356,12 +1356,14 @@ struct ctl_table_header *__register_sysctl_table(
 {
 	struct ctl_table_root *root = set->dir.header.root;
 	struct ctl_table_header *header;
+	struct ctl_table_header h_tmp;
 	struct ctl_dir *dir;
 	struct ctl_table *entry;
 	struct ctl_node *node;
 	int nr_entries = 0;
 
-	list_for_each_table_entry(entry, table)
+	h_tmp.ctl_table = table;
+	list_for_each_table_entry(entry, (&h_tmp))
 		nr_entries++;
 
 	header = kzalloc(sizeof(struct ctl_table_header) +
@@ -1471,7 +1473,7 @@ static void put_links(struct ctl_table_header *header)
 	if (IS_ERR(core_parent))
 		return;
 
-	list_for_each_table_entry(entry, header->ctl_table) {
+	list_for_each_table_entry(entry, header) {
 		struct ctl_table_header *link_head;
 		struct ctl_table *link;
 		const char *name = entry->procname;
-- 
2.30.2

[PATCH 03/14] sysctl: Add ctl_table_size to ctl_table_header

[Joel Granados]

The new ctl_table_size element will hold the size of the ctl_table
contained in the header. This value is passed by the callers to the
sysctl register infrastructure.

This is a preparation commit that allows us to systematically add
ctl_table_size and start using it only when it is in all the places
where there is a sysctl registration.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 include/linux/sysctl.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 59d451f455bf..33252ad58ebe 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -159,12 +159,22 @@ struct ctl_node {
 	struct ctl_table_header *header;
 };
 
-/* struct ctl_table_header is used to maintain dynamic lists of
-   struct ctl_table trees. */
+/**
+ * struct ctl_table_header - maintains dynamic lists of struct ctl_table trees
+ * @ctl_table: pointer to the first element in ctl_table array
+ * @ctl_table_size: number of elements pointed by @ctl_table
+ * @used: The entry will never be touched when equal to 0.
+ * @count: Upped every time something is added to @inodes and downed every time
+ *         something is removed from inodes
+ * @nreg: When nreg drops to 0 the ctl_table_header will be unregistered.
+ * @rcu: Delays the freeing of the inode. Introduced with "unfuck proc_sysctl ->d_compare()"
+ *
+ */
 struct ctl_table_header {
 	union {
 		struct {
 			struct ctl_table *ctl_table;
+			int ctl_table_size;
 			int used;
 			int count;
 			int nreg;
-- 
2.30.2

Re: [PATCH 03/14] sysctl: Add ctl_table_size to ctl_table_header

[Simon Horman]

On Wed, Jul 26, 2023 at 04:06:23PM +0200, Joel Granados wrote:
> The new ctl_table_size element will hold the size of the ctl_table
> contained in the header. This value is passed by the callers to the
> sysctl register infrastructure.
> 
> This is a preparation commit that allows us to systematically add
> ctl_table_size and start using it only when it is in all the places
> where there is a sysctl registration.
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>  include/linux/sysctl.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 59d451f455bf..33252ad58ebe 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -159,12 +159,22 @@ struct ctl_node {
>  	struct ctl_table_header *header;
>  };
>  
> -/* struct ctl_table_header is used to maintain dynamic lists of
> -   struct ctl_table trees. */
> +/**
> + * struct ctl_table_header - maintains dynamic lists of struct ctl_table trees
> + * @ctl_table: pointer to the first element in ctl_table array
> + * @ctl_table_size: number of elements pointed by @ctl_table
> + * @used: The entry will never be touched when equal to 0.
> + * @count: Upped every time something is added to @inodes and downed every time
> + *         something is removed from inodes
> + * @nreg: When nreg drops to 0 the ctl_table_header will be unregistered.
> + * @rcu: Delays the freeing of the inode. Introduced with "unfuck proc_sysctl ->d_compare()"
> + *
> + */

Please consider documenting all fields of struct ctl_table_header.
./scripts/kernel-doc complains that the following are missing:

  unregistering
  ctl_table_arg
  root
  set
  parent
  node
  inodes

Re: [PATCH 03/14] sysctl: Add ctl_table_size to ctl_table_header

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]

On Fri, Jul 28, 2023 at 12:48:31PM +0200, Simon Horman wrote:
> On Wed, Jul 26, 2023 at 04:06:23PM +0200, Joel Granados wrote:
> > The new ctl_table_size element will hold the size of the ctl_table
> > contained in the header. This value is passed by the callers to the
> > sysctl register infrastructure.
> > 
> > This is a preparation commit that allows us to systematically add
> > ctl_table_size and start using it only when it is in all the places
> > where there is a sysctl registration.
> > 
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  include/linux/sysctl.h | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> > index 59d451f455bf..33252ad58ebe 100644
> > --- a/include/linux/sysctl.h
> > +++ b/include/linux/sysctl.h
> > @@ -159,12 +159,22 @@ struct ctl_node {
> >  	struct ctl_table_header *header;
> >  };
> >  
> > -/* struct ctl_table_header is used to maintain dynamic lists of
> > -   struct ctl_table trees. */
> > +/**
> > + * struct ctl_table_header - maintains dynamic lists of struct ctl_table trees
> > + * @ctl_table: pointer to the first element in ctl_table array
> > + * @ctl_table_size: number of elements pointed by @ctl_table
> > + * @used: The entry will never be touched when equal to 0.
> > + * @count: Upped every time something is added to @inodes and downed every time
> > + *         something is removed from inodes
> > + * @nreg: When nreg drops to 0 the ctl_table_header will be unregistered.
> > + * @rcu: Delays the freeing of the inode. Introduced with "unfuck proc_sysctl ->d_compare()"
> > + *
> > + */
> 
> Please consider documenting all fields of struct ctl_table_header.
> ./scripts/kernel-doc complains that the following are missing:
> 
>   unregistering
>   ctl_table_arg
>   root
>   set
>   parent
>   node
>   inodes

This one I'm unsure about as things go in and then get changed without updating the docs
I have tried to follow the changes from the point of introduction, but as I said, I'm
unsure if I missed something.

diff --git i/include/linux/sysctl.h w/include/linux/sysctl.h
index 09d7429d67c0..fc0461f2a0c8 100644
--- i/include/linux/sysctl.h
+++ w/include/linux/sysctl.h
@@ -168,7 +168,13 @@ struct ctl_node {
  *         something is removed from inodes
  * @nreg: When nreg drops to 0 the ctl_table_header will be unregistered.
  * @rcu: Delays the freeing of the inode. Introduced with "unfuck proc_sysctl ->d_compare()"
- *
+ * @unregistering: Holds the completion when dropping (un-registering) a ctl_table
+ * @ctl_table_arg: The ctl_table array that was passed to register_sysctl_paths
+ * @root: The root of a sysctl namespace
+ * @set: Set of sysctls
+ * @parent: Pointer to the ctl_dir of the parent directory
+ * @node: Pointer to the rbtree node for this header
+ * @inodes: head for proc_inode->sysctl_inodes
  */
 struct ctl_table_header {
        union {
@@ -187,7 +193,7 @@ struct ctl_table_header {
        struct ctl_table_set *set;
        struct ctl_dir *parent;
        struct ctl_node *node;
-       struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */
+       struct hlist_head inodes;
 };

 struct ctl_dir {
~
-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 04/14] sysctl: Add size argument to init_header

[Joel Granados]

In this commit, the size of the ctl_table array is passed to initialize
the ctl_table_size element in the ctl_table_header struct. Although
ctl_table_size is not currently used, this step prepares us for when we
begin traversing the ctl_table array with it. In __register_sysctl_table
we use a calculated size until we add the size argument to that
function.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 fs/proc/proc_sysctl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 884460b0385b..fa1438f1a355 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -188,9 +188,10 @@ static void erase_entry(struct ctl_table_header *head, struct ctl_table *entry)
 
 static void init_header(struct ctl_table_header *head,
 	struct ctl_table_root *root, struct ctl_table_set *set,
-	struct ctl_node *node, struct ctl_table *table)
+	struct ctl_node *node, struct ctl_table *table, size_t table_size)
 {
 	head->ctl_table = table;
+	head->ctl_table_size = table_size;
 	head->ctl_table_arg = table;
 	head->used = 0;
 	head->count = 1;
@@ -973,7 +974,7 @@ static struct ctl_dir *new_dir(struct ctl_table_set *set,
 	memcpy(new_name, name, namelen);
 	table[0].procname = new_name;
 	table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO;
-	init_header(&new->header, set->dir.header.root, set, node, table);
+	init_header(&new->header, set->dir.header.root, set, node, table, 1);
 
 	return new;
 }
@@ -1197,7 +1198,8 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_
 		link_name += len;
 		link++;
 	}
-	init_header(links, dir->header.root, dir->header.set, node, link_table);
+	init_header(links, dir->header.root, dir->header.set, node, link_table,
+		    head->ctl_table_size);
 	links->nreg = nr_entries;
 
 	return links;
@@ -1372,7 +1374,7 @@ struct ctl_table_header *__register_sysctl_table(
 		return NULL;
 
 	node = (struct ctl_node *)(header + 1);
-	init_header(header, root, set, node, table);
+	init_header(header, root, set, node, table, nr_entries);
 	if (sysctl_check_table(path, header))
 		goto fail;
 
@@ -1537,7 +1539,7 @@ void setup_sysctl_set(struct ctl_table_set *set,
 {
 	memset(set, 0, sizeof(*set));
 	set->is_seen = is_seen;
-	init_header(&set->dir.header, root, set, NULL, root_table);
+	init_header(&set->dir.header, root, set, NULL, root_table, 1);
 }
 
 void retire_sysctl_set(struct ctl_table_set *set)
-- 
2.30.2

[PATCH 05/14] sysctl: Add a size arg to __register_sysctl_table

[Joel Granados]

This is part of the effort to remove the sentinel element in the
ctl_table arrays. We add a table_size argument to
__register_sysctl_table and adjust callers, all of which pass ctl_table
pointers and need an explicit call to ARRAY_SIZE.

The new table_size argument does not yet have any effect in the
init_header call which is still dependent on the sentinel's presence.
table_size *does* however drive the `kzalloc` allocation in
__register_sysctl_table with no adverse effects as the allocated memory
is either one element greater than the calculated ctl_table array (for
the calls in ipc_sysctl.c, mq_sysctl.c and ucount.c) or the exact size
of the calculated ctl_table array (for the call from sysctl_net.c and
register_sysctl). This approach will allows us to "just" remove the
sentinel without further changes to __register_sysctl_table as
table_size will represent the exact size for all the callers at that
point.

Temporarily implement a size calculation in register_net_sysctl, which
is an indirection call for all the network register calls.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 fs/proc/proc_sysctl.c  | 22 +++++++++++-----------
 include/linux/sysctl.h |  2 +-
 ipc/ipc_sysctl.c       |  4 +++-
 ipc/mq_sysctl.c        |  4 +++-
 kernel/ucount.c        |  3 ++-
 net/sysctl_net.c       |  8 +++++++-
 6 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index fa1438f1a355..8d04f01a89c1 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1354,27 +1354,20 @@ static struct ctl_dir *sysctl_mkdir_p(struct ctl_dir *dir, const char *path)
  */
 struct ctl_table_header *__register_sysctl_table(
 	struct ctl_table_set *set,
-	const char *path, struct ctl_table *table)
+	const char *path, struct ctl_table *table, size_t table_size)
 {
 	struct ctl_table_root *root = set->dir.header.root;
 	struct ctl_table_header *header;
-	struct ctl_table_header h_tmp;
 	struct ctl_dir *dir;
-	struct ctl_table *entry;
 	struct ctl_node *node;
-	int nr_entries = 0;
-
-	h_tmp.ctl_table = table;
-	list_for_each_table_entry(entry, (&h_tmp))
-		nr_entries++;
 
 	header = kzalloc(sizeof(struct ctl_table_header) +
-			 sizeof(struct ctl_node)*nr_entries, GFP_KERNEL_ACCOUNT);
+			 sizeof(struct ctl_node)*table_size, GFP_KERNEL_ACCOUNT);
 	if (!header)
 		return NULL;
 
 	node = (struct ctl_node *)(header + 1);
-	init_header(header, root, set, node, table, nr_entries);
+	init_header(header, root, set, node, table, table_size);
 	if (sysctl_check_table(path, header))
 		goto fail;
 
@@ -1423,8 +1416,15 @@ struct ctl_table_header *__register_sysctl_table(
  */
 struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table)
 {
+	int count = 0;
+	struct ctl_table *entry;
+	struct ctl_table_header t_hdr;
+
+	t_hdr.ctl_table = table;
+	list_for_each_table_entry(entry, (&t_hdr))
+		count++;
 	return __register_sysctl_table(&sysctl_table_root.default_set,
-					path, table);
+					path, table, count);
 }
 EXPORT_SYMBOL(register_sysctl);
 
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 33252ad58ebe..0495c858989f 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -226,7 +226,7 @@ extern void retire_sysctl_set(struct ctl_table_set *set);
 
 struct ctl_table_header *__register_sysctl_table(
 	struct ctl_table_set *set,
-	const char *path, struct ctl_table *table);
+	const char *path, struct ctl_table *table, size_t table_size);
 struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table);
 void unregister_sysctl_table(struct ctl_table_header * table);
 
diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index ef313ecfb53a..8c62e443f78b 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -259,7 +259,9 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 				tbl[i].data = NULL;
 		}
 
-		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set, "kernel", tbl);
+		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set,
+							  "kernel", tbl,
+							  ARRAY_SIZE(ipc_sysctls));
 	}
 	if (!ns->ipc_sysctls) {
 		kfree(tbl);
diff --git a/ipc/mq_sysctl.c b/ipc/mq_sysctl.c
index fbf6a8b93a26..ebb5ed81c151 100644
--- a/ipc/mq_sysctl.c
+++ b/ipc/mq_sysctl.c
@@ -109,7 +109,9 @@ bool setup_mq_sysctls(struct ipc_namespace *ns)
 				tbl[i].data = NULL;
 		}
 
-		ns->mq_sysctls = __register_sysctl_table(&ns->mq_set, "fs/mqueue", tbl);
+		ns->mq_sysctls = __register_sysctl_table(&ns->mq_set,
+							 "fs/mqueue", tbl,
+							 ARRAY_SIZE(mq_sysctls));
 	}
 	if (!ns->mq_sysctls) {
 		kfree(tbl);
diff --git a/kernel/ucount.c b/kernel/ucount.c
index ee8e57fd6f90..2b80264bb79f 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -104,7 +104,8 @@ bool setup_userns_sysctls(struct user_namespace *ns)
 		for (i = 0; i < UCOUNT_COUNTS; i++) {
 			tbl[i].data = &ns->ucount_max[i];
 		}
-		ns->sysctls = __register_sysctl_table(&ns->set, "user", tbl);
+		ns->sysctls = __register_sysctl_table(&ns->set, "user", tbl,
+						      ARRAY_SIZE(user_table));
 	}
 	if (!ns->sysctls) {
 		kfree(tbl);
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 4b45ed631eb8..8ee4b74bc009 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -163,10 +163,16 @@ static void ensure_safe_net_sysctl(struct net *net, const char *path,
 struct ctl_table_header *register_net_sysctl(struct net *net,
 	const char *path, struct ctl_table *table)
 {
+	int count = 0;
+	struct ctl_table *entry;
+
 	if (!net_eq(net, &init_net))
 		ensure_safe_net_sysctl(net, path, table);
 
-	return __register_sysctl_table(&net->sysctls, path, table);
+	for (entry = table; entry->procname; entry++)
+		count++;
+
+	return __register_sysctl_table(&net->sysctls, path, table, count);
 }
 EXPORT_SYMBOL_GPL(register_net_sysctl);
 
-- 
2.30.2

Re: [PATCH 05/14] sysctl: Add a size arg to __register_sysctl_table

[Simon Horman]

On Wed, Jul 26, 2023 at 04:06:25PM +0200, Joel Granados wrote:
> This is part of the effort to remove the sentinel element in the
> ctl_table arrays. We add a table_size argument to
> __register_sysctl_table and adjust callers, all of which pass ctl_table
> pointers and need an explicit call to ARRAY_SIZE.
> 
> The new table_size argument does not yet have any effect in the
> init_header call which is still dependent on the sentinel's presence.
> table_size *does* however drive the `kzalloc` allocation in
> __register_sysctl_table with no adverse effects as the allocated memory
> is either one element greater than the calculated ctl_table array (for
> the calls in ipc_sysctl.c, mq_sysctl.c and ucount.c) or the exact size
> of the calculated ctl_table array (for the call from sysctl_net.c and
> register_sysctl). This approach will allows us to "just" remove the
> sentinel without further changes to __register_sysctl_table as
> table_size will represent the exact size for all the callers at that
> point.
> 
> Temporarily implement a size calculation in register_net_sysctl, which
> is an indirection call for all the network register calls.
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>  fs/proc/proc_sysctl.c  | 22 +++++++++++-----------
>  include/linux/sysctl.h |  2 +-
>  ipc/ipc_sysctl.c       |  4 +++-
>  ipc/mq_sysctl.c        |  4 +++-
>  kernel/ucount.c        |  3 ++-
>  net/sysctl_net.c       |  8 +++++++-
>  6 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index fa1438f1a355..8d04f01a89c1 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1354,27 +1354,20 @@ static struct ctl_dir *sysctl_mkdir_p(struct ctl_dir *dir, const char *path)
>   */
>  struct ctl_table_header *__register_sysctl_table(
>  	struct ctl_table_set *set,
> -	const char *path, struct ctl_table *table)
> +	const char *path, struct ctl_table *table, size_t table_size)

Hi Joel,

Please consider adding table_size to the kernel doc for this function.

...

Re: [PATCH 05/14] sysctl: Add a size arg to __register_sysctl_table

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

On Fri, Jul 28, 2023 at 12:51:22PM +0200, Simon Horman wrote:
> On Wed, Jul 26, 2023 at 04:06:25PM +0200, Joel Granados wrote:
> > This is part of the effort to remove the sentinel element in the
> > ctl_table arrays. We add a table_size argument to
> > __register_sysctl_table and adjust callers, all of which pass ctl_table
> > pointers and need an explicit call to ARRAY_SIZE.
> > 
> > The new table_size argument does not yet have any effect in the
> > init_header call which is still dependent on the sentinel's presence.
> > table_size *does* however drive the `kzalloc` allocation in
> > __register_sysctl_table with no adverse effects as the allocated memory
> > is either one element greater than the calculated ctl_table array (for
> > the calls in ipc_sysctl.c, mq_sysctl.c and ucount.c) or the exact size
> > of the calculated ctl_table array (for the call from sysctl_net.c and
> > register_sysctl). This approach will allows us to "just" remove the
> > sentinel without further changes to __register_sysctl_table as
> > table_size will represent the exact size for all the callers at that
> > point.
> > 
> > Temporarily implement a size calculation in register_net_sysctl, which
> > is an indirection call for all the network register calls.
> > 
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  fs/proc/proc_sysctl.c  | 22 +++++++++++-----------
> >  include/linux/sysctl.h |  2 +-
> >  ipc/ipc_sysctl.c       |  4 +++-
> >  ipc/mq_sysctl.c        |  4 +++-
> >  kernel/ucount.c        |  3 ++-
> >  net/sysctl_net.c       |  8 +++++++-
> >  6 files changed, 27 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> > index fa1438f1a355..8d04f01a89c1 100644
> > --- a/fs/proc/proc_sysctl.c
> > +++ b/fs/proc/proc_sysctl.c
> > @@ -1354,27 +1354,20 @@ static struct ctl_dir *sysctl_mkdir_p(struct ctl_dir *dir, const char *path)
> >   */
> >  struct ctl_table_header *__register_sysctl_table(
> >  	struct ctl_table_set *set,
> > -	const char *path, struct ctl_table *table)
> > +	const char *path, struct ctl_table *table, size_t table_size)
> 
> Hi Joel,
> 
> Please consider adding table_size to the kernel doc for this function.
Good catch. Will do for V2.
> 
> ...

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 06/14] sysctl: Add size to register_sysctl

[Joel Granados]

In order to remove the end element from the ctl_table struct arrays, we
replace the register_syctl function with a macro that will add the
ARRAY_SIZE to the new register_sysctl_sz function. In this way the
callers that are already using an array of ctl_table structs do not have
to change. We *do* change the callers that pass the ctl_table array as a
pointer.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 arch/arm64/kernel/armv8_deprecated.c |  2 +-
 arch/s390/appldata/appldata_base.c   |  2 +-
 fs/proc/proc_sysctl.c                | 30 +++++++++++++++-------------
 include/linux/sysctl.h               | 10 ++++++++--
 kernel/ucount.c                      |  2 +-
 net/sysctl_net.c                     |  2 +-
 6 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 1febd412b4d2..e459cfd33711 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -569,7 +569,7 @@ static void __init register_insn_emulation(struct insn_emulation *insn)
 		sysctl->extra2 = &insn->max;
 		sysctl->proc_handler = emulation_proc_handler;
 
-		register_sysctl("abi", sysctl);
+		register_sysctl_sz("abi", sysctl, 1);
 	}
 }
 
diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index bbefe5e86bdf..3b0994625652 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -365,7 +365,7 @@ int appldata_register_ops(struct appldata_ops *ops)
 	ops->ctl_table[0].proc_handler = appldata_generic_handler;
 	ops->ctl_table[0].data = ops;
 
-	ops->sysctl_header = register_sysctl(appldata_proc_name, ops->ctl_table);
+	ops->sysctl_header = register_sysctl_sz(appldata_proc_name, ops->ctl_table, 1);
 	if (!ops->sysctl_header)
 		goto out;
 	return 0;
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8d04f01a89c1..c04293911e7e 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -43,7 +43,7 @@ static struct ctl_table sysctl_mount_point[] = {
  */
 struct ctl_table_header *register_sysctl_mount_point(const char *path)
 {
-	return register_sysctl(path, sysctl_mount_point);
+	return register_sysctl_sz(path, sysctl_mount_point, 0);
 }
 EXPORT_SYMBOL(register_sysctl_mount_point);
 
@@ -1398,7 +1398,7 @@ struct ctl_table_header *__register_sysctl_table(
 }
 
 /**
- * register_sysctl - register a sysctl table
+ * register_sysctl_sz - register a sysctl table
  * @path: The path to the directory the sysctl table is in. If the path
  * 	doesn't exist we will create it for you.
  * @table: the table structure. The calller must ensure the life of the @table
@@ -1408,25 +1408,20 @@ struct ctl_table_header *__register_sysctl_table(
  * 	to call unregister_sysctl_table() and can instead use something like
  * 	register_sysctl_init() which does not care for the result of the syctl
  * 	registration.
+ * @table_size: The number of elements in table.
  *
  * Register a sysctl table. @table should be a filled in ctl_table
  * array. A completely 0 filled entry terminates the table.
  *
  * See __register_sysctl_table for more details.
  */
-struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table)
+struct ctl_table_header *register_sysctl_sz(const char *path, struct ctl_table *table,
+					    size_t table_size)
 {
-	int count = 0;
-	struct ctl_table *entry;
-	struct ctl_table_header t_hdr;
-
-	t_hdr.ctl_table = table;
-	list_for_each_table_entry(entry, (&t_hdr))
-		count++;
 	return __register_sysctl_table(&sysctl_table_root.default_set,
-					path, table, count);
+					path, table, table_size);
 }
-EXPORT_SYMBOL(register_sysctl);
+EXPORT_SYMBOL(register_sysctl_sz);
 
 /**
  * __register_sysctl_init() - register sysctl table to path
@@ -1451,10 +1446,17 @@ EXPORT_SYMBOL(register_sysctl);
 void __init __register_sysctl_init(const char *path, struct ctl_table *table,
 				 const char *table_name)
 {
-	struct ctl_table_header *hdr = register_sysctl(path, table);
+	int count = 0;
+	struct ctl_table *entry;
+	struct ctl_table_header t_hdr, *hdr;
+
+	t_hdr.ctl_table = table;
+	list_for_each_table_entry(entry, (&t_hdr))
+		count++;
+	hdr = register_sysctl_sz(path, table, count);
 
 	if (unlikely(!hdr)) {
-		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
+		pr_err("failed when register_sysctl_sz %s to %s\n", table_name, path);
 		return;
 	}
 	kmemleak_not_leak(hdr);
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 0495c858989f..b1168ae281c9 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -215,6 +215,9 @@ struct ctl_path {
 	const char *procname;
 };
 
+#define register_sysctl(path, table)	\
+	register_sysctl_sz(path, table, ARRAY_SIZE(table))
+
 #ifdef CONFIG_SYSCTL
 
 void proc_sys_poll_notify(struct ctl_table_poll *poll);
@@ -227,7 +230,8 @@ extern void retire_sysctl_set(struct ctl_table_set *set);
 struct ctl_table_header *__register_sysctl_table(
 	struct ctl_table_set *set,
 	const char *path, struct ctl_table *table, size_t table_size);
-struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table);
+struct ctl_table_header *register_sysctl_sz(const char *path, struct ctl_table *table,
+					    size_t table_size);
 void unregister_sysctl_table(struct ctl_table_header * table);
 
 extern int sysctl_init_bases(void);
@@ -262,7 +266,9 @@ static inline struct ctl_table_header *register_sysctl_mount_point(const char *p
 	return NULL;
 }
 
-static inline struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table)
+static inline struct ctl_table_header *register_sysctl_sz(const char *path,
+							  struct ctl_table *table,
+							  size_t table_size)
 {
 	return NULL;
 }
diff --git a/kernel/ucount.c b/kernel/ucount.c
index 2b80264bb79f..4aa6166cb856 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -365,7 +365,7 @@ static __init int user_namespace_sysctl_init(void)
 	 * default set so that registrations in the child sets work
 	 * properly.
 	 */
-	user_header = register_sysctl("user", empty);
+	user_header = register_sysctl_sz("user", empty, 0);
 	kmemleak_ignore(user_header);
 	BUG_ON(!user_header);
 	BUG_ON(!setup_userns_sysctls(&init_user_ns));
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 8ee4b74bc009..d9cbbb51b143 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -101,7 +101,7 @@ __init int net_sysctl_init(void)
 	 * registering "/proc/sys/net" as an empty directory not in a
 	 * network namespace.
 	 */
-	net_header = register_sysctl("net", empty);
+	net_header = register_sysctl_sz("net", empty, 0);
 	if (!net_header)
 		goto out;
 	ret = register_pernet_subsys(&sysctl_pernet_ops);
-- 
2.30.2

Re: [PATCH 06/14] sysctl: Add size to register_sysctl

[Luis Chamberlain]

On Wed, Jul 26, 2023 at 04:06:26PM +0200, Joel Granados wrote:
> In order to remove the end element from the ctl_table struct arrays, we
> replace the register_syctl function with a macro that will add the
> ARRAY_SIZE to the new register_sysctl_sz function. In this way the
> callers that are already using an array of ctl_table structs do not have
> to change. We *do* change the callers that pass the ctl_table array as a
> pointer.

Thanks for doing this and this series!

> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 0495c858989f..b1168ae281c9 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -215,6 +215,9 @@ struct ctl_path {
>  	const char *procname;
>  };
>  
> +#define register_sysctl(path, table)	\
> +	register_sysctl_sz(path, table, ARRAY_SIZE(table))
> +
>  #ifdef CONFIG_SYSCTL

Wasn't it Greg who had suggested this? Maybe add Suggested-by with him
on it.

Also, your cover letter and first few patches are not CC'd to the netdev
list or others. What you want to do is collect all the email addresses
for this small patch series and add them to who you email for your
entire series, otherwise at times they won't be able to properly review
or understand the exact context of the changes. You want folks to do less
work to review, not more.

So please resend and add others to the other patches.

  Luis

Re: [PATCH 06/14] sysctl: Add size to register_sysctl

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

On Wed, Jul 26, 2023 at 10:58:30AM -0700, Luis Chamberlain wrote:
> On Wed, Jul 26, 2023 at 04:06:26PM +0200, Joel Granados wrote:
> > In order to remove the end element from the ctl_table struct arrays, we
> > replace the register_syctl function with a macro that will add the
> > ARRAY_SIZE to the new register_sysctl_sz function. In this way the
> > callers that are already using an array of ctl_table structs do not have
> > to change. We *do* change the callers that pass the ctl_table array as a
> > pointer.
> 
> Thanks for doing this and this series!
> 
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> > index 0495c858989f..b1168ae281c9 100644
> > --- a/include/linux/sysctl.h
> > +++ b/include/linux/sysctl.h
> > @@ -215,6 +215,9 @@ struct ctl_path {
> >  	const char *procname;
> >  };
> >  
> > +#define register_sysctl(path, table)	\
> > +	register_sysctl_sz(path, table, ARRAY_SIZE(table))
> > +
> >  #ifdef CONFIG_SYSCTL
> 
> Wasn't it Greg who had suggested this? Maybe add Suggested-by with him
> on it.
Yes. I mentioned him in the cover letter and did not add the tag because
I had not asked for permission to use it. I'll drop him a mail and
include the suggested-by if he agrees.

> 
> Also, your cover letter and first few patches are not CC'd to the netdev
> list or others. What you want to do is collect all the email addresses
> for this small patch series and add them to who you email for your
> entire series, otherwise at times they won't be able to properly review
> or understand the exact context of the changes. You want folks to do less
> work to review, not more.
Here I wanted to avoid very big e-mail headers as I have received
rejections from lists in the past. But I for this set, the number of
e-mails is ok to just include everyone.

I'll do that for V2.
thx for your feedback

best

> 
> So please resend and add others to the other patches.
> 
>   Luis

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 06/14] sysctl: Add size to register_sysctl

[Luis Chamberlain]

On Thu, Jul 27, 2023 at 02:22:00PM +0200, Joel Granados wrote:
> On Wed, Jul 26, 2023 at 10:58:30AM -0700, Luis Chamberlain wrote:
> > On Wed, Jul 26, 2023 at 04:06:26PM +0200, Joel Granados wrote:
> > > In order to remove the end element from the ctl_table struct arrays, we
> > > replace the register_syctl function with a macro that will add the
> > > ARRAY_SIZE to the new register_sysctl_sz function. In this way the
> > > callers that are already using an array of ctl_table structs do not have
> > > to change. We *do* change the callers that pass the ctl_table array as a
> > > pointer.
> > 
> > Thanks for doing this and this series!
> > 
> > > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > > ---
> > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> > > index 0495c858989f..b1168ae281c9 100644
> > > --- a/include/linux/sysctl.h
> > > +++ b/include/linux/sysctl.h
> > > @@ -215,6 +215,9 @@ struct ctl_path {
> > >  	const char *procname;
> > >  };
> > >  
> > > +#define register_sysctl(path, table)	\
> > > +	register_sysctl_sz(path, table, ARRAY_SIZE(table))
> > > +
> > >  #ifdef CONFIG_SYSCTL
> > 
> > Wasn't it Greg who had suggested this? Maybe add Suggested-by with him
> > on it.
> Yes. I mentioned him in the cover letter and did not add the tag because
> I had not asked for permission to use it. I'll drop him a mail and
> include the suggested-by if he agrees.

FWIW, I never ask, if they ask for it, clearly they suggested it.

> > Also, your cover letter and first few patches are not CC'd to the netdev
> > list or others. What you want to do is collect all the email addresses
> > for this small patch series and add them to who you email for your
> > entire series, otherwise at times they won't be able to properly review
> > or understand the exact context of the changes. You want folks to do less
> > work to review, not more.
> Here I wanted to avoid very big e-mail headers as I have received
> rejections from lists in the past. But I for this set, the number of
> e-mails is ok to just include everyone.

I hear that from time to time, if you have issues with adding folks on
the To address it may be an SMTP server issue, ie, corp email SMTP
server issues. To fix that I avoid corp email SMTP servers.

  Luis

Re: [PATCH 06/14] sysctl: Add size to register_sysctl

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On Thu, Jul 27, 2023 at 08:42:02AM -0700, Luis Chamberlain wrote:
> On Thu, Jul 27, 2023 at 02:22:00PM +0200, Joel Granados wrote:
> > On Wed, Jul 26, 2023 at 10:58:30AM -0700, Luis Chamberlain wrote:
> > > On Wed, Jul 26, 2023 at 04:06:26PM +0200, Joel Granados wrote:
> > > > In order to remove the end element from the ctl_table struct arrays, we
> > > > replace the register_syctl function with a macro that will add the
> > > > ARRAY_SIZE to the new register_sysctl_sz function. In this way the
> > > > callers that are already using an array of ctl_table structs do not have
> > > > to change. We *do* change the callers that pass the ctl_table array as a
> > > > pointer.
> > > 
> > > Thanks for doing this and this series!
> > > 
> > > > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > > > ---
> > > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> > > > index 0495c858989f..b1168ae281c9 100644
> > > > --- a/include/linux/sysctl.h
> > > > +++ b/include/linux/sysctl.h
> > > > @@ -215,6 +215,9 @@ struct ctl_path {
> > > >  	const char *procname;
> > > >  };
> > > >  
> > > > +#define register_sysctl(path, table)	\
> > > > +	register_sysctl_sz(path, table, ARRAY_SIZE(table))
> > > > +
> > > >  #ifdef CONFIG_SYSCTL
> > > 
> > > Wasn't it Greg who had suggested this? Maybe add Suggested-by with him
> > > on it.
> > Yes. I mentioned him in the cover letter and did not add the tag because
> > I had not asked for permission to use it. I'll drop him a mail and
> > include the suggested-by if he agrees.
> 
> FWIW, I never ask, if they ask for it, clearly they suggested it.
I was following Documentation/process/submitting-patches.rst:
"... Please note that this tag should not be added without the
reporter's permission... ".
In any case, Greg has already said yes :)

> 
> > > Also, your cover letter and first few patches are not CC'd to the netdev
> > > list or others. What you want to do is collect all the email addresses
> > > for this small patch series and add them to who you email for your
> > > entire series, otherwise at times they won't be able to properly review
> > > or understand the exact context of the changes. You want folks to do less
> > > work to review, not more.
> > Here I wanted to avoid very big e-mail headers as I have received
> > rejections from lists in the past. But I for this set, the number of
> > e-mails is ok to just include everyone.
> 
> I hear that from time to time, if you have issues with adding folks on
> the To address it may be an SMTP server issue, ie, corp email SMTP
> server issues. To fix that I avoid corp email SMTP servers.
My experience was more from the lists rejecting the e-mail because the
header was too big. With that said, I'll look into SMTP alternatives to
reduce possible errors

> 
>   Luis

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 07/14] sysctl: Add size arg to __register_sysctl_init

[Joel Granados]

This is part of the effort to remove the sentinel element from the
ctl_table array at register time. We add a size argument to
__register_sysctl_init and modify the register_sysctl_init macro to
calculate the array size with ARRAY_SIZE. The original callers do not
need to be updated as they will go through the new macro.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 fs/proc/proc_sysctl.c  | 11 ++---------
 include/linux/sysctl.h |  5 +++--
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index c04293911e7e..6c0721cd35f3 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1444,16 +1444,9 @@ EXPORT_SYMBOL(register_sysctl_sz);
  * Context: if your base directory does not exist it will be created for you.
  */
 void __init __register_sysctl_init(const char *path, struct ctl_table *table,
-				 const char *table_name)
+				 const char *table_name, size_t table_size)
 {
-	int count = 0;
-	struct ctl_table *entry;
-	struct ctl_table_header t_hdr, *hdr;
-
-	t_hdr.ctl_table = table;
-	list_for_each_table_entry(entry, (&t_hdr))
-		count++;
-	hdr = register_sysctl_sz(path, table, count);
+	struct ctl_table_header *hdr = register_sysctl_sz(path, table, table_size);
 
 	if (unlikely(!hdr)) {
 		pr_err("failed when register_sysctl_sz %s to %s\n", table_name, path);
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index b1168ae281c9..09d7429d67c0 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -236,8 +236,9 @@ void unregister_sysctl_table(struct ctl_table_header * table);
 
 extern int sysctl_init_bases(void);
 extern void __register_sysctl_init(const char *path, struct ctl_table *table,
-				 const char *table_name);
-#define register_sysctl_init(path, table) __register_sysctl_init(path, table, #table)
+				 const char *table_name, size_t table_size);
+#define register_sysctl_init(path, table)	\
+	__register_sysctl_init(path, table, #table, ARRAY_SIZE(table))
 extern struct ctl_table_header *register_sysctl_mount_point(const char *path);
 
 void do_sysctl_args(void);
-- 
2.30.2

Re: [PATCH 07/14] sysctl: Add size arg to __register_sysctl_init

[Simon Horman]

On Wed, Jul 26, 2023 at 04:06:27PM +0200, Joel Granados wrote:
> This is part of the effort to remove the sentinel element from the
> ctl_table array at register time. We add a size argument to
> __register_sysctl_init and modify the register_sysctl_init macro to
> calculate the array size with ARRAY_SIZE. The original callers do not
> need to be updated as they will go through the new macro.
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>
> ---
>  fs/proc/proc_sysctl.c  | 11 ++---------
>  include/linux/sysctl.h |  5 +++--
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index c04293911e7e..6c0721cd35f3 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1444,16 +1444,9 @@ EXPORT_SYMBOL(register_sysctl_sz);
>   * Context: if your base directory does not exist it will be created for you.
>   */
>  void __init __register_sysctl_init(const char *path, struct ctl_table *table,
> -				 const char *table_name)
> +				 const char *table_name, size_t table_size)

Hi Joel,

in the same vein as my comment on another patch.
Please add table_size to the kernel doc for this function.

Re: [PATCH 07/14] sysctl: Add size arg to __register_sysctl_init

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Fri, Jul 28, 2023 at 12:56:36PM +0200, Simon Horman wrote:
> On Wed, Jul 26, 2023 at 04:06:27PM +0200, Joel Granados wrote:
> > This is part of the effort to remove the sentinel element from the
> > ctl_table array at register time. We add a size argument to
> > __register_sysctl_init and modify the register_sysctl_init macro to
> > calculate the array size with ARRAY_SIZE. The original callers do not
> > need to be updated as they will go through the new macro.
> > 
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> > ---
> >  fs/proc/proc_sysctl.c  | 11 ++---------
> >  include/linux/sysctl.h |  5 +++--
> >  2 files changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> > index c04293911e7e..6c0721cd35f3 100644
> > --- a/fs/proc/proc_sysctl.c
> > +++ b/fs/proc/proc_sysctl.c
> > @@ -1444,16 +1444,9 @@ EXPORT_SYMBOL(register_sysctl_sz);
> >   * Context: if your base directory does not exist it will be created for you.
> >   */
> >  void __init __register_sysctl_init(const char *path, struct ctl_table *table,
> > -				 const char *table_name)
> > +				 const char *table_name, size_t table_size)
> 
> Hi Joel,
> 
> in the same vein as my comment on another patch.
> Please add table_size to the kernel doc for this function.
Will do.

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 14/14] sysctl: Use size as stopping criteria for list macro

[Joel Granados]

Add header->ctl_table_size as an additional stopping criteria for the
list_for_each_table_entry macro. In this way it will execute until it
finds an "empty" ->procname or until the size runs out. Therefore if a
ctl_table array with a sentinel is passed its size will be too big (by
one element) but it will stop on the sentinel. On the other hand, if the
ctl_table array without a sentinel is passed its size will be just write
and there will be no need for a sentinel.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 fs/proc/proc_sysctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 6c0721cd35f3..3eea34d98d54 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -19,8 +19,9 @@
 #include <linux/kmemleak.h>
 #include "internal.h"
 
-#define list_for_each_table_entry(entry, header) \
-	for ((entry) = (header->ctl_table); (entry)->procname; (entry)++)
+#define list_for_each_table_entry(entry, header)	\
+	entry = header->ctl_table;			\
+	for (size_t i = 0 ; i < header->ctl_table_size && entry->procname; ++i, entry++)
 
 static const struct dentry_operations proc_sys_dentry_operations;
 static const struct file_operations proc_sys_file_operations;
-- 
2.30.2

Re: [PATCH 00/14] sysctl: Add a size argument to register functions in sysctl

[Luis Chamberlain]

On Wed, Jul 26, 2023 at 04:06:20PM +0200, Joel Granados wrote:
> What?
> These commits set things up so we can start removing the sentinel elements.

Yes but the why must explained right away.

> Why?
> This is part of the push to trim down kernel/sysctl.c by moving the large array
> that was causing merge conflicts. 

Let me elaborate on that:

While the move moving over time of array elements out of kernel/sysctl.c
to their own place helps merge conflicts this patch set does not help
with that in and of itself, what it does is help make sure the move of
sysctls to their own files does not bloat the kernel more, and in fact
helps reduce the overall build time size of the kernel and run time
memory consumed by the kernel by about ~64 bytes per array.

Without this patch set each time we moved a set of sysctls out of
kernel/sysctl.c to its own subsystem we'd have to add a new sentinel
element (an empty sysctl entry), and while that helps clean up
kernel/sysctl.c to avoid merge conflicts, it also bloats the kernel
by about 64 bytes on average each time.

We can do better. We can make those moves *not* have a size penalty, and
all around also reduce the build / run time of the kernel.

*This* is the why, that if we don't do this the cleanup of
kernel/sysctl.c ends up slowly bloating the kernel. Willy had
suggested we instead remove the sentinel so that each move does not
incur a size penalty, but also that in turn reduces the size of the
kernel at build time / run time by a ballpark about ~64 bytes per
array.

Then the following is more details about estimates of overall size
savings, it's not miscellaneous information at all, it's very relevant
information to this patch set.

> Misc:
> A consequence of eventually removing all the sentinels (64 bytes per sentinel)
> is the bytes we save. Here I include numbers for when all sentinels are removed
> to contextualize this chunk
>   * bloat-o-meter:
>     The "yesall" configuration results save 9158 bytes (you can see the output here
>     https://lore.kernel.org/all/20230621091000.424843-1-j.granados@samsung.com/.
>     The "tiny" configuration + CONFIG_SYSCTL save 1215 bytes (you can see the
>     output here [2])
>   * memory usage:
>     As we no longer need the sentinel element within proc_sysctl.c, we save some
>     bytes in main memory as well. In my testing kernel I measured a difference of
>     6720 bytes. I include the way to measure this in [1]

  Luis

Re: [PATCH 00/14] sysctl: Add a size argument to register functions in sysctl

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Wed, Jul 26, 2023 at 11:15:40AM -0700, Luis Chamberlain wrote:
> On Wed, Jul 26, 2023 at 04:06:20PM +0200, Joel Granados wrote:
> > What?
> > These commits set things up so we can start removing the sentinel elements.
> 
> Yes but the why must explained right away.
My intention of putting the "what" first was to explain the chunking and
clarify right away that what was contained in the "why" was not for this
chunk but for the *whole* patchset.

I have swapped them in my cover letter as I can see that the ambiguity
is gone once you start reading the "what"

> 
> > Why?
> > This is part of the push to trim down kernel/sysctl.c by moving the large array
> > that was causing merge conflicts. 
> 
> Let me elaborate on that:
> 
> While the move moving over time of array elements out of kernel/sysctl.c
> to their own place helps merge conflicts this patch set does not help
> with that in and of itself, what it does is help make sure the move of
> sysctls to their own files does not bloat the kernel more, and in fact
> helps reduce the overall build time size of the kernel and run time
> memory consumed by the kernel by about ~64 bytes per array.
> 
> Without this patch set each time we moved a set of sysctls out of
> kernel/sysctl.c to its own subsystem we'd have to add a new sentinel
> element (an empty sysctl entry), and while that helps clean up
> kernel/sysctl.c to avoid merge conflicts, it also bloats the kernel
> by about 64 bytes on average each time.
> 
> We can do better. We can make those moves *not* have a size penalty, and
> all around also reduce the build / run time of the kernel.
> 
> *This* is the why, that if we don't do this the cleanup of
> kernel/sysctl.c ends up slowly bloating the kernel. Willy had
> suggested we instead remove the sentinel so that each move does not
> incur a size penalty, but also that in turn reduces the size of the
> kernel at build time / run time by a ballpark about ~64 bytes per
> array.
Thx for this.
This is a more clear wording for the "Why". Do you mind if I copy/paste
it (with some changes to make it flow) into my next cover letter?

> 
> Then the following is more details about estimates of overall size
> savings, it's not miscellaneous information at all, it's very relevant
> information to this patch set.
Did not mean to downplay the importance here. Just did not have a good
title for the section. I'll change it to "Size saving estimates".
> 
> > Misc:
> > A consequence of eventually removing all the sentinels (64 bytes per sentinel)
> > is the bytes we save. Here I include numbers for when all sentinels are removed
> > to contextualize this chunk
> >   * bloat-o-meter:
> >     The "yesall" configuration results save 9158 bytes (you can see the output here
> >     https://lore.kernel.org/all/20230621091000.424843-1-j.granados@samsung.com/.
> >     The "tiny" configuration + CONFIG_SYSCTL save 1215 bytes (you can see the
> >     output here [2])
> >   * memory usage:
> >     As we no longer need the sentinel element within proc_sysctl.c, we save some
> >     bytes in main memory as well. In my testing kernel I measured a difference of
> >     6720 bytes. I include the way to measure this in [1]
> 
>   Luis

-- 

best

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/14] sysctl: Add a size argument to register functions in sysctl

[Luis Chamberlain]

On Thu, Jul 27, 2023 at 01:43:18PM +0200, Joel Granados wrote:
> On Wed, Jul 26, 2023 at 11:15:40AM -0700, Luis Chamberlain wrote:
> > On Wed, Jul 26, 2023 at 04:06:20PM +0200, Joel Granados wrote:
> > > What?
> > > These commits set things up so we can start removing the sentinel elements.
> > 
> > Yes but the why must explained right away.
> My intention of putting the "what" first was to explain the chunking and

It may help also just to clarify:

   sentinel, the extra empty struct ctl_table at the end of each
   sysctl array.

> Thx for this.
> This is a more clear wording for the "Why". Do you mind if I copy/paste
> it (with some changes to make it flow) into my next cover letter?

I don't mind at all.

  Luis

Re: [PATCH 00/14] sysctl: Add a size argument to register functions in sysctl

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Thu, Jul 27, 2023 at 08:39:59AM -0700, Luis Chamberlain wrote:
> On Thu, Jul 27, 2023 at 01:43:18PM +0200, Joel Granados wrote:
> > On Wed, Jul 26, 2023 at 11:15:40AM -0700, Luis Chamberlain wrote:
> > > On Wed, Jul 26, 2023 at 04:06:20PM +0200, Joel Granados wrote:
> > > > What?
> > > > These commits set things up so we can start removing the sentinel elements.
> > > 
> > > Yes but the why must explained right away.
> > My intention of putting the "what" first was to explain the chunking and
> 
> It may help also just to clarify:
> 
>    sentinel, the extra empty struct ctl_table at the end of each
>    sysctl array.
This clarification is already there: "... sentinel element (the extra
empty element in the ctl_table arrays ...".

> 
> > Thx for this.
> > This is a more clear wording for the "Why". Do you mind if I copy/paste
> > it (with some changes to make it flow) into my next cover letter?
> 
> I don't mind at all.
> 
>   Luis

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]