Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Wed, 26 Sep 2012 17:34:11 -0500 Daniel Santos wrote: > Sorry to resurrect the dead here, but I'm playing catch-up and this > looks important. > > On 08/20/2012 05:17 PM, Andrew Morton wrote: > > I'm inclined to agree with Peter here - "inline" is now a vague, > > pathetic and useless thing. The problem is that the reader just > > doesn't *know* whether or not the writer really wanted it to be > > inlined. > > > > If we have carefully made a decision to inline a function, we should > > (now) use __always_inline. > Are we all aware here that __always_inline (a.k.a. > "__attribute__((always_inline))") just means "inline even when not > optimizing"? This appears to be a very common misunderstanding (unless > the gcc docs are wrong, see > http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bflatten_007d-function-attribute-2512). > > If you want to *force* gcc to inline a function (when inlining is > enabled), you can currently only do it from the calling function by > adding the |flatten attribute to it, which I have proposed adding here: > https://lkml.org/lkml/2012/9/25/643. > > Thus, all of the __always_inline markings we have in the kernel only > affect unoptimized builds (and maybe -O1?). If we need this feature > (and I think it would be darned handy!) we'll have to work on gcc to get it. When I replace the four __always_inline's in fs/namei.c with "inline", namei.o's .text shrinks 2kbytes (gcc-4.4.4), so __always_inline does allear to be doing what we think it does? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
Sorry to resurrect the dead here, but I'm playing catch-up and this looks important. On 08/20/2012 05:17 PM, Andrew Morton wrote: > I'm inclined to agree with Peter here - "inline" is now a vague, > pathetic and useless thing. The problem is that the reader just > doesn't *know* whether or not the writer really wanted it to be > inlined. > > If we have carefully made a decision to inline a function, we should > (now) use __always_inline. Are we all aware here that __always_inline (a.k.a. "__attribute__((always_inline))") just means "inline even when not optimizing"? This appears to be a very common misunderstanding (unless the gcc docs are wrong, see http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bflatten_007d-function-attribute-2512). If you want to *force* gcc to inline a function (when inlining is enabled), you can currently only do it from the calling function by adding the |flatten attribute to it, which I have proposed adding here: https://lkml.org/lkml/2012/9/25/643. Thus, all of the __always_inline markings we have in the kernel only affect unoptimized builds (and maybe -O1?). If we need this feature (and I think it would be darned handy!) we'll have to work on gcc to get it. Daniel | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
Sorry to resurrect the dead here, but I'm playing catch-up and this looks important. On 08/20/2012 05:17 PM, Andrew Morton wrote: I'm inclined to agree with Peter here - inline is now a vague, pathetic and useless thing. The problem is that the reader just doesn't *know* whether or not the writer really wanted it to be inlined. If we have carefully made a decision to inline a function, we should (now) use __always_inline. Are we all aware here that __always_inline (a.k.a. __attribute__((always_inline))) just means inline even when not optimizing? This appears to be a very common misunderstanding (unless the gcc docs are wrong, see http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bflatten_007d-function-attribute-2512). If you want to *force* gcc to inline a function (when inlining is enabled), you can currently only do it from the calling function by adding the |flatten attribute to it, which I have proposed adding here: https://lkml.org/lkml/2012/9/25/643. Thus, all of the __always_inline markings we have in the kernel only affect unoptimized builds (and maybe -O1?). If we need this feature (and I think it would be darned handy!) we'll have to work on gcc to get it. Daniel | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Wed, 26 Sep 2012 17:34:11 -0500 Daniel Santos danielfsan...@att.net wrote: Sorry to resurrect the dead here, but I'm playing catch-up and this looks important. On 08/20/2012 05:17 PM, Andrew Morton wrote: I'm inclined to agree with Peter here - inline is now a vague, pathetic and useless thing. The problem is that the reader just doesn't *know* whether or not the writer really wanted it to be inlined. If we have carefully made a decision to inline a function, we should (now) use __always_inline. Are we all aware here that __always_inline (a.k.a. __attribute__((always_inline))) just means inline even when not optimizing? This appears to be a very common misunderstanding (unless the gcc docs are wrong, see http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bflatten_007d-function-attribute-2512). If you want to *force* gcc to inline a function (when inlining is enabled), you can currently only do it from the calling function by adding the |flatten attribute to it, which I have proposed adding here: https://lkml.org/lkml/2012/9/25/643. Thus, all of the __always_inline markings we have in the kernel only affect unoptimized builds (and maybe -O1?). If we need this feature (and I think it would be darned handy!) we'll have to work on gcc to get it. When I replace the four __always_inline's in fs/namei.c with inline, namei.o's .text shrinks 2kbytes (gcc-4.4.4), so __always_inline does allear to be doing what we think it does? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Thu, 2012-08-23 at 14:05 +0200, Jan Engelhardt wrote: > The current use of "inline" is to shut up the compiler, otherwise gcc > would emit a warning about "function declared but not used". Don't we have __maybe_unused for that? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Tuesday 2012-08-21 00:17, Andrew Morton wrote: > >If we have carefully made a decision to inline a function, we should >(now) use __always_inline. >If we have carefully made a decision to not inline a function, we >should use noinline. > >If we don't care, we should omit all such markings. >This leaves no place for "inline"? The current use of "inline" is to shut up the compiler, otherwise gcc would emit a warning about "function declared but not used". -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Tuesday 2012-08-21 00:17, Andrew Morton wrote: If we have carefully made a decision to inline a function, we should (now) use __always_inline. If we have carefully made a decision to not inline a function, we should use noinline. If we don't care, we should omit all such markings. This leaves no place for inline? The current use of inline is to shut up the compiler, otherwise gcc would emit a warning about function declared but not used. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Thu, 2012-08-23 at 14:05 +0200, Jan Engelhardt wrote: The current use of inline is to shut up the compiler, otherwise gcc would emit a warning about function declared but not used. Don't we have __maybe_unused for that? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Mon, 20 Aug 2012 15:05:24 -0700
Michel Lespinasse wrote:
> Add __rb_change_child() as an inline helper function to replace code that
> would otherwise be duplicated 4 times in the source.
>
> No changes to binary size or speed.
>
> ...
>
> --- a/lib/rbtree.c
> +++ b/lib/rbtree.c
> @@ -66,6 +66,19 @@ static inline struct rb_node *rb_red_parent(struct rb_node
> *red)
> return (struct rb_node *)red->__rb_parent_color;
> }
>
> +static inline void
> +__rb_change_child(struct rb_node *old, struct rb_node *new,
> + struct rb_node *parent, struct rb_root *root)
> +{
> + if (parent) {
> + if (parent->rb_left == old)
> + parent->rb_left = new;
> + else
> + parent->rb_right = new;
> + } else
> + root->rb_node = new;
> +}
I'm inclined to agree with Peter here - "inline" is now a vague,
pathetic and useless thing. The problem is that the reader just
doesn't *know* whether or not the writer really wanted it to be
inlined.
If we have carefully made a decision to inline a function, we should
(now) use __always_inline.
If we have carefully made a decision to not inline a function, we
should use noinline.
If we don't care, we should omit all such markings.
This leaves no place for "inline"?
Marking it noinline shrinks the text by 60-odd bytes. Given the number
of args, my gut feel is that this will be slower, despite the cache
benefit. But that might be wrong.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 2/9] rbtree: add __rb_change_child() helper function
Add __rb_change_child() as an inline helper function to replace code that
would otherwise be duplicated 4 times in the source.
No changes to binary size or speed.
Signed-off-by: Michel Lespinasse
Reviewed-by: Rik van Riel
---
lib/rbtree.c | 46 +-
1 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/lib/rbtree.c b/lib/rbtree.c
index 61cdd0e3e538..de89a614b1ba 100644
--- a/lib/rbtree.c
+++ b/lib/rbtree.c
@@ -66,6 +66,19 @@ static inline struct rb_node *rb_red_parent(struct rb_node
*red)
return (struct rb_node *)red->__rb_parent_color;
}
+static inline void
+__rb_change_child(struct rb_node *old, struct rb_node *new,
+ struct rb_node *parent, struct rb_root *root)
+{
+ if (parent) {
+ if (parent->rb_left == old)
+ parent->rb_left = new;
+ else
+ parent->rb_right = new;
+ } else
+ root->rb_node = new;
+}
+
/*
* Helper function for rotations:
* - old's parent and color get assigned to new
@@ -78,13 +91,7 @@ __rb_rotate_set_parents(struct rb_node *old, struct rb_node
*new,
struct rb_node *parent = rb_parent(old);
new->__rb_parent_color = old->__rb_parent_color;
rb_set_parent_color(old, new, color);
- if (parent) {
- if (parent->rb_left == old)
- parent->rb_left = new;
- else
- parent->rb_right = new;
- } else
- root->rb_node = new;
+ __rb_change_child(old, new, parent, root);
}
void rb_insert_color(struct rb_node *node, struct rb_root *root)
@@ -375,13 +382,7 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
while ((left = node->rb_left) != NULL)
node = left;
- if (rb_parent(old)) {
- if (rb_parent(old)->rb_left == old)
- rb_parent(old)->rb_left = node;
- else
- rb_parent(old)->rb_right = node;
- } else
- root->rb_node = node;
+ __rb_change_child(old, node, rb_parent(old), root);
child = node->rb_right;
parent = rb_parent(node);
@@ -410,13 +411,7 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
if (child)
rb_set_parent(child, parent);
- if (parent) {
- if (parent->rb_left == node)
- parent->rb_left = child;
- else
- parent->rb_right = child;
- } else
- root->rb_node = child;
+ __rb_change_child(node, child, parent, root);
color:
if (color == RB_BLACK)
@@ -591,14 +586,7 @@ void rb_replace_node(struct rb_node *victim, struct
rb_node *new,
struct rb_node *parent = rb_parent(victim);
/* Set the surrounding nodes to point to the replacement */
- if (parent) {
- if (victim == parent->rb_left)
- parent->rb_left = new;
- else
- parent->rb_right = new;
- } else {
- root->rb_node = new;
- }
+ __rb_change_child(victim, new, parent, root);
if (victim->rb_left)
rb_set_parent(victim->rb_left, new);
if (victim->rb_right)
--
1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 2/9] rbtree: add __rb_change_child() helper function
Add __rb_change_child() as an inline helper function to replace code that
would otherwise be duplicated 4 times in the source.
No changes to binary size or speed.
Signed-off-by: Michel Lespinasse wal...@google.com
Reviewed-by: Rik van Riel r...@redhat.com
---
lib/rbtree.c | 46 +-
1 files changed, 17 insertions(+), 29 deletions(-)
diff --git a/lib/rbtree.c b/lib/rbtree.c
index 61cdd0e3e538..de89a614b1ba 100644
--- a/lib/rbtree.c
+++ b/lib/rbtree.c
@@ -66,6 +66,19 @@ static inline struct rb_node *rb_red_parent(struct rb_node
*red)
return (struct rb_node *)red-__rb_parent_color;
}
+static inline void
+__rb_change_child(struct rb_node *old, struct rb_node *new,
+ struct rb_node *parent, struct rb_root *root)
+{
+ if (parent) {
+ if (parent-rb_left == old)
+ parent-rb_left = new;
+ else
+ parent-rb_right = new;
+ } else
+ root-rb_node = new;
+}
+
/*
* Helper function for rotations:
* - old's parent and color get assigned to new
@@ -78,13 +91,7 @@ __rb_rotate_set_parents(struct rb_node *old, struct rb_node
*new,
struct rb_node *parent = rb_parent(old);
new-__rb_parent_color = old-__rb_parent_color;
rb_set_parent_color(old, new, color);
- if (parent) {
- if (parent-rb_left == old)
- parent-rb_left = new;
- else
- parent-rb_right = new;
- } else
- root-rb_node = new;
+ __rb_change_child(old, new, parent, root);
}
void rb_insert_color(struct rb_node *node, struct rb_root *root)
@@ -375,13 +382,7 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
while ((left = node-rb_left) != NULL)
node = left;
- if (rb_parent(old)) {
- if (rb_parent(old)-rb_left == old)
- rb_parent(old)-rb_left = node;
- else
- rb_parent(old)-rb_right = node;
- } else
- root-rb_node = node;
+ __rb_change_child(old, node, rb_parent(old), root);
child = node-rb_right;
parent = rb_parent(node);
@@ -410,13 +411,7 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
if (child)
rb_set_parent(child, parent);
- if (parent) {
- if (parent-rb_left == node)
- parent-rb_left = child;
- else
- parent-rb_right = child;
- } else
- root-rb_node = child;
+ __rb_change_child(node, child, parent, root);
color:
if (color == RB_BLACK)
@@ -591,14 +586,7 @@ void rb_replace_node(struct rb_node *victim, struct
rb_node *new,
struct rb_node *parent = rb_parent(victim);
/* Set the surrounding nodes to point to the replacement */
- if (parent) {
- if (victim == parent-rb_left)
- parent-rb_left = new;
- else
- parent-rb_right = new;
- } else {
- root-rb_node = new;
- }
+ __rb_change_child(victim, new, parent, root);
if (victim-rb_left)
rb_set_parent(victim-rb_left, new);
if (victim-rb_right)
--
1.7.7.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/9] rbtree: add __rb_change_child() helper function
On Mon, 20 Aug 2012 15:05:24 -0700
Michel Lespinasse wal...@google.com wrote:
Add __rb_change_child() as an inline helper function to replace code that
would otherwise be duplicated 4 times in the source.
No changes to binary size or speed.
...
--- a/lib/rbtree.c
+++ b/lib/rbtree.c
@@ -66,6 +66,19 @@ static inline struct rb_node *rb_red_parent(struct rb_node
*red)
return (struct rb_node *)red-__rb_parent_color;
}
+static inline void
+__rb_change_child(struct rb_node *old, struct rb_node *new,
+ struct rb_node *parent, struct rb_root *root)
+{
+ if (parent) {
+ if (parent-rb_left == old)
+ parent-rb_left = new;
+ else
+ parent-rb_right = new;
+ } else
+ root-rb_node = new;
+}
I'm inclined to agree with Peter here - inline is now a vague,
pathetic and useless thing. The problem is that the reader just
doesn't *know* whether or not the writer really wanted it to be
inlined.
If we have carefully made a decision to inline a function, we should
(now) use __always_inline.
If we have carefully made a decision to not inline a function, we
should use noinline.
If we don't care, we should omit all such markings.
This leaves no place for inline?
Marking it noinline shrinks the text by 60-odd bytes. Given the number
of args, my gut feel is that this will be slower, despite the cache
benefit. But that might be wrong.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
