Messages in this thread | | | Date | Thu, 7 May 2009 10:31:47 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 4/7] ring-buffer: change test to be more latency friendly |
| |
* Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt <srostedt@redhat.com> > > The ring buffer benchmark/test runs a producer for 10 seconds. > This is done with preemption and interrupts enabled. But if the > kernel is not compiled with CONFIG_PREEMPT, it basically stops > everything but interrupts for 10 seconds. > > Although this is just a test and is not for production, this attribute > can be quite annoying. It can also spawn badness elsewhere.
Yep, this probably explains that lockdep splat i got in a networking driver. Some functionality (a workqueue iirc) of the driver got starved and a time-out timer triggered - where lockdep caught locking badness.
> This patch solves the issues by calling "cond_resched" when the > system is not compiled with CONFIG_PREEMPT. It also keeps track of > the time spent to call cond_resched such that it does not go > against the time calculations. That is, if the task schedules > away, the time scheduled out is removed from the test data. Note, > this only works for non PREEMPT because we do not know when the > task is scheduled out if we have PREEMPT enabled. > > [ Impact: prevent test from stopping the world for 10 seconds ] > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org> > --- > kernel/trace/ring_buffer_benchmark.c | 31 +++++++++++++++++++++++++++++++ > 1 files changed, 31 insertions(+), 0 deletions(-) > > diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c > index dcd75e9..a26fc67 100644 > --- a/kernel/trace/ring_buffer_benchmark.c > +++ b/kernel/trace/ring_buffer_benchmark.c > @@ -185,6 +185,35 @@ static void ring_buffer_consumer(void) > complete(&read_done); > } > > +/* > + * If we are a non preempt kernel, the 10 second run will > + * stop everything while it runs. Instead, we will call cond_resched > + * and also add any time that was lost by a rescedule. > + */ > +#ifdef CONFIG_PREEMPT > +static void sched_if_needed(struct timeval *start_tv, struct timeval *end_tv) > +{ > +} > +#else > +static void sched_if_needed(struct timeval *start_tv, struct timeval *end_tv) > +{ > + struct timeval tv; > + > + cond_resched(); > + do_gettimeofday(&tv); > + if (tv.tv_usec < end_tv->tv_usec) { > + tv.tv_usec += 1000000; > + tv.tv_sec--; > + } > + start_tv->tv_sec += tv.tv_sec - end_tv->tv_sec; > + start_tv->tv_usec += tv.tv_usec - end_tv->tv_usec; > + if (start_tv->tv_usec > 1000000) { > + start_tv->tv_usec -= 1000000; > + start_tv->tv_sec++; > + } > +} > +#endif
This is _way_ too ugly. Why not just add a cond_resched() to the inner loop and be done with it? cond_resched() is conditional already, so it will only schedule 'if needed'.
If the test's timing gets skewed, what's the big deal? If its being preempted there will be impact _anyway_. (due to cache footprint elimination, etc.) People obviously should only rely on the numbers if the system is idle.
Ingo
| |