* [PATCH 0/3] Coldify, constify, purify (function attributes)
@ 2019-10-01 17:57 David Sterba
2019-10-01 17:57 ` [PATCH 1/3] btrfs: add __cold attribute to more functions David Sterba
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: David Sterba @ 2019-10-01 17:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
There's a gcc option -Wsuggest-attribute that, as it says, suggests some
function attributes to provide some hints and allowing more
optimizations.
Debug build
text data bss dec hex filename
1514058 146768 27496 1688322 19c302 pre/btrfs.ko
1512603 146736 27496 1686835 19bd33 post/btrfs.ko
DELTA: -1455
Release build
text data bss dec hex filename
1079288 17316 14912 1111516 10f5dc pre/btrfs.ko
1078138 17316 14912 1110366 10f15e post/btrfs.ko
DELTA: -1150
David Sterba (3):
btrfs: add __cold attribute to more functions
btrfs: add const function attribute
btrfs: add __pure attribute to functions
fs/btrfs/async-thread.c | 6 ++----
fs/btrfs/async-thread.h | 4 ++--
fs/btrfs/ctree.c | 2 +-
fs/btrfs/ctree.h | 6 +++---
fs/btrfs/dev-replace.c | 2 +-
fs/btrfs/dev-replace.h | 2 +-
fs/btrfs/disk-io.c | 4 ++--
fs/btrfs/disk-io.h | 4 ++--
fs/btrfs/ioctl.c | 2 +-
fs/btrfs/space-info.c | 2 +-
fs/btrfs/space-info.h | 2 +-
fs/btrfs/super.c | 4 ++--
fs/btrfs/volumes.c | 4 ++--
fs/btrfs/volumes.h | 2 +-
14 files changed, 22 insertions(+), 24 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] btrfs: add __cold attribute to more functions
2019-10-01 17:57 [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
@ 2019-10-01 17:57 ` David Sterba
2019-10-02 10:52 ` Nikolay Borisov
2019-10-01 17:57 ` [PATCH 2/3] btrfs: add const function attribute David Sterba
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2019-10-01 17:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
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,
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;
--
2.23.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] btrfs: add const function attribute
2019-10-01 17:57 [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
2019-10-01 17:57 ` [PATCH 1/3] btrfs: add __cold attribute to more functions David Sterba
@ 2019-10-01 17:57 ` David Sterba
2019-10-02 11:07 ` Nikolay Borisov
2019-10-01 17:57 ` [PATCH 3/3] btrfs: add __pure attribute to functions David Sterba
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2019-10-01 17:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
For some reason the attribute is called __attribute_const__ and not
__const, marks functions that have no observable effects on program
state, IOW not reading pointers, just the arguments and calculating a
value. Allows the compiler to do some optimizations, based on
-Wsuggest-attribute=const . The effects are rather small, though, about
60 bytes decrese of btrfs.ko.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/ctree.h | 2 +-
fs/btrfs/super.c | 2 +-
fs/btrfs/volumes.c | 2 +-
fs/btrfs/volumes.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4bf0433b1179..793085770c84 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3146,7 +3146,7 @@ __cold
void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function,
unsigned int line, int errno, const char *fmt, ...);
-const char *btrfs_decode_error(int errno);
+const char * __attribute_const__ btrfs_decode_error(int errno);
__cold
void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3da35d8b21a3..b3e6d7aa3402 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -66,7 +66,7 @@ static struct file_system_type btrfs_root_fs_type;
static int btrfs_remount(struct super_block *sb, int *flags, char *data);
-const char *btrfs_decode_error(int errno)
+const char * __attribute_const__ btrfs_decode_error(int errno)
{
char *errstr = "unknown";
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3fd89aee539d..c343b7cdfb53 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -297,7 +297,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
DEFINE_MUTEX(uuid_mutex);
static LIST_HEAD(fs_uuids);
-struct list_head *btrfs_get_fs_uuids(void)
+struct list_head * __attribute_const__ btrfs_get_fs_uuids(void)
{
return &fs_uuids;
}
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index a7da1f3e3627..0ae0677a8d86 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -571,7 +571,7 @@ static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
void btrfs_commit_device_sizes(struct btrfs_transaction *trans);
-struct list_head *btrfs_get_fs_uuids(void);
+struct list_head * __attribute_const__ btrfs_get_fs_uuids(void);
void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info);
void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info);
bool btrfs_check_rw_degradable(struct btrfs_fs_info *fs_info,
--
2.23.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] btrfs: add __pure attribute to functions
2019-10-01 17:57 [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
2019-10-01 17:57 ` [PATCH 1/3] btrfs: add __cold attribute to more functions David Sterba
2019-10-01 17:57 ` [PATCH 2/3] btrfs: add const function attribute David Sterba
@ 2019-10-01 17:57 ` David Sterba
2019-10-02 11:09 ` Nikolay Borisov
2019-10-01 17:57 ` [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
2019-10-02 12:20 ` Nikolay Borisov
4 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2019-10-01 17:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The attribute is more relaxed than const and the functions could
dereference pointers, as long as the observable state is not changed. We
do have such functions, based on -Wsuggest-attribute=pure .
The visible effects of this patch are negligible, there are differences
in the assembly but hard to summarize.
Signed-off-by: David Sterba <dsterba@suse.com>
---
fs/btrfs/async-thread.c | 6 ++----
fs/btrfs/async-thread.h | 4 ++--
fs/btrfs/ctree.c | 2 +-
fs/btrfs/ctree.h | 4 ++--
fs/btrfs/dev-replace.c | 2 +-
fs/btrfs/dev-replace.h | 2 +-
fs/btrfs/ioctl.c | 2 +-
fs/btrfs/space-info.c | 2 +-
fs/btrfs/space-info.h | 2 +-
9 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index b97ae1b03417..1d32a07bb2d1 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -53,14 +53,12 @@ struct btrfs_workqueue {
struct __btrfs_workqueue *high;
};
-struct btrfs_fs_info *
-btrfs_workqueue_owner(const struct __btrfs_workqueue *wq)
+struct btrfs_fs_info * __pure btrfs_workqueue_owner(const struct __btrfs_workqueue *wq)
{
return wq->fs_info;
}
-struct btrfs_fs_info *
-btrfs_work_owner(const struct btrfs_work *work)
+struct btrfs_fs_info * __pure btrfs_work_owner(const struct btrfs_work *work)
{
return work->wq->fs_info;
}
diff --git a/fs/btrfs/async-thread.h b/fs/btrfs/async-thread.h
index c5bf2b117c05..a4434301d84d 100644
--- a/fs/btrfs/async-thread.h
+++ b/fs/btrfs/async-thread.h
@@ -41,8 +41,8 @@ void btrfs_queue_work(struct btrfs_workqueue *wq,
void btrfs_destroy_workqueue(struct btrfs_workqueue *wq);
void btrfs_workqueue_set_max(struct btrfs_workqueue *wq, int max);
void btrfs_set_work_high_priority(struct btrfs_work *work);
-struct btrfs_fs_info *btrfs_work_owner(const struct btrfs_work *work);
-struct btrfs_fs_info *btrfs_workqueue_owner(const struct __btrfs_workqueue *wq);
+struct btrfs_fs_info * __pure btrfs_work_owner(const struct btrfs_work *work);
+struct btrfs_fs_info * __pure btrfs_workqueue_owner(const struct __btrfs_workqueue *wq);
bool btrfs_workqueue_normal_congested(const struct btrfs_workqueue *wq);
#endif
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index f2f9cf1149a4..3a4d8e27e565 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1538,7 +1538,7 @@ static int comp_keys(const struct btrfs_disk_key *disk,
/*
* same as comp_keys only with two btrfs_key's
*/
-int btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2)
+int __pure btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2)
{
if (k1->objectid > k2->objectid)
return 1;
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 793085770c84..df0f2de69991 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2511,7 +2511,7 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root);
/* ctree.c */
int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
int level, int *slot);
-int btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2);
+int __pure btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2);
int btrfs_previous_item(struct btrfs_root *root,
struct btrfs_path *path, u64 min_objectid,
int type);
@@ -2912,7 +2912,7 @@ long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
int btrfs_ioctl_get_supported_features(void __user *arg);
void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
-int btrfs_is_empty_uuid(u8 *uuid);
+int __pure btrfs_is_empty_uuid(u8 *uuid);
int btrfs_defrag_file(struct inode *inode, struct file *file,
struct btrfs_ioctl_defrag_range_args *range,
u64 newer_than, unsigned long max_pages);
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 48890826b5e6..f639dde2a679 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -986,7 +986,7 @@ static int btrfs_dev_replace_kthread(void *data)
return 0;
}
-int btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace)
+int __pure btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace)
{
if (!dev_replace->is_valid)
return 0;
diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
index 78c5d8f1adda..60b70dacc299 100644
--- a/fs/btrfs/dev-replace.h
+++ b/fs/btrfs/dev-replace.h
@@ -17,6 +17,6 @@ void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
void btrfs_dev_replace_suspend_for_unmount(struct btrfs_fs_info *fs_info);
int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info);
-int btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace);
+int __pure btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace);
#endif
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index de730e56d3f5..55b41d5e7ea9 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -541,7 +541,7 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)
return 0;
}
-int btrfs_is_empty_uuid(u8 *uuid)
+int __pure btrfs_is_empty_uuid(u8 *uuid)
{
int i;
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 98dc092a905e..f32993efbf61 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -10,7 +10,7 @@
#include "transaction.h"
#include "block-group.h"
-u64 btrfs_space_info_used(struct btrfs_space_info *s_info,
+u64 __pure btrfs_space_info_used(struct btrfs_space_info *s_info,
bool may_use_included)
{
ASSERT(s_info);
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index 8867e84aa33d..2d8c811a9792 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -116,7 +116,7 @@ void btrfs_update_space_info(struct btrfs_fs_info *info, u64 flags,
struct btrfs_space_info **space_info);
struct btrfs_space_info *btrfs_find_space_info(struct btrfs_fs_info *info,
u64 flags);
-u64 btrfs_space_info_used(struct btrfs_space_info *s_info,
+u64 __pure btrfs_space_info_used(struct btrfs_space_info *s_info,
bool may_use_included);
void btrfs_clear_space_info_full(struct btrfs_fs_info *info);
void btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
--
2.23.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 0/3] Coldify, constify, purify (function attributes)
2019-10-01 17:57 [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
` (2 preceding siblings ...)
2019-10-01 17:57 ` [PATCH 3/3] btrfs: add __pure attribute to functions David Sterba
@ 2019-10-01 17:57 ` David Sterba
2019-10-02 12:20 ` Nikolay Borisov
4 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2019-10-01 17:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
There's a gcc option -Wsuggest-attribute that, as it says, suggests some
function attributes to provide some hints and allowing more
optimizations.
Debug build
text data bss dec hex filename
1514058 146768 27496 1688322 19c302 pre/btrfs.ko
1512603 146736 27496 1686835 19bd33 post/btrfs.ko
DELTA: -1455
Release build
text data bss dec hex filename
1079288 17316 14912 1111516 10f5dc pre/btrfs.ko
1078138 17316 14912 1110366 10f15e post/btrfs.ko
DELTA: -1150
David Sterba (3):
btrfs: add __cold attribute to more functions
btrfs: add const function attribute
btrfs: add __pure attribute to functions
fs/btrfs/async-thread.c | 6 ++----
fs/btrfs/async-thread.h | 4 ++--
fs/btrfs/ctree.c | 2 +-
fs/btrfs/ctree.h | 6 +++---
fs/btrfs/dev-replace.c | 2 +-
fs/btrfs/dev-replace.h | 2 +-
fs/btrfs/disk-io.c | 4 ++--
fs/btrfs/disk-io.h | 4 ++--
fs/btrfs/ioctl.c | 2 +-
fs/btrfs/space-info.c | 2 +-
fs/btrfs/space-info.h | 2 +-
fs/btrfs/super.c | 4 ++--
fs/btrfs/volumes.c | 4 ++--
fs/btrfs/volumes.h | 2 +-
14 files changed, 22 insertions(+), 24 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] btrfs: add __cold attribute to more functions
2019-10-01 17:57 ` [PATCH 1/3] btrfs: add __cold attribute to more functions David Sterba
@ 2019-10-02 10:52 ` Nikolay Borisov
2019-10-04 10:56 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: Nikolay Borisov @ 2019-10-02 10:52 UTC (permalink / raw)
To: David Sterba, linux-btrfs
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;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: add const function attribute
2019-10-01 17:57 ` [PATCH 2/3] btrfs: add const function attribute David Sterba
@ 2019-10-02 11:07 ` Nikolay Borisov
2019-10-04 11:01 ` David Sterba
0 siblings, 1 reply; 11+ messages in thread
From: Nikolay Borisov @ 2019-10-02 11:07 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 1.10.19 г. 20:57 ч., David Sterba wrote:
> For some reason the attribute is called __attribute_const__ and not
> __const, marks functions that have no observable effects on program
> state, IOW not reading pointers, just the arguments and calculating a
> value. Allows the compiler to do some optimizations, based on
> -Wsuggest-attribute=const . The effects are rather small, though, about
> 60 bytes decrese of btrfs.ko.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/ctree.h | 2 +-
> fs/btrfs/super.c | 2 +-
> fs/btrfs/volumes.c | 2 +-
> fs/btrfs/volumes.h | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4bf0433b1179..793085770c84 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3146,7 +3146,7 @@ __cold
> void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function,
> unsigned int line, int errno, const char *fmt, ...);
>
> -const char *btrfs_decode_error(int errno);
> +const char * __attribute_const__ btrfs_decode_error(int errno);
>
> __cold
> void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 3da35d8b21a3..b3e6d7aa3402 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -66,7 +66,7 @@ static struct file_system_type btrfs_root_fs_type;
>
> static int btrfs_remount(struct super_block *sb, int *flags, char *data);
>
> -const char *btrfs_decode_error(int errno)
> +const char * __attribute_const__ btrfs_decode_error(int errno)
> {
> char *errstr = "unknown";
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 3fd89aee539d..c343b7cdfb53 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -297,7 +297,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>
> DEFINE_MUTEX(uuid_mutex);
> static LIST_HEAD(fs_uuids);
> -struct list_head *btrfs_get_fs_uuids(void)
> +struct list_head * __attribute_const__ btrfs_get_fs_uuids(void)
I'm not entirely sure this function is cons. According to the manual:
Calls to functions whose return value is not affected by changes to the
observable state of the program and that have no observable effects on
such state other than to return a value may lend themselves to
optimizations such as common subexpression elimination.
The const attribute prohibits a function from reading objects that
affect its return value between successive invocations. However,
functions declared with the attribute can safely read objects that do
not change their return value, such as non-volatile constants.
My doubt stems from the fact this function actually references outside
memory, namely gets the ptr to fs_uuids. There is a specific remark not
to use const when the function takes a ptr argument but it doesn't say
anything when getting a ptr from a global var.
> {
> return &fs_uuids;
> }
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index a7da1f3e3627..0ae0677a8d86 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -571,7 +571,7 @@ static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags)
>
> void btrfs_commit_device_sizes(struct btrfs_transaction *trans);
>
> -struct list_head *btrfs_get_fs_uuids(void);
> +struct list_head * __attribute_const__ btrfs_get_fs_uuids(void);
> void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info);
> void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info);
> bool btrfs_check_rw_degradable(struct btrfs_fs_info *fs_info,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] btrfs: add __pure attribute to functions
2019-10-01 17:57 ` [PATCH 3/3] btrfs: add __pure attribute to functions David Sterba
@ 2019-10-02 11:09 ` Nikolay Borisov
0 siblings, 0 replies; 11+ messages in thread
From: Nikolay Borisov @ 2019-10-02 11:09 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 1.10.19 г. 20:57 ч., David Sterba wrote:
> The attribute is more relaxed than const and the functions could
> dereference pointers, as long as the observable state is not changed. We
> do have such functions, based on -Wsuggest-attribute=pure .
>
> The visible effects of this patch are negligible, there are differences
> in the assembly but hard to summarize.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
one remark: I'm not sure the attribute needs to be duplicated in the
declaration and definition.
> ---
> fs/btrfs/async-thread.c | 6 ++----
> fs/btrfs/async-thread.h | 4 ++--
> fs/btrfs/ctree.c | 2 +-
> fs/btrfs/ctree.h | 4 ++--
> fs/btrfs/dev-replace.c | 2 +-
> fs/btrfs/dev-replace.h | 2 +-
> fs/btrfs/ioctl.c | 2 +-
> fs/btrfs/space-info.c | 2 +-
> fs/btrfs/space-info.h | 2 +-
> 9 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
> index b97ae1b03417..1d32a07bb2d1 100644
> --- a/fs/btrfs/async-thread.c
> +++ b/fs/btrfs/async-thread.c
> @@ -53,14 +53,12 @@ struct btrfs_workqueue {
> struct __btrfs_workqueue *high;
> };
>
> -struct btrfs_fs_info *
> -btrfs_workqueue_owner(const struct __btrfs_workqueue *wq)
> +struct btrfs_fs_info * __pure btrfs_workqueue_owner(const struct __btrfs_workqueue *wq)
> {
> return wq->fs_info;
> }
>
> -struct btrfs_fs_info *
> -btrfs_work_owner(const struct btrfs_work *work)
> +struct btrfs_fs_info * __pure btrfs_work_owner(const struct btrfs_work *work)
> {
> return work->wq->fs_info;
> }
> diff --git a/fs/btrfs/async-thread.h b/fs/btrfs/async-thread.h
> index c5bf2b117c05..a4434301d84d 100644
> --- a/fs/btrfs/async-thread.h
> +++ b/fs/btrfs/async-thread.h
> @@ -41,8 +41,8 @@ void btrfs_queue_work(struct btrfs_workqueue *wq,
> void btrfs_destroy_workqueue(struct btrfs_workqueue *wq);
> void btrfs_workqueue_set_max(struct btrfs_workqueue *wq, int max);
> void btrfs_set_work_high_priority(struct btrfs_work *work);
> -struct btrfs_fs_info *btrfs_work_owner(const struct btrfs_work *work);
> -struct btrfs_fs_info *btrfs_workqueue_owner(const struct __btrfs_workqueue *wq);
> +struct btrfs_fs_info * __pure btrfs_work_owner(const struct btrfs_work *work);
> +struct btrfs_fs_info * __pure btrfs_workqueue_owner(const struct __btrfs_workqueue *wq);
> bool btrfs_workqueue_normal_congested(const struct btrfs_workqueue *wq);
>
> #endif
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index f2f9cf1149a4..3a4d8e27e565 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1538,7 +1538,7 @@ static int comp_keys(const struct btrfs_disk_key *disk,
> /*
> * same as comp_keys only with two btrfs_key's
> */
> -int btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2)
> +int __pure btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2)
> {
> if (k1->objectid > k2->objectid)
> return 1;
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 793085770c84..df0f2de69991 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2511,7 +2511,7 @@ void btrfs_wait_for_snapshot_creation(struct btrfs_root *root);
> /* ctree.c */
> int btrfs_bin_search(struct extent_buffer *eb, const struct btrfs_key *key,
> int level, int *slot);
> -int btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2);
> +int __pure btrfs_comp_cpu_keys(const struct btrfs_key *k1, const struct btrfs_key *k2);
> int btrfs_previous_item(struct btrfs_root *root,
> struct btrfs_path *path, u64 min_objectid,
> int type);
> @@ -2912,7 +2912,7 @@ long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> int btrfs_ioctl_get_supported_features(void __user *arg);
> void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
> -int btrfs_is_empty_uuid(u8 *uuid);
> +int __pure btrfs_is_empty_uuid(u8 *uuid);
> int btrfs_defrag_file(struct inode *inode, struct file *file,
> struct btrfs_ioctl_defrag_range_args *range,
> u64 newer_than, unsigned long max_pages);
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 48890826b5e6..f639dde2a679 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -986,7 +986,7 @@ static int btrfs_dev_replace_kthread(void *data)
> return 0;
> }
>
> -int btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace)
> +int __pure btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace)
> {
> if (!dev_replace->is_valid)
> return 0;
> diff --git a/fs/btrfs/dev-replace.h b/fs/btrfs/dev-replace.h
> index 78c5d8f1adda..60b70dacc299 100644
> --- a/fs/btrfs/dev-replace.h
> +++ b/fs/btrfs/dev-replace.h
> @@ -17,6 +17,6 @@ void btrfs_dev_replace_status(struct btrfs_fs_info *fs_info,
> int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info);
> void btrfs_dev_replace_suspend_for_unmount(struct btrfs_fs_info *fs_info);
> int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info);
> -int btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace);
> +int __pure btrfs_dev_replace_is_ongoing(struct btrfs_dev_replace *dev_replace);
>
> #endif
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index de730e56d3f5..55b41d5e7ea9 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -541,7 +541,7 @@ static noinline int btrfs_ioctl_fitrim(struct file *file, void __user *arg)
> return 0;
> }
>
> -int btrfs_is_empty_uuid(u8 *uuid)
> +int __pure btrfs_is_empty_uuid(u8 *uuid)
> {
> int i;
>
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 98dc092a905e..f32993efbf61 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -10,7 +10,7 @@
> #include "transaction.h"
> #include "block-group.h"
>
> -u64 btrfs_space_info_used(struct btrfs_space_info *s_info,
> +u64 __pure btrfs_space_info_used(struct btrfs_space_info *s_info,
> bool may_use_included)
> {
> ASSERT(s_info);
> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
> index 8867e84aa33d..2d8c811a9792 100644
> --- a/fs/btrfs/space-info.h
> +++ b/fs/btrfs/space-info.h
> @@ -116,7 +116,7 @@ void btrfs_update_space_info(struct btrfs_fs_info *info, u64 flags,
> struct btrfs_space_info **space_info);
> struct btrfs_space_info *btrfs_find_space_info(struct btrfs_fs_info *info,
> u64 flags);
> -u64 btrfs_space_info_used(struct btrfs_space_info *s_info,
> +u64 __pure btrfs_space_info_used(struct btrfs_space_info *s_info,
> bool may_use_included);
> void btrfs_clear_space_info_full(struct btrfs_fs_info *info);
> void btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Coldify, constify, purify (function attributes)
2019-10-01 17:57 [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
` (3 preceding siblings ...)
2019-10-01 17:57 ` [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
@ 2019-10-02 12:20 ` Nikolay Borisov
4 siblings, 0 replies; 11+ messages in thread
From: Nikolay Borisov @ 2019-10-02 12:20 UTC (permalink / raw)
To: David Sterba, linux-btrfs
On 1.10.19 г. 20:57 ч., David Sterba wrote:
> There's a gcc option -Wsuggest-attribute that, as it says, suggests some
> function attributes to provide some hints and allowing more
> optimizations.
>
> Debug build
>
> text data bss dec hex filename
> 1514058 146768 27496 1688322 19c302 pre/btrfs.ko
> 1512603 146736 27496 1686835 19bd33 post/btrfs.ko
> DELTA: -1455
>
> Release build
>
> text data bss dec hex filename
> 1079288 17316 14912 1111516 10f5dc pre/btrfs.ko
> 1078138 17316 14912 1110366 10f15e post/btrfs.ko
> DELTA: -1150
>
> David Sterba (3):
> btrfs: add __cold attribute to more functions
> btrfs: add const function attribute
> btrfs: add __pure attribute to functions
Checking up the with compiler ppl and discussing I can say:
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
> fs/btrfs/async-thread.c | 6 ++----
> fs/btrfs/async-thread.h | 4 ++--
> fs/btrfs/ctree.c | 2 +-
> fs/btrfs/ctree.h | 6 +++---
> fs/btrfs/dev-replace.c | 2 +-
> fs/btrfs/dev-replace.h | 2 +-
> fs/btrfs/disk-io.c | 4 ++--
> fs/btrfs/disk-io.h | 4 ++--
> fs/btrfs/ioctl.c | 2 +-
> fs/btrfs/space-info.c | 2 +-
> fs/btrfs/space-info.h | 2 +-
> fs/btrfs/super.c | 4 ++--
> fs/btrfs/volumes.c | 4 ++--
> fs/btrfs/volumes.h | 2 +-
> 14 files changed, 22 insertions(+), 24 deletions(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] btrfs: add __cold attribute to more functions
2019-10-02 10:52 ` Nikolay Borisov
@ 2019-10-04 10:56 ` David Sterba
0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2019-10-04 10:56 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: David Sterba, linux-btrfs
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 :)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] btrfs: add const function attribute
2019-10-02 11:07 ` Nikolay Borisov
@ 2019-10-04 11:01 ` David Sterba
0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2019-10-04 11:01 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: David Sterba, linux-btrfs
On Wed, Oct 02, 2019 at 02:07:50PM +0300, Nikolay Borisov wrote:
> > +struct list_head * __attribute_const__ btrfs_get_fs_uuids(void)
>
> I'm not entirely sure this function is cons. According to the manual:
>
> Calls to functions whose return value is not affected by changes to the
> observable state of the program and that have no observable effects on
> such state other than to return a value may lend themselves to
> optimizations such as common subexpression elimination.
>
> The const attribute prohibits a function from reading objects that
> affect its return value between successive invocations. However,
> functions declared with the attribute can safely read objects that do
> not change their return value, such as non-volatile constants.
>
>
> My doubt stems from the fact this function actually references outside
> memory, namely gets the ptr to fs_uuids. There is a specific remark not
> to use const when the function takes a ptr argument but it doesn't say
> anything when getting a ptr from a global var.
The fs_uuids are a non-volatile constant. It does not change accross the
lifetime of the module, so all code executed will always see the same
value.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-04 11:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 17:57 [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
2019-10-01 17:57 ` [PATCH 1/3] btrfs: add __cold attribute to more functions David Sterba
2019-10-02 10:52 ` Nikolay Borisov
2019-10-04 10:56 ` David Sterba
2019-10-01 17:57 ` [PATCH 2/3] btrfs: add const function attribute David Sterba
2019-10-02 11:07 ` Nikolay Borisov
2019-10-04 11:01 ` David Sterba
2019-10-01 17:57 ` [PATCH 3/3] btrfs: add __pure attribute to functions David Sterba
2019-10-02 11:09 ` Nikolay Borisov
2019-10-01 17:57 ` [PATCH 0/3] Coldify, constify, purify (function attributes) David Sterba
2019-10-02 12:20 ` Nikolay Borisov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).