Re: [PATCH 08/14] sched: add latency tracing for -deadline tasks.

[Juri Lelli <juri.lelli@gmail.com>]

On 11/27/2013 03:16 PM, Steven Rostedt wrote:
> On Wed, 27 Nov 2013 14:43:45 +0100
> Juri Lelli <juri.lelli@gmail.com> wrote:
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
>> index f76f8d6..ad94604 100644
>> --- a/kernel/trace/trace_selftest.c
>> +++ b/kernel/trace/trace_selftest.c
>> @@ -1023,16 +1023,16 @@ trace_selftest_startup_nop(struct tracer *trace, struct trace_array *tr)
>>  static int trace_wakeup_test_thread(void *data)
>>  {
>>  	/* Make this a -deadline thread */
>> -	struct sched_param2 paramx = {
>> +	static const struct sched_param2 param = {
>>  		.sched_priority = 0,
>> +		.sched_flags = 0,
>>  		.sched_runtime = 100000ULL,
>>  		.sched_deadline = 10000000ULL,
>>  		.sched_period = 10000000ULL
>> -		.sched_flags = 0
> 
> Assigning structures like this, you don't need to set the zero fields.
> all fields not explicitly stated, are set to zero.
>

Right.

>>  	};
>>  	struct completion *x = data;
>>  
>>
>> --------------------------------
>>  kernel/trace/trace_sched_wakeup.c |   28 ++++++++++++++++++++++++----
>>  1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
>> index 1457fb1..090c4d9 100644
>> --- a/kernel/trace/trace_sched_wakeup.c
>> +++ b/kernel/trace/trace_sched_wakeup.c
>> @@ -28,6 +28,7 @@ static int			wakeup_current_cpu;
>>  static unsigned			wakeup_prio = -1;
>>  static int			wakeup_rt;
>>  static int			wakeup_dl;
>> +static int			tracing_dl = 0;
> 
> Get rid of the ' = 0', its implicit to all static and global variables
> that are not given any value.
> 

And right.

>>  
>>  static arch_spinlock_t wakeup_lock =
>>  	(arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
>> @@ -438,6 +439,7 @@ static void __wakeup_reset(struct trace_array *tr)
>>  {
>>  	wakeup_cpu = -1;
>>  	wakeup_prio = -1;
>> +	tracing_dl = 0;
>>  
>>  	if (wakeup_task)
>>  		put_task_struct(wakeup_task);
>> @@ -481,9 +483,9 @@ probe_wakeup(void *ignore, struct task_struct *p, int success)
>>  	 *    sched_rt class;
>>  	 *  - wakeup_dl handles tasks belonging to sched_dl class only.
>>  	 */
>> -	if ((wakeup_dl && !dl_task(p)) ||
>> +	if (tracing_dl || (wakeup_dl && !dl_task(p)) ||
>>  	    (wakeup_rt && !dl_task(p) && !rt_task(p)) ||
>> -	    (p->prio >= wakeup_prio || p->prio >= current->prio))
>> +	    (!dl_task(p) && (p->prio >= wakeup_prio || p->prio >= current->prio)))
>>  		return;
>>  
>>  	pc = preempt_count();
>> @@ -495,7 +497,8 @@ probe_wakeup(void *ignore, struct task_struct *p, int success)
>>  	arch_spin_lock(&wakeup_lock);
>>  
>>  	/* check for races. */
>> -	if (!tracer_enabled || (!dl_task(p) && p->prio >= wakeup_prio))
>> +	if (!tracer_enabled || tracing_dl ||
>> +	    (!dl_task(p) && p->prio >= wakeup_prio))
>>  		goto out_locked;
>>  
>>  	/* reset the trace */
>> @@ -505,6 +508,15 @@ probe_wakeup(void *ignore, struct task_struct *p, int success)
>>  	wakeup_current_cpu = wakeup_cpu;
>>  	wakeup_prio = p->prio;
>>  
>> +	/*
>> +	 * Once you start tracing a -deadline task, don't bother tracing
>> +	 * another task until the first one wakes up.
>> +	 */
>> +	if (dl_task(p))
>> +		tracing_dl = 1;
>> +	else
>> +		tracing_dl = 0;
> 
> Do we need the else statement? I would think the only way to get here
> is if tracing_dl is already set to zero.
> 

No, indeed.

Thanks,

- Juri

>> +
>>  	wakeup_task = p;
>>  	get_task_struct(wakeup_task);
>>  
>> @@ -700,10 +712,18 @@ static struct tracer wakeup_dl_tracer __read_mostly =
>>  	.start		= wakeup_tracer_start,
>>  	.stop		= wakeup_tracer_stop,
>>  	.wait_pipe	= poll_wait_pipe,
>> -	.print_max	= 1,
>> +	.print_max	= true,
>> +	.print_header	= wakeup_print_header,
>> +	.print_line	= wakeup_print_line,
>> +	.flags		= &tracer_flags,
>> +	.set_flag	= wakeup_set_flag,
>> +	.flag_changed	= wakeup_flag_changed,
>>  #ifdef CONFIG_FTRACE_SELFTEST
>>  	.selftest    = trace_selftest_startup_wakeup,
>>  #endif
>> +	.open		= wakeup_trace_open,
>> +	.close		= wakeup_trace_close,
>> +	.use_max_tr	= true,
>>  };
>>  
>>  __init static int init_wakeup_tracer(void)
>> -----------------------------------
>>
>> Makes sense? :)
>>
>> Thanks,
>>
>> - Juri
>