In kernel/params.c::module_attr_show and
kernel/params.c::module_attr_store we call to_module_kobject() and save
the result in a local variable right before a conditional statement that
does not depend on the call to to_module_kobject() and may cause the
function to return. If the function returns before we use the result of
to_module_kobject() then the call is just a waste of time.
The patch moves the call to to_module_kobject() down just before it is
actually used, thus we save a call to the function in a few cases. I doubt
this is in any way measurable, but I see no reason to not move the call -
it should be an infinitesimal improvement with no ill sideeffects.
Please consider applying.
Signed-off-by: Jesper Juhl <[email protected]>
diff -up linux-2.6.11-rc2-bk7-orig/kernel/params.c linux-2.6.11-rc2-bk7/kernel/params.c
--- linux-2.6.11-rc2-bk7-orig/kernel/params.c 2005-01-29 23:54:53.000000000 +0100
+++ linux-2.6.11-rc2-bk7/kernel/params.c 2005-01-30 00:27:08.000000000 +0100
@@ -625,11 +625,12 @@ static ssize_t module_attr_show(struct k
int ret;
attribute = to_module_attr(attr);
- mk = to_module_kobject(kobj);
if (!attribute->show)
return -EPERM;
+ mk = to_module_kobject(kobj);
+
if (!try_module_get(mk->mod))
return -ENODEV;
@@ -649,11 +650,12 @@ static ssize_t module_attr_store(struct
int ret;
attribute = to_module_attr(attr);
- mk = to_module_kobject(kobj);
if (!attribute->store)
return -EPERM;
+ mk = to_module_kobject(kobj);
+
if (!try_module_get(mk->mod))
return -ENODEV;
Jesper Juhl wrote:
> In kernel/params.c::module_attr_show and
> kernel/params.c::module_attr_store we call to_module_kobject() and save
> the result in a local variable right before a conditional statement that
> does not depend on the call to to_module_kobject() and may cause the
> function to return. If the function returns before we use the result of
> to_module_kobject() then the call is just a waste of time.
> The patch moves the call to to_module_kobject() down just before it is
> actually used, thus we save a call to the function in a few cases. I doubt
> this is in any way measurable, but I see no reason to not move the call -
> it should be an infinitesimal improvement with no ill sideeffects.
> Please consider applying.
>
>
> Signed-off-by: Jesper Juhl <[email protected]>
>
> diff -up linux-2.6.11-rc2-bk7-orig/kernel/params.c linux-2.6.11-rc2-bk7/kernel/params.c
> --- linux-2.6.11-rc2-bk7-orig/kernel/params.c 2005-01-29 23:54:53.000000000 +0100
> +++ linux-2.6.11-rc2-bk7/kernel/params.c 2005-01-30 00:27:08.000000000 +0100
> @@ -625,11 +625,12 @@ static ssize_t module_attr_show(struct k
> int ret;
>
> attribute = to_module_attr(attr);
> - mk = to_module_kobject(kobj);
>
> if (!attribute->show)
> return -EPERM;
>
> + mk = to_module_kobject(kobj);
> +
> if (!try_module_get(mk->mod))
> return -ENODEV;
>
> @@ -649,11 +650,12 @@ static ssize_t module_attr_store(struct
> int ret;
>
> attribute = to_module_attr(attr);
> - mk = to_module_kobject(kobj);
>
> if (!attribute->store)
> return -EPERM;
>
> + mk = to_module_kobject(kobj);
> +
> if (!try_module_get(mk->mod))
> return -ENODEV;
>
>
>
>
I'd bet that the compiler already reorders the assignment since there is
no side effect to to_module_kobject(). Even with a patch like this you
can't control exactly when the assignment is done. There may not even
be an assignment, since mk is a constant offset of kobj.
--
Brian Gerst
On Sat, 29 Jan 2005, Brian Gerst wrote:
> Jesper Juhl wrote:
> > In kernel/params.c::module_attr_show and kernel/params.c::module_attr_store
> > we call to_module_kobject() and save the result in a local variable right
> > before a conditional statement that does not depend on the call to
> > to_module_kobject() and may cause the function to return. If the function
> > returns before we use the result of to_module_kobject() then the call is
> > just a waste of time. The patch moves the call to to_module_kobject() down
> > just before it is actually used, thus we save a call to the function in a
> > few cases. I doubt this is in any way measurable, but I see no reason to not
> > move the call - it should be an infinitesimal improvement with no ill
> > sideeffects.
> > Please consider applying.
> >
> >
> > Signed-off-by: Jesper Juhl <[email protected]>
> >
> > diff -up linux-2.6.11-rc2-bk7-orig/kernel/params.c
> > linux-2.6.11-rc2-bk7/kernel/params.c
> > --- linux-2.6.11-rc2-bk7-orig/kernel/params.c 2005-01-29 23:54:53.000000000
> > +0100
> > +++ linux-2.6.11-rc2-bk7/kernel/params.c 2005-01-30 00:27:08.000000000
> > +0100
> > @@ -625,11 +625,12 @@ static ssize_t module_attr_show(struct k
> > int ret;
> > attribute = to_module_attr(attr);
> > - mk = to_module_kobject(kobj);
> > if (!attribute->show)
> > return -EPERM;
> > + mk = to_module_kobject(kobj);
> > +
> > if (!try_module_get(mk->mod))
> > return -ENODEV;
> > @@ -649,11 +650,12 @@ static ssize_t module_attr_store(struct int
> > ret;
> > attribute = to_module_attr(attr);
> > - mk = to_module_kobject(kobj);
> > if (!attribute->store)
> > return -EPERM;
> > + mk = to_module_kobject(kobj);
> > +
> > if (!try_module_get(mk->mod))
> > return -ENODEV;
> >
> >
>
> I'd bet that the compiler already reorders the assignment since there is no
> side effect to to_module_kobject(). Even with a patch like this you can't
> control exactly when the assignment is done. There may not even be an
> assignment, since mk is a constant offset of kobj.
>
True, the compiler is free to be clever, but I still think it's best to
write the code in the most optimal way as seen from a C perspective.
I just took a look at the compiled object files with and without the
patch, and it makes no difference what-so-ever - gcc generates the exact
same code. So you are right, gcc is clever about it.
Hmm, it's Rusty's code as far as I can see, I'll leave it up to him to
apply the patch or not.
--
Jesper
On Sun, 2005-01-30 at 13:43 +0100, Jesper Juhl wrote:
> True, the compiler is free to be clever, but I still think it's best to
> write the code in the most optimal way as seen from a C perspective.
> I just took a look at the compiled object files with and without the
> patch, and it makes no difference what-so-ever - gcc generates the exact
> same code. So you are right, gcc is clever about it.
If the code were not already in the kernel, I'd probably apply this.
However, cleanups this trivial are not worthwhile IMHO.
Rusty.
--
A bad analogy is like a leaky screwdriver -- Richard Braakman