All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] Btrfs: if else cleanups
@ 2017-06-07  0:58 Timofey Titovets
  2017-06-07  0:58 ` [PATCH 1/7] Btrfs: __compare_inode_defrag decrease max compare count Timofey Titovets
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

Most of patches regroup if else logic
in attemp to avoid useless checks

Last patch convert if else to switch case,
because in that place code work with enum
and usage of switch case can make more obvious
to compiler how to optimize that code

This is if else vs case in GCC C++,
but i think gcc do the same with C.
https://godbolt.org/g/YzPL93

Timofey Titovets (7):
  Btrfs: __compare_inode_defrag decrease max compare count
  Btrfs: backref_comp decrease max compare count
  Btrfs: ref_node_cmp decrease max compare count
  Btrfs: ref_tree_add remove useless compare
  Btrfs: add_all_parents skip compare
  Btrfs: __tree_mod_log_insert decrease max compare count
  Btrfs: end_workqueue_bio use switch case instead of else if

 fs/btrfs/backref.c | 36 ++++++++++++++++++++----------------
 fs/btrfs/ctree.c   | 20 +++++++++++---------
 fs/btrfs/disk-io.c | 17 +++++++++++++----
 fs/btrfs/file.c    | 17 +++++++++--------
 fs/btrfs/inode.c   | 21 ++++++++++++---------
 5 files changed, 65 insertions(+), 46 deletions(-)

--
2.13.0

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/7] Btrfs: __compare_inode_defrag decrease max compare count
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07  0:58 ` [PATCH 2/7] Btrfs: backref_comp " Timofey Titovets
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

In worst case code do 4 comparison,
just add some new checks to switch check branch faster
now in worst case code do 3 comparison

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/file.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index da1096eb1..39efda26c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -71,16 +71,17 @@ struct inode_defrag {
 static int __compare_inode_defrag(struct inode_defrag *defrag1,
 				  struct inode_defrag *defrag2)
 {
-	if (defrag1->root > defrag2->root)
-		return 1;
-	else if (defrag1->root < defrag2->root)
+	if (defrag1->root != defrag2->root) {
+		if (defrag1->root > defrag2->root)
+			return 1;
 		return -1;
-	else if (defrag1->ino > defrag2->ino)
-		return 1;
-	else if (defrag1->ino < defrag2->ino)
+	}
+	if (defrag1->ino != defrag2->ino) {
+		if (defrag1->ino > defrag2->ino)
+			return 1;
 		return -1;
-	else
-		return 0;
+	}
+	return 0;
 }
 
 /* pop a record for an inode into the defrag tree.  The lock
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/7] Btrfs: backref_comp decrease max compare count
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
  2017-06-07  0:58 ` [PATCH 1/7] Btrfs: __compare_inode_defrag decrease max compare count Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07  0:58 ` [PATCH 3/7] Btrfs: ref_node_cmp " Timofey Titovets
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

In worst case code do 6 comparison,
just add some new checks to switch check branch faster
now in worst case code do 4 comparison

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/inode.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 17cbe9306..fa024642a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2249,20 +2249,23 @@ struct new_sa_defrag_extent {
 static int backref_comp(struct sa_defrag_extent_backref *b1,
 			struct sa_defrag_extent_backref *b2)
 {
-	if (b1->root_id < b2->root_id)
-		return -1;
-	else if (b1->root_id > b2->root_id)
+	if (b1->root_id != b2->root_id) {
+		if (b1->root_id < b2->root_id)
+			return -1;
 		return 1;
+	}
 
-	if (b1->inum < b2->inum)
-		return -1;
-	else if (b1->inum > b2->inum)
+	if (b1->inum != b2->inum) {
+		if (b1->inum < b2->inum)
+			return -1;
 		return 1;
+	}
 
-	if (b1->file_pos < b2->file_pos)
-		return -1;
-	else if (b1->file_pos > b2->file_pos)
+	if (b1->file_pos != b2->file_pos) {
+		if (b1->file_pos < b2->file_pos)
+			return -1;
 		return 1;
+	}
 
 	/*
 	 * [------------------------------] ===> (a range of space)
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/7] Btrfs: ref_node_cmp decrease max compare count
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
  2017-06-07  0:58 ` [PATCH 1/7] Btrfs: __compare_inode_defrag decrease max compare count Timofey Titovets
  2017-06-07  0:58 ` [PATCH 2/7] Btrfs: backref_comp " Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07  0:58 ` [PATCH 4/7] Btrfs: ref_tree_add remove useless compare Timofey Titovets
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

In worst case code do 8 comparison,
just add some new checks to switch check branch faster
now in worst case code do 5 comparison

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/backref.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 24865da63..2e4709e0c 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -120,25 +120,29 @@ static void ref_root_free(struct ref_root *ref_tree)
  */
 static int ref_node_cmp(struct ref_node *a, struct ref_node *b)
 {
-	if (a->root_id < b->root_id)
-		return -1;
-	else if (a->root_id > b->root_id)
+	if (a->root_id != b->root_id) {
+		if (a->root_id < b->root_id)
+			return -1;
 		return 1;
+	}
 
-	if (a->object_id < b->object_id)
-		return -1;
-	else if (a->object_id > b->object_id)
+	if (a->object_id != b->object_id) {
+		if (a->object_id < b->object_id)
+			return -1;
 		return 1;
+	}
 
-	if (a->offset < b->offset)
-		return -1;
-	else if (a->offset > b->offset)
+	if (a->offset != b->offset) {
+		if (a->offset < b->offset)
+			return -1;
 		return 1;
+	}
 
-	if (a->parent < b->parent)
-		return -1;
-	else if (a->parent > b->parent)
+	if (a->parent != b->parent) {
+		if (a->parent < b->parent)
+			return -1;
 		return 1;
+	}
 
 	return 0;
 }
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/7] Btrfs: ref_tree_add remove useless compare
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
                   ` (2 preceding siblings ...)
  2017-06-07  0:58 ` [PATCH 3/7] Btrfs: ref_node_cmp " Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07  0:58 ` [PATCH 5/7] Btrfs: add_all_parents skip compare Timofey Titovets
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

