Both in Case 2 and 3, we exchange n and s. This mean no matter whether
child2 is NULL or not, successor's parent should be assigned to node's.
This patch takes this step out to make it explicit and reduce the
ambiguity.
Besides, this step reduces some symbol size like rb_erase().
KERN_CONFIG upstream patched
OPT_FOR_PERF 877 870
OPT_FOR_SIZE 635 621
Signed-off-by: Wei Yang <[email protected]>
---
include/linux/rbtree_augmented.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
index fdd421b8d9ae..99c42e1a74b8 100644
--- a/include/linux/rbtree_augmented.h
+++ b/include/linux/rbtree_augmented.h
@@ -283,14 +283,13 @@ __rb_erase_augmented(struct rb_node *node, struct rb_root *root,
__rb_change_child(node, successor, tmp, root);
if (child2) {
- successor->__rb_parent_color = pc;
rb_set_parent_color(child2, parent, RB_BLACK);
rebalance = NULL;
} else {
unsigned long pc2 = successor->__rb_parent_color;
- successor->__rb_parent_color = pc;
rebalance = __rb_is_black(pc2) ? parent : NULL;
}
+ successor->__rb_parent_color = pc;
tmp = successor;
}
--
2.17.1
Code looks fine, for both commits in this series. Please make sure to
double check that lib/rbtree_test does not show any performance
regressions, but assuming they don't, looks great !
Reviewed-By: Michel Lespinasse <[email protected]>
On Sun, Oct 27, 2019 at 7:15 PM Wei Yang <[email protected]> wrote:
>
> Both in Case 2 and 3, we exchange n and s. This mean no matter whether
> child2 is NULL or not, successor's parent should be assigned to node's.
>
> This patch takes this step out to make it explicit and reduce the
> ambiguity.
>
> Besides, this step reduces some symbol size like rb_erase().
>
> KERN_CONFIG upstream patched
> OPT_FOR_PERF 877 870
> OPT_FOR_SIZE 635 621
>
> Signed-off-by: Wei Yang <[email protected]>
> ---
> include/linux/rbtree_augmented.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
> index fdd421b8d9ae..99c42e1a74b8 100644
> --- a/include/linux/rbtree_augmented.h
> +++ b/include/linux/rbtree_augmented.h
> @@ -283,14 +283,13 @@ __rb_erase_augmented(struct rb_node *node, struct rb_root *root,
> __rb_change_child(node, successor, tmp, root);
>
> if (child2) {
> - successor->__rb_parent_color = pc;
> rb_set_parent_color(child2, parent, RB_BLACK);
> rebalance = NULL;
> } else {
> unsigned long pc2 = successor->__rb_parent_color;
> - successor->__rb_parent_color = pc;
> rebalance = __rb_is_black(pc2) ? parent : NULL;
> }
> + successor->__rb_parent_color = pc;
> tmp = successor;
> }
>
> --
> 2.17.1
>
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
After move parent assignment out, we can check the color directly.
Signed-off-by: Wei Yang <[email protected]>
---
include/linux/rbtree_augmented.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
index 99c42e1a74b8..724b0d036b57 100644
--- a/include/linux/rbtree_augmented.h
+++ b/include/linux/rbtree_augmented.h
@@ -286,8 +286,7 @@ __rb_erase_augmented(struct rb_node *node, struct rb_root *root,
rb_set_parent_color(child2, parent, RB_BLACK);
rebalance = NULL;
} else {
- unsigned long pc2 = successor->__rb_parent_color;
- rebalance = __rb_is_black(pc2) ? parent : NULL;
+ rebalance = rb_is_black(successor) ? parent : NULL;
}
successor->__rb_parent_color = pc;
tmp = successor;
--
2.17.1
On Sun, Oct 27, 2019 at 09:29:43PM -0700, Michel Lespinasse wrote:
>Code looks fine, for both commits in this series. Please make sure to
>double check that lib/rbtree_test does not show any performance
>regressions, but assuming they don't, looks great !
>
Thanks :-)
>Reviewed-By: Michel Lespinasse <[email protected]>
>
>On Sun, Oct 27, 2019 at 7:15 PM Wei Yang <[email protected]> wrote:
>>
>> Both in Case 2 and 3, we exchange n and s. This mean no matter whether
>> child2 is NULL or not, successor's parent should be assigned to node's.
>>
>> This patch takes this step out to make it explicit and reduce the
>> ambiguity.
>>
>> Besides, this step reduces some symbol size like rb_erase().
>>
>> KERN_CONFIG upstream patched
>> OPT_FOR_PERF 877 870
>> OPT_FOR_SIZE 635 621
>>
>> Signed-off-by: Wei Yang <[email protected]>
>> ---
>> include/linux/rbtree_augmented.h | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
>> index fdd421b8d9ae..99c42e1a74b8 100644
>> --- a/include/linux/rbtree_augmented.h
>> +++ b/include/linux/rbtree_augmented.h
>> @@ -283,14 +283,13 @@ __rb_erase_augmented(struct rb_node *node, struct rb_root *root,
>> __rb_change_child(node, successor, tmp, root);
>>
>> if (child2) {
>> - successor->__rb_parent_color = pc;
>> rb_set_parent_color(child2, parent, RB_BLACK);
>> rebalance = NULL;
>> } else {
>> unsigned long pc2 = successor->__rb_parent_color;
>> - successor->__rb_parent_color = pc;
>> rebalance = __rb_is_black(pc2) ? parent : NULL;
>> }
>> + successor->__rb_parent_color = pc;
>> tmp = successor;
>> }
>>
>> --
>> 2.17.1
>>
>
>
>--
>Michel "Walken" Lespinasse
>A program is never fully debugged until the last user dies.
--
Wei Yang
Help you, Help me
On Mon, Oct 28, 2019 at 10:14:41AM +0800, Wei Yang wrote:
> Both in Case 2 and 3, we exchange n and s. This mean no matter whether
> child2 is NULL or not, successor's parent should be assigned to node's.
>
> This patch takes this step out to make it explicit and reduce the
> ambiguity.
>
> Besides, this step reduces some symbol size like rb_erase().
>
> KERN_CONFIG upstream patched
> OPT_FOR_PERF 877 870
> OPT_FOR_SIZE 635 621
>
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
On Mon, Oct 28, 2019 at 10:14:42AM +0800, Wei Yang wrote:
> After move parent assignment out, we can check the color directly.
>
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>