Re: simplify procfs code for seq_file instances V3
On Wed, May 16, 2018 at 11:43:04AM +0200, Christoph Hellwig wrote: > We currently have hundreds of proc files that implement plain, read-only > seq_file based interfaces. This series consolidates them using new > procfs helpers that take the seq_operations or simple show callback > directly. > > A git tree is available at: > > git://git.infradead.org/users/hch/misc.git proc_create.3 Pulled, but the last bit is a bleeding atrocity in need of followup cleanup.
simplify procfs code for seq_file instances V3
We currently have hundreds of proc files that implement plain, read-only seq_file based interfaces. This series consolidates them using new procfs helpers that take the seq_operations or simple show callback directly. A git tree is available at: git://git.infradead.org/users/hch/misc.git proc_create.3 Gitweb: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/proc_create.3 Changes since V2: - use unsigned int for state_size everywhere - move state_size around in proc_dir_entry to use a struct packing hole - update SIZEOF_PDE_INLINE_NAME - added a new proc_pid_ns helper - improved a few changelogs - added back a nubus comment - minor typo fix - collected various ACKs Changes since V1: - open code proc_create_data to avoid setting not fully initialized entries live - use unsigned int for state_size - dropped the s390/cio/blacklist hunk as it has a write method - dropped the IPMI patch given that IPMI proc support is scheduled for removal.
Re: simplify procfs code for seq_file instances V2
On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote: > On Wed, Apr 25, 2018 at 05:47:47PM +0200, Christoph Hellwig wrote: > > Changes since V1: > > - open code proc_create_data to avoid setting not fully initialized > >entries live > > - use unsigned int for state_size > > Need this to maintain sizeof(struct proc_dir_entry): I'll fold your changes into the relevant patches. > Otherwise ACK fs/proc/ part. I'll take this as a formal ACK-ed by for all patches touching procfs. If I was wrong please scream.
Re: simplify procfs code for seq_file instances V2
On Sun, May 06, 2018 at 06:45:31PM +0100, Al Viro wrote: > On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote: > > @@ -62,9 +62,9 @@ struct proc_dir_entry { > > umode_t mode; > > u8 namelen; > > #ifdef CONFIG_64BIT > > -#define SIZEOF_PDE_INLINE_NAME (192-139) > > +#define SIZEOF_PDE_INLINE_NAME (192-155) > > #else > > -#define SIZEOF_PDE_INLINE_NAME (128-87) > > +#define SIZEOF_PDE_INLINE_NAME (128-95) > > > #endif > > char inline_name[SIZEOF_PDE_INLINE_NAME]; > > } __randomize_layout; > > *UGH* I agree. Maintaining these numbers is a pain point. Who knew people were going to start adding fields right away. > Both to the original state and that kind of "adjustments". > Incidentally, with __bugger_layout in there these expressions > are simply wrong. Struct randomization is exempt from maintaining sizeof as they are already screwing cachelines and everything. But if patch works with lockdep and randomization -- even better. > If nothing else, I would suggest turning the last one into > char inline_name[]; > in hope that layout won't get... randomized that much and > used > > #ifdef CONFIG_64BIT > #define PDE_SIZE 192 > #else > #define PDE_SIZE 128 > #endif > > union __proc_dir_entry { > char pad[PDE_SIZE]; > struct proc_dir_entry real; > }; > > #define SIZEOF_PDE_INLINE_NAME (PDE_SIZE - offsetof(struct proc_dir_entry, > inline_name)) > > for constants, adjusted sizeof and sizeof_field when creating > proc_dir_entry_cache and turned proc_root into > > union __proc_dir_entry __proc_root = { .real = { > .low_ino= PROC_ROOT_INO, > .namelen= 5, > .mode = S_IFDIR | S_IRUGO | S_IXUGO, > .nlink = 2, > .refcnt = REFCOUNT_INIT(1), > .proc_iops = &proc_root_inode_operations, > .proc_fops = &proc_root_operations, > .parent = &__proc_root.real, > .subdir = RB_ROOT, > .name = __proc_root.real.inline_name, > .inline_name= "/proc", > }}; > > #define proc_root __proc_root.real > (or actually used __proc_root.real in all of a 6 places where it remains). > > > - size_t state_size = PDE(inode)->state_size; > > + unsigned int state_size = PDE(inode)->state_size; > > > You and your "size_t is evil" crusade... [nods] unsigned long flags; /* error bits */ unsigned long i_state; unsigned long s_blocksize; unsigned long s_flags; unsigned long s_iflags; /* internal SB_I_* flags */
Re: simplify procfs code for seq_file instances V2
On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote: > +++ b/fs/proc/internal.h > @@ -48,8 +48,8 @@ struct proc_dir_entry { > const struct seq_operations *seq_ops; > int (*single_show)(struct seq_file *, void *); > }; > - unsigned int state_size; > void *data; > + unsigned int state_size; > unsigned int low_ino; > nlink_t nlink; > kuid_t uid; Makes sense > @@ -62,9 +62,9 @@ struct proc_dir_entry { > umode_t mode; > u8 namelen; > #ifdef CONFIG_64BIT > -#define SIZEOF_PDE_INLINE_NAME (192-139) > +#define SIZEOF_PDE_INLINE_NAME (192-155) > #else > -#define SIZEOF_PDE_INLINE_NAME (128-87) > +#define SIZEOF_PDE_INLINE_NAME (128-95) > #endif > char inline_name[SIZEOF_PDE_INLINE_NAME]; > } __randomize_layout; *UGH* Both to the original state and that kind of "adjustments". Incidentally, with __bugger_layout in there these expressions are simply wrong. If nothing else, I would suggest turning the last one into char inline_name[]; in hope that layout won't get... randomized that much and used #ifdef CONFIG_64BIT #define PDE_SIZE 192 #else #define PDE_SIZE 128 #endif union __proc_dir_entry { char pad[PDE_SIZE]; struct proc_dir_entry real; }; #define SIZEOF_PDE_INLINE_NAME (PDE_SIZE - offsetof(struct proc_dir_entry, inline_name)) for constants, adjusted sizeof and sizeof_field when creating proc_dir_entry_cache and turned proc_root into union __proc_dir_entry __proc_root = { .real = { .low_ino= PROC_ROOT_INO, .namelen= 5, .mode = S_IFDIR | S_IRUGO | S_IXUGO, .nlink = 2, .refcnt = REFCOUNT_INIT(1), .proc_iops = &proc_root_inode_operations, .proc_fops = &proc_root_operations, .parent = &__proc_root.real, .subdir = RB_ROOT, .name = __proc_root.real.inline_name, .inline_name= "/proc", }}; #define proc_root __proc_root.real (or actually used __proc_root.real in all of a 6 places where it remains). > diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c > index baf1994289ce..7d94fa005b0d 100644 > --- a/fs/proc/proc_net.c > +++ b/fs/proc/proc_net.c > @@ -40,7 +40,7 @@ static struct net *get_proc_net(const struct inode *inode) > > static int seq_open_net(struct inode *inode, struct file *file) > { > - size_t state_size = PDE(inode)->state_size; > + unsigned int state_size = PDE(inode)->state_size; > struct seq_net_private *p; > struct net *net; You and your "size_t is evil" crusade...
Re: simplify procfs code for seq_file instances V2
On Wed, Apr 25, 2018 at 05:47:47PM +0200, Christoph Hellwig wrote: > Changes since V1: > - open code proc_create_data to avoid setting not fully initialized >entries live > - use unsigned int for state_size Need this to maintain sizeof(struct proc_dir_entry): Otherwise ACK fs/proc/ part. diff --git a/fs/proc/internal.h b/fs/proc/internal.h index 6d171485c45b..a318ae5b36b4 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -48,8 +48,8 @@ struct proc_dir_entry { const struct seq_operations *seq_ops; int (*single_show)(struct seq_file *, void *); }; - unsigned int state_size; void *data; + unsigned int state_size; unsigned int low_ino; nlink_t nlink; kuid_t uid; @@ -62,9 +62,9 @@ struct proc_dir_entry { umode_t mode; u8 namelen; #ifdef CONFIG_64BIT -#define SIZEOF_PDE_INLINE_NAME (192-139) +#define SIZEOF_PDE_INLINE_NAME (192-155) #else -#define SIZEOF_PDE_INLINE_NAME (128-87) +#define SIZEOF_PDE_INLINE_NAME (128-95) #endif char inline_name[SIZEOF_PDE_INLINE_NAME]; } __randomize_layout; diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c index baf1994289ce..7d94fa005b0d 100644 --- a/fs/proc/proc_net.c +++ b/fs/proc/proc_net.c @@ -40,7 +40,7 @@ static struct net *get_proc_net(const struct inode *inode) static int seq_open_net(struct inode *inode, struct file *file) { - size_t state_size = PDE(inode)->state_size; + unsigned int state_size = PDE(inode)->state_size; struct seq_net_private *p; struct net *net;
Re: simplify procfs code for seq_file instances V2
Note that your kernel hits the: inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage. swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes: (ptrval) (fs_reclaim){?.+.}, at: fs_reclaim_acquire+0x12/0x35 {HARDIRQ-ON-W} state was registered at: fs_reclaim_acquire+0x32/0x35 kmem_cache_alloc_node_trace+0x49/0x2cf alloc_worker+0x1d/0x49 init_rescuer.part.7+0x19/0x8f workqueue_init+0xc0/0x1fe kernel_init_freeable+0xdc/0x433 kernel_init+0xa/0xf5 ret_from_fork+0x24/0x30 bug, as described here: https://groups.google.com/forum/#!msg/syzkaller-bugs/sJC3Y3hOM08/aO3z9JXoAgAJ David
Re: simplify procfs code for seq_file instances
On Tue, Apr 24, 2018 at 06:06:53PM +0200, Christoph Hellwig wrote: > On Tue, Apr 24, 2018 at 08:19:16AM -0700, Andrew Morton wrote: > > > > I want to ask if it is time to start using poorman function overloading > > > > with _b_c_e(). There are millions of allocation functions for example, > > > > all slightly difference, and people will add more. Seeing /proc > > > > interfaces > > > > doubled like this is painful. > > > > > > Function overloading is totally unacceptable. > > > > > > And I very much disagree with a tradeoff that keeps 5000 lines of > > > code vs a few new helpers. > > > > OK, the curiosity and suspense are killing me. What the heck is > > "function overloading with _b_c_e()"? > > The way I understood Alexey was to use have a proc_create macro > that can take different ops types. Although the short cut for > __builtin_types_compatible_p would be _b_t_c or similar, so maybe > I misunderstood him. That's correct. I also think that several dozens kmalloc signatures are a problem. And there will be more with pmalloc* stuff and more 2D/3D array checked allocations and who knows what. And I want to add typed kmalloc!
simplify procfs code for seq_file instances V2
We currently have hundreds of proc files that implement plain, read-only seq_file based interfaces. This series consolidates them using new procfs helpers that take the seq_operations or simple show callback directly. A git tree is available at: git://git.infradead.org/users/hch/misc.git proc_create.2 Gitweb: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/proc_create.2 Changes since V1: - open code proc_create_data to avoid setting not fully initialized entries live - use unsigned int for state_size - dropped the s390/cio/blacklist hunk as it has a write method - dropped the IPMI patch given that IPMI proc support is scheduled for removal.
Re: simplify procfs code for seq_file instances
On Tue, Apr 24, 2018 at 08:19:16AM -0700, Andrew Morton wrote: > > > I want to ask if it is time to start using poorman function overloading > > > with _b_c_e(). There are millions of allocation functions for example, > > > all slightly difference, and people will add more. Seeing /proc interfaces > > > doubled like this is painful. > > > > Function overloading is totally unacceptable. > > > > And I very much disagree with a tradeoff that keeps 5000 lines of > > code vs a few new helpers. > > OK, the curiosity and suspense are killing me. What the heck is > "function overloading with _b_c_e()"? The way I understood Alexey was to use have a proc_create macro that can take different ops types. Although the short cut for __builtin_types_compatible_p would be _b_t_c or similar, so maybe I misunderstood him.
Re: simplify procfs code for seq_file instances
On Tue, 24 Apr 2018 16:23:04 +0200 Christoph Hellwig wrote: > On Thu, Apr 19, 2018 at 09:57:50PM +0300, Alexey Dobriyan wrote: > > > git://git.infradead.org/users/hch/misc.git proc_create > > > > > > I want to ask if it is time to start using poorman function overloading > > with _b_c_e(). There are millions of allocation functions for example, > > all slightly difference, and people will add more. Seeing /proc interfaces > > doubled like this is painful. > > Function overloading is totally unacceptable. > > And I very much disagree with a tradeoff that keeps 5000 lines of > code vs a few new helpers. OK, the curiosity and suspense are killing me. What the heck is "function overloading with _b_c_e()"?
Re: simplify procfs code for seq_file instances
On Thu, Apr 19, 2018 at 09:57:50PM +0300, Alexey Dobriyan wrote: > > git://git.infradead.org/users/hch/misc.git proc_create > > > I want to ask if it is time to start using poorman function overloading > with _b_c_e(). There are millions of allocation functions for example, > all slightly difference, and people will add more. Seeing /proc interfaces > doubled like this is painful. Function overloading is totally unacceptable. And I very much disagree with a tradeoff that keeps 5000 lines of code vs a few new helpers.
Re: simplify procfs code for seq_file instances
> git://git.infradead.org/users/hch/misc.git proc_create I want to ask if it is time to start using poorman function overloading with _b_c_e(). There are millions of allocation functions for example, all slightly difference, and people will add more. Seeing /proc interfaces doubled like this is painful.
simplify procfs code for seq_file instances
We currently have hundreds of proc files that implement plain, read-only seq_file based interfaces. This series consolidates them using new procfs helpers that take the seq_operations or simple show callback directly. A git tree is available at: git://git.infradead.org/users/hch/misc.git proc_create Gitweb: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/proc_create Diffstat: 268 files changed, 1193 insertions(+), 6194 deletions(-)