As code already know that (node->ref_mod > 0)
else if (node->ref_mod <= 0) - useless
So just leave 'else'

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/backref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 2e4709e0c..897d664a9 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -294,7 +294,7 @@ static int ref_tree_add(struct ref_root *ref_tree, u64 root_id, u64 object_id,
 
 	if (node->ref_mod > 0)
 		ref_tree->unique_refs += origin_count > 0 ? 0 : 1;
-	else if (node->ref_mod <= 0)
+	else
 		ref_tree->unique_refs += origin_count > 0 ? -1 : 0;
 
 	if (!node->ref_mod)
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/7] Btrfs: add_all_parents skip compare
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
                   ` (3 preceding siblings ...)
  2017-06-07  0:58 ` [PATCH 4/7] Btrfs: ref_tree_add remove useless compare Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07  0:58 ` [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count Timofey Titovets
  2017-06-07  0:58 ` [PATCH 7/7] Btrfs: end_workqueue_bio use switch case instead of else if Timofey Titovets
  6 siblings, 0 replies; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

By comparison logic if ret => 0 -> ret=0;
So lets cleanup compare logic

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/backref.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 897d664a9..f53045891 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -592,10 +592,10 @@ static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path,
 			ret = btrfs_next_old_item(root, path, time_seq);
 	}
 
-	if (ret > 0)
-		ret = 0;
-	else if (ret < 0)
+	if (ret < 0)
 		free_inode_elem_list(eie);
+	else
+		ret = 0;
 	return ret;
 }
 
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
                   ` (4 preceding siblings ...)
  2017-06-07  0:58 ` [PATCH 5/7] Btrfs: add_all_parents skip compare Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07  9:45   ` Filipe Manana
  2017-06-07  0:58 ` [PATCH 7/7] Btrfs: end_workqueue_bio use switch case instead of else if Timofey Titovets
  6 siblings, 1 reply; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

