| Message ID | 20220224093201.12440-1-tangmeng@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/proc: optimize exactly register one ctl_table | expand |
On Thu, Feb 24, 2022 at 05:32:01PM +0800, Meng Tang wrote: > Currently, sysctl is being moved to its own file. But ctl_table > is quite large(64 bytes per entry) and every array is terminated > with an empty one. This leads to thar when register exactly one > ctl_table, we've gone from 64 bytes to 128 bytes. How about: Sysctls are being moved out of kernel/sysctl.c and out to their own respective subsystems / users to help with easier maintance and avoid merge conflicts. But when we move just one entry and to its own new file the last entry for this new file must be empty, so we are essentialy bloating the kernel one extra empty entry per each newly moved sysctl. > So, it is obviously the right thing that we need to fix. > > In order to avoid compatibility problems, and to be compatible > with array terminated with an empty one and register exactly one > ctl_table, add the register_one variable in the ctl_table > structure to fix it. How about: To help with this, this adds support for registering just one ctl_table, therefore not bloating the kernel when we move a single ctl_table to its own file. > When we register exactly one table, we only need to add > "table->register = true" to avoid gone from 64 bytes to 128 bytes. Hmm, let me think about this.... > Signed-off-by: Meng Tang <tangmeng@uniontech.com> > --- > fs/proc/proc_sysctl.c | 58 +++++++++++++++++++++++++++++++++++++++--- > include/linux/sysctl.h | 1 + > 2 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 7d9cfc730bd4..9ecd5c87e8dd 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -215,16 +215,24 @@ static void init_header(struct ctl_table_header *head, > INIT_HLIST_HEAD(&head->inodes); > if (node) { > struct ctl_table *entry; > - for (entry = table; entry->procname; entry++, node++) > + for (entry = table; entry->procname; entry++, node++) { > node->header = head; > + > + if (entry->register_one) > + break; > + } > } > } Sure.. > static void erase_header(struct ctl_table_header *head) > { > struct ctl_table *entry; > - for (entry = head->ctl_table; entry->procname; entry++) > + for (entry = head->ctl_table; entry->procname; entry++) { > erase_entry(head, entry); > + > + if (entry->register_one) > + break; > + } > } Sure.. > > static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > @@ -252,6 +260,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) > err = insert_entry(header, entry); > if (err) > goto fail; > + > + if (entry->register_one) > + break; > } > return 0; > fail: > @@ -1159,6 +1170,9 @@ static int sysctl_check_table(const char *path, struct ctl_table *table) > if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode) > err |= sysctl_err(path, table, "bogus .mode 0%o", > table->mode); > + > + if (table->register_one) > + break; sure.. > } > return err; > } > @@ -1177,6 +1191,9 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table > for (entry = table; entry->procname; entry++) { > nr_entries++; > name_bytes += strlen(entry->procname) + 1; > + > + if (entry->register_one) > + break; sure.. > } > > links = kzalloc(sizeof(struct ctl_table_header) + > @@ -1199,6 +1216,9 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table > link->mode = S_IFLNK|S_IRWXUGO; > link->data = link_root; > link_name += len; > + > + if (entry->register_one) > + break; > } > init_header(links, dir->header.root, dir->header.set, node, link_table); > links->nreg = nr_entries; > @@ -1218,6 +1238,15 @@ static bool get_links(struct ctl_dir *dir, > link = find_entry(&head, dir, procname, strlen(procname)); > if (!link) > return false; > + > + if (entry->register_one) { > + if (S_ISDIR(link->mode) && S_ISDIR(entry->mode)) > + break; > + if (S_ISLNK(link->mode) && (link->data == link_root)) > + break; > + return false; > + } > + > if (S_ISDIR(link->mode) && S_ISDIR(entry->mode)) > continue; > if (S_ISLNK(link->mode) && (link->data == link_root)) > @@ -1230,6 +1259,8 @@ static bool get_links(struct ctl_dir *dir, > const char *procname = entry->procname; > link = find_entry(&head, dir, procname, strlen(procname)); > head->nreg++; > + if (entry->register_one) > + break; Etc... > } > return true; > } > @@ -1295,6 +1326,8 @@ static int insert_links(struct ctl_table_header *head) > * > * mode - the file permissions for the /proc/sys file > * > + * register_one - set to true when exactly register one ctl_table > + * > * child - must be %NULL. > * > * proc_handler - the text handler routine (described below) > @@ -1329,9 +1362,13 @@ struct ctl_table_header *__register_sysctl_table( > struct ctl_node *node; > int nr_entries = 0; > > - for (entry = table; entry->procname; entry++) > + for (entry = table; entry->procname; entry++) { > nr_entries++; > > + if (entry->register_one) > + break; > + } > + > header = kzalloc(sizeof(struct ctl_table_header) + > sizeof(struct ctl_node)*nr_entries, GFP_KERNEL); > if (!header) > @@ -1461,6 +1498,9 @@ static int count_subheaders(struct ctl_table *table) > nr_subheaders += count_subheaders(entry->child); > else > has_files = 1; > + > + if (entry->register_one) > + break; > } > return nr_subheaders + has_files; > } > @@ -1480,6 +1520,9 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, > nr_dirs++; > else > nr_files++; > + > + if (entry->register_one) > + break; > } > > files = table; > @@ -1497,6 +1540,9 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, > continue; > *new = *entry; > new++; > + > + if (entry->register_one) > + break; > } > } > > @@ -1532,6 +1578,9 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, > pos[0] = '\0'; > if (err) > goto out; > + > + if (entry->register_one) > + break; > } > err = 0; > out: > @@ -1686,6 +1735,9 @@ static void put_links(struct ctl_table_header *header) > sysctl_print_dir(parent); > pr_cont("%s\n", name); > } > + > + if (entry->register_one) > + break; > } > } > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index 6353d6db69b2..889c995d8a08 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -134,6 +134,7 @@ struct ctl_table { > void *data; > int maxlen; > umode_t mode; > + bool register_one; /* Exactly register one ctl_table*/ > struct ctl_table *child; /* Deprecated */ > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; This effort is trying to save space. But now you are adding a new bool for every single struct ctl_table.... So doesn't the math work out against us if you do a build size comparison? Can you just instead add a new helper which deals with one entry? Perhaps then make the other caller which loops use that? That way we don't bloat the kernel with an extra bool per ctl_table? Or can you add a new parameter which specififes the size of the array? Please provide vmlinux size comparisons before and after on allyesconfig builds. Luis
On Mon, Feb 28, 2022 at 10:42:19AM +0800, tangmeng wrote: > > > On 2022/2/28 05:26, Luis Chamberlain wrote: > > > > > This effort is trying to save space. But now you are adding a new bool > > for every single struct ctl_table.... So doesn't the math work out > > against us if you do a build size comparison? > > > Currently, You mean after your patch. > the definition of the ctl_table structure and the size of the > members are as follows. > /* In 64-bit system*/ > struct ctl_table { ... > bool register_one; /* 1 bytes */ ... > } __randomize_layout; > > Before Before it the bool was not there. How can it be you are not increasing the size? > > Can you just instead add a new helper which deals with one entry? > > Perhaps then make the other caller which loops use that? That way > > we don't bloat the kernel with an extra bool per ctl_table? > > > I have considered add a new helper which deals with one entry. But > considered that the code will be similar to array, That's fine, if we have tons of these. > > Or can you add a new parameter which specififes the size of the array? > > > When I considered add a new parameter which specififes the size of the > array. I have encountered the following difficulties. > > The current status is that during the ctl_table registration process, the > method of traversing the table is implemented by a movement of the pointer > entry pointing to the struct ctl_table. When entry->procname is empty, it > considers table traversal. > > This leads to that when the ctl_tables have child tables in the table, it is > not possible to get the child table's size by ARRAY_SIZE(*entry), so > transmitting the Child Table Size becomes very difficult. I see. A simple routine for dealing with single entries might be best then. And while at it, see if you can add a DECLARE_SYSCTL_SINGLE or something which will wrap up all the ugly stuff. Luis
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 7d9cfc730bd4..9ecd5c87e8dd 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -215,16 +215,24 @@ static void init_header(struct ctl_table_header *head, INIT_HLIST_HEAD(&head->inodes); if (node) { struct ctl_table *entry; - for (entry = table; entry->procname; entry++, node++) + for (entry = table; entry->procname; entry++, node++) { node->header = head; + + if (entry->register_one) + break; + } } } static void erase_header(struct ctl_table_header *head) { struct ctl_table *entry; - for (entry = head->ctl_table; entry->procname; entry++) + for (entry = head->ctl_table; entry->procname; entry++) { erase_entry(head, entry); + + if (entry->register_one) + break; + } } static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) @@ -252,6 +260,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header) err = insert_entry(header, entry); if (err) goto fail; + + if (entry->register_one) + break; } return 0; fail: @@ -1159,6 +1170,9 @@ static int sysctl_check_table(const char *path, struct ctl_table *table) if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode) err |= sysctl_err(path, table, "bogus .mode 0%o", table->mode); + + if (table->register_one) + break; } return err; } @@ -1177,6 +1191,9 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table for (entry = table; entry->procname; entry++) { nr_entries++; name_bytes += strlen(entry->procname) + 1; + + if (entry->register_one) + break; } links = kzalloc(sizeof(struct ctl_table_header) + @@ -1199,6 +1216,9 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table link->mode = S_IFLNK|S_IRWXUGO; link->data = link_root; link_name += len; + + if (entry->register_one) + break; } init_header(links, dir->header.root, dir->header.set, node, link_table); links->nreg = nr_entries; @@ -1218,6 +1238,15 @@ static bool get_links(struct ctl_dir *dir, link = find_entry(&head, dir, procname, strlen(procname)); if (!link) return false; + + if (entry->register_one) { + if (S_ISDIR(link->mode) && S_ISDIR(entry->mode)) + break; + if (S_ISLNK(link->mode) && (link->data == link_root)) + break; + return false; + } + if (S_ISDIR(link->mode) && S_ISDIR(entry->mode)) continue; if (S_ISLNK(link->mode) && (link->data == link_root)) @@ -1230,6 +1259,8 @@ static bool get_links(struct ctl_dir *dir, const char *procname = entry->procname; link = find_entry(&head, dir, procname, strlen(procname)); head->nreg++; + if (entry->register_one) + break; } return true; } @@ -1295,6 +1326,8 @@ static int insert_links(struct ctl_table_header *head) * * mode - the file permissions for the /proc/sys file * + * register_one - set to true when exactly register one ctl_table + * * child - must be %NULL. * * proc_handler - the text handler routine (described below) @@ -1329,9 +1362,13 @@ struct ctl_table_header *__register_sysctl_table( struct ctl_node *node; int nr_entries = 0; - for (entry = table; entry->procname; entry++) + for (entry = table; entry->procname; entry++) { nr_entries++; + if (entry->register_one) + break; + } + header = kzalloc(sizeof(struct ctl_table_header) + sizeof(struct ctl_node)*nr_entries, GFP_KERNEL); if (!header) @@ -1461,6 +1498,9 @@ static int count_subheaders(struct ctl_table *table) nr_subheaders += count_subheaders(entry->child); else has_files = 1; + + if (entry->register_one) + break; } return nr_subheaders + has_files; } @@ -1480,6 +1520,9 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, nr_dirs++; else nr_files++; + + if (entry->register_one) + break; } files = table; @@ -1497,6 +1540,9 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, continue; *new = *entry; new++; + + if (entry->register_one) + break; } } @@ -1532,6 +1578,9 @@ static int register_leaf_sysctl_tables(const char *path, char *pos, pos[0] = '\0'; if (err) goto out; + + if (entry->register_one) + break; } err = 0; out: @@ -1686,6 +1735,9 @@ static void put_links(struct ctl_table_header *header) sysctl_print_dir(parent); pr_cont("%s\n", name); } + + if (entry->register_one) + break; } } diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index 6353d6db69b2..889c995d8a08 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -134,6 +134,7 @@ struct ctl_table { void *data; int maxlen; umode_t mode; + bool register_one; /* Exactly register one ctl_table*/ struct ctl_table *child; /* Deprecated */ proc_handler *proc_handler; /* Callback for text formatting */ struct ctl_table_poll *poll;
Currently, sysctl is being moved to its own file. But ctl_table is quite large(64 bytes per entry) and every array is terminated with an empty one. This leads to thar when register exactly one ctl_table, we've gone from 64 bytes to 128 bytes. So, it is obviously the right thing that we need to fix. In order to avoid compatibility problems, and to be compatible with array terminated with an empty one and register exactly one ctl_table, add the register_one variable in the ctl_table structure to fix it. When we register exactly one table, we only need to add "table->register = true" to avoid gone from 64 bytes to 128 bytes. Signed-off-by: Meng Tang <tangmeng@uniontech.com> --- fs/proc/proc_sysctl.c | 58 +++++++++++++++++++++++++++++++++++++++--- include/linux/sysctl.h | 1 + 2 files changed, 56 insertions(+), 3 deletions(-)