Prev: Documentation/isdn: CAPI controller interface amendment
Next: [PATCH 17/49] memblock: Move memblock_init() to the bottom of the file
From: Steven Rostedt on 6 Jul 2010 18:20
On Tue, 2010-07-06 at 09:51 +0900, KOSAKI Motohiro wrote:
> Andrew Morton pointed out almost sched_setscheduler() caller are
> using fixed parameter and it can be converted static. it reduce
> runtume memory waste a bit.

We are replacing runtime waste with permanent waste?

>
> Reported-by: Andrew Morton <akpm(a)linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com>



> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -560,7 +560,7 @@ trace_selftest_startup_nop(struct tracer *trace, struct trace_array *tr)
> static int trace_wakeup_test_thread(void *data)
> {
> /* Make this a RT thread, doesn't need to be too high */
> - struct sched_param param = { .sched_priority = 5 };
> + static struct sched_param param = { .sched_priority = 5 };
> struct completion *x = data;
>

This is a thread that runs on boot up to test the sched_wakeup tracer.
Then it is deleted and all memory is reclaimed.

Thus, this patch just took memory that was usable at run time and
removed it permanently.

Please Cc me on all tracing changes.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 6 Jul 2010 19:20
On Tue, 06 Jul 2010 18:13:58 -0400 Steven Rostedt <rostedt(a)goodmis.org> wrote:

> On Tue, 2010-07-06 at 09:51 +0900, KOSAKI Motohiro wrote:
> > Andrew Morton pointed out almost sched_setscheduler() caller are
> > using fixed parameter and it can be converted static. it reduce
> > runtume memory waste a bit.
>
> We are replacing runtime waste with permanent waste?

Confused. kernel/trace/ appears to waste resources by design, so what's
the issue?

I don't think this change will cause more waste. It'll consume 4 bytes
of .data and will save a little more .text.

> >
> > Reported-by: Andrew Morton <akpm(a)linux-foundation.org>
> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com>
>
>
>
> > --- a/kernel/trace/trace_selftest.c
> > +++ b/kernel/trace/trace_selftest.c
> > @@ -560,7 +560,7 @@ trace_selftest_startup_nop(struct tracer *trace, struct trace_array *tr)
> > static int trace_wakeup_test_thread(void *data)
> > {
> > /* Make this a RT thread, doesn't need to be too high */
> > - struct sched_param param = { .sched_priority = 5 };
> > + static struct sched_param param = { .sched_priority = 5 };
> > struct completion *x = data;
> >
>
> This is a thread that runs on boot up to test the sched_wakeup tracer.
> Then it is deleted and all memory is reclaimed.
>
> Thus, this patch just took memory that was usable at run time and
> removed it permanently.
>
> Please Cc me on all tracing changes.

Well if we're so worried about resource wastage then how about making
all boot-time-only text and data reside in __init and __initdata
sections rather than hanging around uselessly in memory for ever?

Only that's going to be hard because we went and added pointers into
..init.text from .data due to `struct tracer.selftest', which will cause
a storm of section mismatch warnings. Doh, should have invoked the
selftests from initcalls. That might open the opportunity of running
the selftests by modprobing the selftest module, too.

And I _do_ wish the selftest module was modprobeable, rather than this
monstrosity:

#ifdef CONFIG_FTRACE_SELFTEST
/* Let selftest have access to static functions in this file */
#include "trace_selftest.c"
#endif

Really? Who had a tastebudectomy over there? At least call it
trace_selftest.inc or something, so poor schmucks don't go scrabbling
around wondering "how the hell does this thing get built oh no they
didn't really go and #include it did they?"


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 6 Jul 2010 20:00
On Tue, 2010-07-06 at 16:12 -0700, Andrew Morton wrote:

> Well if we're so worried about resource wastage then how about making
> all boot-time-only text and data reside in __init and __initdata
> sections rather than hanging around uselessly in memory for ever?

That would be a patch I would like :-)

I could probably do that when I get some time.

>
> Only that's going to be hard because we went and added pointers into
> .init.text from .data due to `struct tracer.selftest', which will cause
> a storm of section mismatch warnings. Doh, should have invoked the
> selftests from initcalls. That might open the opportunity of running
> the selftests by modprobing the selftest module, too.

They are called by initcalls. The initcalls register the tracers and
that is the time we call the selftest. No other time.

Is there a way that we set up a function pointer to let the section
checks know that it is only called at bootup?

>
> And I _do_ wish the selftest module was modprobeable, rather than this
> monstrosity:

The selftests are done by individual tracers at boot up. It would be
hard to modprobe them at that time.


> #ifdef CONFIG_FTRACE_SELFTEST
> /* Let selftest have access to static functions in this file */
> #include "trace_selftest.c"
> #endif
>
> Really? Who had a tastebudectomy over there? At least call it
> trace_selftest.inc or something, so poor schmucks don't go scrabbling
> around wondering "how the hell does this thing get built oh no they
> didn't really go and #include it did they?"


Well this is also the way sched.c adds all its extra code. Making it
trace_selftest.inc would make it hard to know what the hell it was. And
also hard for editors to know what type of file it is, or things can be
missed with a 'find . -name "*.[ch]" | xargs grep blahblah'

Yes, the self tests are ugly and can probably go with an overhaul. Since
we are trying to get away from the tracer plugins anyway, they will
start disappearing when the plugins do.

We should have some main selftests anyway. Those are for the TRACE_EVENT
tests (which are not even in the trace_selftest.c file, and the function
testing which currently are, as well as the latency testers.

The trace_selftest.c should eventually be replaced with more compact
tests for the specific types of tracing.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 6 Jul 2010 20:10
On Tue, 06 Jul 2010 19:49:47 -0400 Steven Rostedt <rostedt(a)goodmis.org> wrote:

> On Tue, 2010-07-06 at 16:12 -0700, Andrew Morton wrote:
>
> > Well if we're so worried about resource wastage then how about making
> > all boot-time-only text and data reside in __init and __initdata
> > sections rather than hanging around uselessly in memory for ever?
>
> That would be a patch I would like :-)
>
> I could probably do that when I get some time.
>
> >
> > Only that's going to be hard because we went and added pointers into
> > .init.text from .data due to `struct tracer.selftest', which will cause
> > a storm of section mismatch warnings. Doh, should have invoked the
> > selftests from initcalls. That might open the opportunity of running
> > the selftests by modprobing the selftest module, too.
>
> They are called by initcalls. The initcalls register the tracers and
> that is the time we call the selftest. No other time.

It should all be __init!

> Is there a way that we set up a function pointer to let the section
> checks know that it is only called at bootup?

<sticks his nose in modpost.c for the first time>

There are various whitelisting hacks in there based on the name of the
offending symbol. Search term: DEFAULT_SYMBOL_WHITE_LIST.

It'd be cleaner to just zap the tracer.selftest field altogether and
run the tests from initcalls if possible?

> >
> > And I _do_ wish the selftest module was modprobeable, rather than this
> > monstrosity:
>
> The selftests are done by individual tracers at boot up. It would be
> hard to modprobe them at that time.

No, if tracer_selftest.o was linked into vmlinux then the tests get run
within do_initcalls(). If tracer_selftest.o is a module, then the tests
get run at modprobe-time. The latter option may not be terribly useful
but it comes basically for free as a reward for doing stuff correctly.

>
> > #ifdef CONFIG_FTRACE_SELFTEST
> > /* Let selftest have access to static functions in this file */
> > #include "trace_selftest.c"
> > #endif
> >
> > Really? Who had a tastebudectomy over there? At least call it
> > trace_selftest.inc or something, so poor schmucks don't go scrabbling
> > around wondering "how the hell does this thing get built oh no they
> > didn't really go and #include it did they?"
>
>
> Well this is also the way sched.c adds all its extra code.

The sched.c hack sucks too.

> Making it
> trace_selftest.inc would make it hard to know what the hell it was.

trace_selftest.i_really_suck?

> And
> also hard for editors to know what type of file it is, or things can be
> missed with a 'find . -name "*.[ch]" | xargs grep blahblah'

Well bad luck. Of _course_ it makes a mess. It's already a mess.

How's about just removing the `static' from whichever symbols
tracer_selftest needs? That'd surely be better than #including a .c
file.

> Yes, the self tests are ugly and can probably go with an overhaul. Since
> we are trying to get away from the tracer plugins anyway, they will
> start disappearing when the plugins do.
>
> We should have some main selftests anyway. Those are for the TRACE_EVENT
> tests (which are not even in the trace_selftest.c file, and the function
> testing which currently are, as well as the latency testers.
>
> The trace_selftest.c should eventually be replaced with more compact
> tests for the specific types of tracing.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 7 Jul 2010 15:50
On Tue, 2010-07-06 at 17:02 -0700, Andrew Morton wrote:
> > Well this is also the way sched.c adds all its extra code.
>
> The sched.c hack sucks too.

Agreed, moving things to kernel/sched/ and adding some internal.h thing
could cure that, but I simply haven't gotten around to cleaning that
up..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: Documentation/isdn: CAPI controller interface amendment
Next: [PATCH 17/49] memblock: Move memblock_init() to the bottom of the file