In worst case code do 4 comparison,
just add some new checks to switch check branch faster
now in worst case code do 3 comparison

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/ctree.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a3a75f1de..318379531 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -460,15 +460,17 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
 	while (*new) {
 		cur = rb_entry(*new, struct tree_mod_elem, node);
 		parent = *new;
-		if (cur->logical < tm->logical)
-			new = &((*new)->rb_left);
-		else if (cur->logical > tm->logical)
-			new = &((*new)->rb_right);
-		else if (cur->seq < tm->seq)
-			new = &((*new)->rb_left);
-		else if (cur->seq > tm->seq)
-			new = &((*new)->rb_right);
-		else
+		if (cur->logical != tm->logical) {
+			if (cur->logical < tm->logical)
+				new = &((*new)->rb_left);
+			else
+				new = &((*new)->rb_right);
+		} else if (cur->seq != tm->seq) {
+			if (cur->seq < tm->seq)
+				new = &((*new)->rb_left);
+			else
+				new = &((*new)->rb_right);
+		} else
 			return -EEXIST;
 	}
 
-- 
2.13.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 7/7] Btrfs: end_workqueue_bio use switch case instead of else if
  2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
                   ` (5 preceding siblings ...)
  2017-06-07  0:58 ` [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count Timofey Titovets
@ 2017-06-07  0:58 ` Timofey Titovets
  2017-06-07 22:48   ` kbuild test robot
  6 siblings, 1 reply; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07  0:58 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Timofey Titovets

If arg to "switch case" is determined and it's a consecutive numbers
(This is enum btrfs_wq_endio_type)
Compiler can create jump table to optimize logic

Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
---
 fs/btrfs/disk-io.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8685d6718..72208826d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -802,18 +802,27 @@ static void end_workqueue_bio(struct bio *bio)
 	end_io_wq->error = bio->bi_error;

 	if (bio_op(bio) == REQ_OP_WRITE) {
-		if (end_io_wq->metadata == BTRFS_WQ_ENDIO_METADATA) {
+		switch (end_io_wq->metadata) {
+		case BTRFS_WQ_ENDIO_DATA:
+			wq = fs_info->endio_write_workers;
+			func = btrfs_endio_write_helper;
+			break;
+		case BTRFS_WQ_ENDIO_METADATA:
 			wq = fs_info->endio_meta_write_workers;
 			func = btrfs_endio_meta_write_helper;
-		} else if (end_io_wq->metadata == BTRFS_WQ_ENDIO_FREE_SPACE) {
+			break;
+		case BTRFS_WQ_ENDIO_FREE_SPACE:
 			wq = fs_info->endio_freespace_worker;
 			func = btrfs_freespace_write_helper;
-		} else if (end_io_wq->metadata == BTRFS_WQ_ENDIO_RAID56) {
+			break;
+		case BTRFS_WQ_ENDIO_RAID56:
 			wq = fs_info->endio_raid56_workers;
 			func = btrfs_endio_raid56_helper;
-		} else {
+			break;
+		case BTRFS_WQ_ENDIO_DIO_REPAIR:
 			wq = fs_info->endio_write_workers;
 			func = btrfs_endio_write_helper;
+			break;
 		}
 	} else {
 		if (unlikely(end_io_wq->metadata ==
--
2.13.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count
  2017-06-07  0:58 ` [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count Timofey Titovets
@ 2017-06-07  9:45   ` Filipe Manana
  2017-06-07 11:01     ` Timofey Titovets
  0 siblings, 1 reply; 12+ messages in thread
From: Filipe Manana @ 2017-06-07  9:45 UTC (permalink / raw)
  To: Timofey Titovets; +Cc: linux-btrfs

On Wed, Jun 7, 2017 at 1:58 AM, Timofey Titovets <nefelim4ag@gmail.com> wrote:
> In worst case code do 4 comparison,
> just add some new checks to switch check branch faster
> now in worst case code do 3 comparison

Some comments below.

>
> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
> ---
>  fs/btrfs/ctree.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a3a75f1de..318379531 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -460,15 +460,17 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
>         while (*new) {
>                 cur = rb_entry(*new, struct tree_mod_elem, node);
>                 parent = *new;
> -               if (cur->logical < tm->logical)
> -                       new = &((*new)->rb_left);
> -               else if (cur->logical > tm->logical)
> -                       new = &((*new)->rb_right);
> -               else if (cur->seq < tm->seq)
> -                       new = &((*new)->rb_left);
> -               else if (cur->seq > tm->seq)
> -                       new = &((*new)->rb_right);
> -               else
> +               if (cur->logical != tm->logical) {
> +                       if (cur->logical < tm->logical)
> +                               new = &((*new)->rb_left);

So now when cur->logical is less then tm->logical, we do 2 comparisons
instead of 1.
Is this case much less common always? If it were it could be a good change.
What guarantees that this change will give better performance for
common workloads? What guarantees you the example I just gave isn't
typical for most/many common workloads?

And most importantly, did you actually do some benchmarks that justify
this change? Is this a hot code path that would benefit from such
changes?
(Since it's the tree mod log, seldom used, I really doubt it).

Same comments apply to all the other similar patches you have sent.

thanks

> +                       else
> +                               new = &((*new)->rb_right);
> +               } else if (cur->seq != tm->seq) {
> +                       if (cur->seq < tm->seq)
> +                               new = &((*new)->rb_left);
> +                       else
> +                               new = &((*new)->rb_right);
> +               } else
>                         return -EEXIST;
>         }
>
> --
> 2.13.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count
  2017-06-07  9:45   ` Filipe Manana
@ 2017-06-07 11:01     ` Timofey Titovets
  2017-06-13 14:24       ` David Sterba
  0 siblings, 1 reply; 12+ messages in thread
From: Timofey Titovets @ 2017-06-07 11:01 UTC (permalink / raw)
  To: Filipe Manana; +Cc: linux-btrfs

2017-06-07 12:45 GMT+03:00 Filipe Manana <fdmanana@gmail.com>:
> On Wed, Jun 7, 2017 at 1:58 AM, Timofey Titovets <nefelim4ag@gmail.com> wrote:
>> In worst case code do 4 comparison,
>> just add some new checks to switch check branch faster
>> now in worst case code do 3 comparison
>
> Some comments below.
>
>>
>> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
>> ---
>>  fs/btrfs/ctree.c | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index a3a75f1de..318379531 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -460,15 +460,17 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
>>         while (*new) {
>>                 cur = rb_entry(*new, struct tree_mod_elem, node);
>>                 parent = *new;
>> -               if (cur->logical < tm->logical)
>> -                       new = &((*new)->rb_left);
>> -               else if (cur->logical > tm->logical)
>> -                       new = &((*new)->rb_right);
>> -               else if (cur->seq < tm->seq)
>> -                       new = &((*new)->rb_left);
>> -               else if (cur->seq > tm->seq)
>> -                       new = &((*new)->rb_right);
>> -               else
>> +               if (cur->logical != tm->logical) {
>> +                       if (cur->logical < tm->logical)
>> +                               new = &((*new)->rb_left);
>
> So now when cur->logical is less then tm->logical, we do 2 comparisons
> instead of 1.
> Is this case much less common always? If it were it could be a good change.
> What guarantees that this change will give better performance for
> common workloads? What guarantees you the example I just gave isn't
> typical for most/many common workloads?
>
> And most importantly, did you actually do some benchmarks that justify
> this change? Is this a hot code path that would benefit from such
> changes?
> (Since it's the tree mod log, seldom used, I really doubt it).
>
> Same comments apply to all the other similar patches you have sent.
>
> thanks
>

Nope, i just lack of expirience to do such benchmarks,
same for understand all btrfs code, it's difficult to say that path is hot/cold.

I just did reread all btrfs code, analize that i understand and trying
fix places that looks questionably.

In most cases i can explain why this can help (IMHO):
01 - __compare_inode_defrag - we compare inodes, by my expirience
in most distros and setups btrfs have 1-2 subvolumes (root_ids), so in
most time root_id1 == root_id2
02 - backref_comp - same
03 - ref_node_cmp - same
04 - ref_tree_add - just a cleanup, compiller do the same
https://godbolt.org/g/Ww93ws
05 - add_all_parents - just a cleanup, compiller do the same
https://godbolt.org/g/srSGeW
07 - end_workqueue_bio - if i understand correctly this code executed
on every write,
so by convertion to switch case will save several cpu cycles on every write

For that function (06 -  __tree_mod_log_insert):
i do the same as for 01-03 "by inertia" - as i just not know all code,
i just assume that as this fuction work with rb tree, values will be
equally distributed

Thanks
-- 
Have a nice day,
Timofey.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 7/7] Btrfs: end_workqueue_bio use switch case instead of else if
  2017-06-07  0:58 ` [PATCH 7/7] Btrfs: end_workqueue_bio use switch case instead of else if Timofey Titovets
@ 2017-06-07 22:48   ` kbuild test robot
  0 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2017-06-07 22:48 UTC (permalink / raw)
  To: Timofey Titovets; +Cc: kbuild-all, linux-btrfs, Timofey Titovets

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

Hi Timofey,

[auto build test WARNING on next-20170605]
[also build test WARNING on v4.12-rc4]
[cannot apply to btrfs/next v4.9-rc8 v4.9-rc7 v4.9-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Timofey-Titovets/Btrfs-if-else-cleanups/20170608-055223
config: x86_64-randconfig-x016-201723 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   fs//btrfs/disk-io.c: In function 'end_workqueue_bio':
>> fs//btrfs/disk-io.c:843:2: warning: 'func' may be used uninitialized in this function [-Wmaybe-uninitialized]
     btrfs_init_work(&end_io_wq->work, func, end_workqueue_fn, NULL, NULL);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> fs//btrfs/disk-io.c:844:2: warning: 'wq' may be used uninitialized in this function [-Wmaybe-uninitialized]
     btrfs_queue_work(wq, &end_io_wq->work);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/func +843 fs//btrfs/disk-io.c

9e0af237 Liu Bo      2014-08-15  837  		} else {
9e0af237 Liu Bo      2014-08-15  838  			wq = fs_info->endio_workers;
9e0af237 Liu Bo      2014-08-15  839  			func = btrfs_endio_helper;
d20f7043 Chris Mason 2008-12-08  840  		}
ce9adaa5 Chris Mason 2008-04-09  841  	}
ce9adaa5 Chris Mason 2008-04-09  842  
9e0af237 Liu Bo      2014-08-15 @843  	btrfs_init_work(&end_io_wq->work, func, end_workqueue_fn, NULL, NULL);
9e0af237 Liu Bo      2014-08-15 @844  	btrfs_queue_work(wq, &end_io_wq->work);
ce9adaa5 Chris Mason 2008-04-09  845  }
ce9adaa5 Chris Mason 2008-04-09  846  
22c59948 Chris Mason 2008-04-09  847  int btrfs_bio_wq_end_io(struct btrfs_fs_info *info, struct bio *bio,

:::::: The code at line 843 was first introduced by commit
:::::: 9e0af23764344f7f1b68e4eefbe7dc865018b63d Btrfs: fix task hang under heavy compressed write

:::::: TO: Liu Bo <bo.li.liu@oracle.com>
:::::: CC: Chris Mason <clm@fb.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24830 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count
  2017-06-07 11:01     ` Timofey Titovets
@ 2017-06-13 14:24       ` David Sterba
  0 siblings, 0 replies; 12+ messages in thread
From: David Sterba @ 2017-06-13 14:24 UTC (permalink / raw)
  To: Timofey Titovets; +Cc: Filipe Manana, linux-btrfs

On Wed, Jun 07, 2017 at 02:01:46PM +0300, Timofey Titovets wrote:
> 2017-06-07 12:45 GMT+03:00 Filipe Manana <fdmanana@gmail.com>:
> > On Wed, Jun 7, 2017 at 1:58 AM, Timofey Titovets <nefelim4ag@gmail.com> wrote:
> >> In worst case code do 4 comparison,
> >> just add some new checks to switch check branch faster
> >> now in worst case code do 3 comparison
> >
> > Some comments below.
> >
> >>
> >> Signed-off-by: Timofey Titovets <nefelim4ag@gmail.com>
> >> ---
> >>  fs/btrfs/ctree.c | 20 +++++++++++---------
> >>  1 file changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> >> index a3a75f1de..318379531 100644
> >> --- a/fs/btrfs/ctree.c
> >> +++ b/fs/btrfs/ctree.c
> >> @@ -460,15 +460,17 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
> >>         while (*new) {
> >>                 cur = rb_entry(*new, struct tree_mod_elem, node);
> >>                 parent = *new;
> >> -               if (cur->logical < tm->logical)
> >> -                       new = &((*new)->rb_left);
> >> -               else if (cur->logical > tm->logical)
> >> -                       new = &((*new)->rb_right);
> >> -               else if (cur->seq < tm->seq)
> >> -                       new = &((*new)->rb_left);
> >> -               else if (cur->seq > tm->seq)
> >> -                       new = &((*new)->rb_right);
> >> -               else
> >> +               if (cur->logical != tm->logical) {
> >> +                       if (cur->logical < tm->logical)
> >> +                               new = &((*new)->rb_left);
> >
> > So now when cur->logical is less then tm->logical, we do 2 comparisons
> > instead of 1.
> > Is this case much less common always? If it were it could be a good change.
> > What guarantees that this change will give better performance for
> > common workloads? What guarantees you the example I just gave isn't
> > typical for most/many common workloads?
> >
> > And most importantly, did you actually do some benchmarks that justify
> > this change? Is this a hot code path that would benefit from such
> > changes?
> > (Since it's the tree mod log, seldom used, I really doubt it).
> >
> > Same comments apply to all the other similar patches you have sent.
> 
> Nope, i just lack of expirience to do such benchmarks,
> same for understand all btrfs code, it's difficult to say that path is hot/cold.

So if you call the patchset cleanups and then claim some
performance-related improvements, you can expect to be called out to
provide numbers or an analysis.

I've checked the resulting assembly of "Btrfs: backref_comp decrease max
compare count" that does similar conversion of the sequence of ifs.
Surprise surprise, the assembly does not differ regarding number of
branches. This at mininimum can be done before any profiling to see if
this makes sense.

If the assembly stays the same, the code readability may be preferred,
which is one of the points of cleanup patches. In that case I prefer the
current code over your change, but it may be just personal preference
and because I'm used to reading the "linear ifs" pattern.

> I just did reread all btrfs code, analize that i understand and trying
> fix places that looks questionably.
> 
> In most cases i can explain why this can help (IMHO):
> 01 - __compare_inode_defrag - we compare inodes, by my expirience
> in most distros and setups btrfs have 1-2 subvolumes (root_ids), so in
> most time root_id1 == root_id2
> 02 - backref_comp - same
> 03 - ref_node_cmp - same
> 04 - ref_tree_add - just a cleanup, compiller do the same
> https://godbolt.org/g/Ww93ws
> 05 - add_all_parents - just a cleanup, compiller do the same
> https://godbolt.org/g/srSGeW
> 07 - end_workqueue_bio - if i understand correctly this code executed
> on every write,
> so by convertion to switch case will save several cpu cycles on every write
> 
> For that function (06 -  __tree_mod_log_insert):
> i do the same as for 01-03 "by inertia" - as i just not know all code,
> i just assume that as this fuction work with rb tree, values will be
> equally distributed

>From my point above, I'll not merge patches 1, 2, 3 and 6. Patches 4 and 5
seem ok.

Patch 7 does not result in the equivalent code, as reported by the
kbuild robot. But converting the ifs to switch is desired in
end_workqueue_bio, namely the second branch as it's not clear how are
some of the BTRFS_WQ_ENDIO_* values handled.

If you still care, please resend 4 and 5. The changelog does not need to
repeat the code, more describe that the change simplifies the code to
make it more readable.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-06-13 14:25 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-07  0:58 [PATCH 0/7] Btrfs: if else cleanups Timofey Titovets
2017-06-07  0:58 ` [PATCH 1/7] Btrfs: __compare_inode_defrag decrease max compare count Timofey Titovets
2017-06-07  0:58 ` [PATCH 2/7] Btrfs: backref_comp " Timofey Titovets
2017-06-07  0:58 ` [PATCH 3/7] Btrfs: ref_node_cmp " Timofey Titovets
2017-06-07  0:58 ` [PATCH 4/7] Btrfs: ref_tree_add remove useless compare Timofey Titovets
2017-06-07  0:58 ` [PATCH 5/7] Btrfs: add_all_parents skip compare Timofey Titovets
2017-06-07  0:58 ` [PATCH 6/7] Btrfs: __tree_mod_log_insert decrease max compare count Timofey Titovets
2017-06-07  9:45   ` Filipe Manana
2017-06-07 11:01     ` Timofey Titovets
2017-06-13 14:24       ` David Sterba
2017-06-07  0:58 ` [PATCH 7/7] Btrfs: end_workqueue_bio use switch case instead of else if Timofey Titovets
2017-06-07 22:48   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.