Re: [PATCH 4/7] ring-buffer: change test to be more latency friendly

[Ingo Molnar <mingo@elte.hu>]

* 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