| Message ID | 244616cd0a823e44fcca051a569ff68e0c7dc29e.1569587835.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coldify, constify, purify (function attributes) | expand |
On 1.10.19 г. 20:57 ч., David Sterba wrote: > The attribute can mark functions supposed to be called rarely if at all > and the text can be moved to sections far from the other code. The > attribute has been added to several functions already, this patch is > based on hints given by gcc -Wsuggest-attribute=cold. > > The net effect of this patch is decrease of btrfs.ko by 1000-1300, > depending on the config options. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/disk-io.c | 4 ++-- > fs/btrfs/disk-io.h | 4 ++-- > fs/btrfs/super.c | 2 +- > fs/btrfs/volumes.c | 2 +- > 4 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index e335fa4c4d1d..04d86e11117b 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2583,7 +2583,7 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info, > return ret; > } > > -int open_ctree(struct super_block *sb, > +int __cold open_ctree(struct super_block *sb, According to the documentation (https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html) of gcc attributes are placed in the declaration of a function (3rd paragraph): "Function attributes are introduced by the __attribute__ keyword in the declaration of a function, ..." > struct btrfs_fs_devices *fs_devices, > char *options) > { > @@ -3968,7 +3968,7 @@ int btrfs_commit_super(struct btrfs_fs_info *fs_info) > return btrfs_commit_transaction(trans); > } > > -void close_ctree(struct btrfs_fs_info *fs_info) > +void __cold close_ctree(struct btrfs_fs_info *fs_info) > { > int ret; > > diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h > index a6958103d87e..76f123ebb292 100644 > --- a/fs/btrfs/disk-io.h > +++ b/fs/btrfs/disk-io.h > @@ -49,10 +49,10 @@ struct extent_buffer *btrfs_find_create_tree_block( > struct btrfs_fs_info *fs_info, > u64 bytenr); > void btrfs_clean_tree_block(struct extent_buffer *buf); > -int open_ctree(struct super_block *sb, > +int __cold open_ctree(struct super_block *sb, > struct btrfs_fs_devices *fs_devices, > char *options); > -void close_ctree(struct btrfs_fs_info *fs_info); > +void __cold close_ctree(struct btrfs_fs_info *fs_info); > int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); > struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); > int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c> index 843015b9a11e..3da35d8b21a3 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -187,7 +187,7 @@ static struct ratelimit_state printk_limits[] = { > RATELIMIT_STATE_INIT(printk_limits[7], DEFAULT_RATELIMIT_INTERVAL, 100), > }; > > -void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...) > +void __cold btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...) > { Is printk really cold though? It's used in the various print helpers, even for info level print which might not be rare once the fs is mounted? What's a possible negative effect of this size optimisation - runtime cost? > char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0"; > struct va_format vaf; > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index fed4c9fe2ea2..3fd89aee539d 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2048,7 +2048,7 @@ static struct btrfs_device * btrfs_find_next_active_device( > * where this function called, there should be always be another device (or > * this_dev) which is active. > */ > -void btrfs_assign_next_active_device(struct btrfs_device *device, > +void __cold btrfs_assign_next_active_device(struct btrfs_device *device, > struct btrfs_device *this_dev) > { > struct btrfs_fs_info *fs_info = device->fs_info; >
On Wed, Oct 02, 2019 at 01:52:16PM +0300, Nikolay Borisov wrote: > On 1.10.19 г. 20:57 ч., David Sterba wrote: > > The attribute can mark functions supposed to be called rarely if at all > > and the text can be moved to sections far from the other code. The > > attribute has been added to several functions already, this patch is > > based on hints given by gcc -Wsuggest-attribute=cold. > > > > The net effect of this patch is decrease of btrfs.ko by 1000-1300, > > depending on the config options. > > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/disk-io.c | 4 ++-- > > fs/btrfs/disk-io.h | 4 ++-- > > fs/btrfs/super.c | 2 +- > > fs/btrfs/volumes.c | 2 +- > > 4 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index e335fa4c4d1d..04d86e11117b 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -2583,7 +2583,7 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info, > > return ret; > > } > > > > -int open_ctree(struct super_block *sb, > > +int __cold open_ctree(struct super_block *sb, > > According to the documentation > (https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html) of gcc > attributes are placed in the declaration of a function (3rd paragraph): > > > "Function attributes are introduced by the __attribute__ keyword in the > declaration of a function, ..." I'd rather keep the attributes to declaration and definition so it's in sync and looks consistent without further questions. > > +void __cold btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...) > > { > > Is printk really cold though? It's used in the various print helpers, > even for info level print which might not be rare once the fs is > mounted? What's a possible negative effect of this size optimisation - > runtime cost? Messages are printed rarely under normal circumstances, ie. a message once in a few minutes maybe. The penalty of loading the code into caches is IMO justified here, there's not much chance of reusing the cache hot code. And that it also involves all other helpers is kind of intentional. A very frequent pattern: x = find_some_structure(); if (!x) { btrfs_err("there is a problem"); ret = -EUCLEAN; goto out_error; } In this case the cold attribute is another hint to the compiler that the whole code block following the 'if' is cold and can be rearranged out of the way. If you have counter examples for printk-related functions that are on a hot path in btrfs, I'd like to hear about them. To move them out of the that hot path :)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index e335fa4c4d1d..04d86e11117b 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2583,7 +2583,7 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info, return ret; } -int open_ctree(struct super_block *sb, +int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_devices, char *options) { @@ -3968,7 +3968,7 @@ int btrfs_commit_super(struct btrfs_fs_info *fs_info) return btrfs_commit_transaction(trans); } -void close_ctree(struct btrfs_fs_info *fs_info) +void __cold close_ctree(struct btrfs_fs_info *fs_info) { int ret; diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h index a6958103d87e..76f123ebb292 100644 --- a/fs/btrfs/disk-io.h +++ b/fs/btrfs/disk-io.h @@ -49,10 +49,10 @@ struct extent_buffer *btrfs_find_create_tree_block( struct btrfs_fs_info *fs_info, u64 bytenr); void btrfs_clean_tree_block(struct extent_buffer *buf); -int open_ctree(struct super_block *sb, +int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_devices, char *options); -void close_ctree(struct btrfs_fs_info *fs_info); +void __cold close_ctree(struct btrfs_fs_info *fs_info); int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors); struct buffer_head *btrfs_read_dev_super(struct block_device *bdev); int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num, diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 843015b9a11e..3da35d8b21a3 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -187,7 +187,7 @@ static struct ratelimit_state printk_limits[] = { RATELIMIT_STATE_INIT(printk_limits[7], DEFAULT_RATELIMIT_INTERVAL, 100), }; -void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...) +void __cold btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...) { char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0"; struct va_format vaf; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index fed4c9fe2ea2..3fd89aee539d 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2048,7 +2048,7 @@ static struct btrfs_device * btrfs_find_next_active_device( * where this function called, there should be always be another device (or * this_dev) which is active. */ -void btrfs_assign_next_active_device(struct btrfs_device *device, +void __cold btrfs_assign_next_active_device(struct btrfs_device *device, struct btrfs_device *this_dev) { struct btrfs_fs_info *fs_info = device->fs_info;
The attribute can mark functions supposed to be called rarely if at all and the text can be moved to sections far from the other code. The attribute has been added to several functions already, this patch is based on hints given by gcc -Wsuggest-attribute=cold. The net effect of this patch is decrease of btrfs.ko by 1000-1300, depending on the config options. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/disk-io.c | 4 ++-- fs/btrfs/disk-io.h | 4 ++-- fs/btrfs/super.c | 2 +- fs/btrfs/volumes.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-)