Messages in this thread | | | Date | Thu, 7 May 2009 10:34:23 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 4/7] ring-buffer: change test to be more latency friendly |
| |
* Ingo Molnar <mingo@elte.hu> wrote:
> > * 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.
Note, i pulled, because the other bits are really nice - but it would be good to eliminate the code above in a followup patch.
Ingo
| |