lkml.org 
[lkml]   [2009]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2009-05-07 10:35    [W:0.103 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site