Re: [PATCH tip 1/1] tracing: Only drop the BKL in register_tracer if there is a selftest

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Feb 12, 2009 at 05:23:54PM -0200, Arnaldo Carvalho de Melo wrote:
> Impact: cleanup
> 
> We only need to drop the BKL for some tracers, and then only if the
> tracer being registered has a selftest method. So clean up moving the
> selftest bits to a different function.
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  kernel/trace/trace.c |  138 ++++++++++++++++++++++++++++----------------------
>  1 files changed, 77 insertions(+), 61 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 95f99a7..fc01e0c 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -452,6 +452,76 @@ update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int cpu)
>  	__raw_spin_unlock(&ftrace_max_lock);
>  }
>  
> +static int trace_selftest(struct tracer *trace)
> +{
> +	int ret = 0;
> +#ifdef CONFIG_FTRACE_STARTUP_TEST
> +	struct tracer *saved_tracer;
> +	struct trace_array *tr;
> +	int i;
> +
> +	if (trace->selftest == NULL || tracing_selftest_disabled)
> +		return 0;
> +	/*
> +	 * When this gets called we hold the BKL which means that
> +	 * preemption is disabled. Various trace selftests however
> +	 * need to disable and enable preemption for successful tests.
> +	 * So we drop the BKL here and grab it after the tests again.
> +	 */
> +	unlock_kernel();
> +
> +	saved_tracer = current_trace;
> +	tr = &global_trace;
> +
> +	/*
> +	 * Run a selftest on this tracer.
> +	 * Here we reset the trace buffer, and set the current
> +	 * tracer to be this tracer. The tracer can then run some
> +	 * internal tracing to verify that everything is in order.
> +	 * If we fail, we do not register this tracer.
> +	 */
> +	for_each_tracing_cpu(i)
> +		tracing_reset(tr, i);
> +
> +	current_trace = trace;
> +	/* the test is responsible for initializing and enabling */
> +	pr_info("Testing tracer %s: ", trace->name);
> +	ret = trace->selftest(trace, tr);
> +	/* the test is responsible for resetting too */
> +	current_trace = saved_tracer;
> +	if (ret)
> +		printk(KERN_CONT "FAILED!\n");
> +	else {
> +		/* Only reset on passing, to avoid touching corrupted buffers */
> +		for_each_tracing_cpu(i)
> +			tracing_reset(tr, i);
> +
> +		printk(KERN_CONT "PASSED\n");
> +	}
> +
> +	tracing_selftest_running = false;
> +	lock_kernel();
> +#endif
> +	return ret;
> +}
> +
> +static void trace_boot(struct tracer *trace)
> +{
> +	if (strncmp(default_bootup_tracer, trace->name, BOOTUP_TRACER_SIZE))
> +		return;
> +
> +	pr_info("Starting tracer '%s'\n", trace->name);
> +	/* Do we want this tracer to start on bootup? */
> +	tracing_set_tracer(trace->name);
> +	default_bootup_tracer = NULL;
> +	/* disable other selftests, since this will break it. */
> +	tracing_selftest_disabled = 1;
> +#ifdef CONFIG_FTRACE_STARTUP_TEST
> +	pr_info("Disabling FTRACE selftests due to running tracer '%s'\n",
> +		trace->name);
> +#endif
> +}
> +
>  /**
>   * register_tracer - register a tracer with the ftrace system.
>   * @type - the plugin for the tracer
> @@ -471,13 +541,6 @@ __acquires(kernel_lock)
>  		return -1;
>  	}
>  
> -	/*
> -	 * When this gets called we hold the BKL which means that
> -	 * preemption is disabled. Various trace selftests however
> -	 * need to disable and enable preemption for successful tests.
> -	 * So we drop the BKL here and grab it after the tests again.
> -	 */
> -	unlock_kernel();
>  	mutex_lock(&trace_types_lock);
>  
>  	tracing_selftest_running = true;
> @@ -488,7 +551,7 @@ __acquires(kernel_lock)
>  			pr_info("Trace %s already registered\n",
>  				type->name);
>  			ret = -1;
> -			goto out;
> +			goto out_mutex_unlock;
>  		}
>  	}
>  
> @@ -500,39 +563,9 @@ __acquires(kernel_lock)
>  		if (!type->flags->opts)
>  			type->flags->opts = dummy_tracer_opt;
>  
> -#ifdef CONFIG_FTRACE_STARTUP_TEST
> -	if (type->selftest && !tracing_selftest_disabled) {
> -		struct tracer *saved_tracer = current_trace;
> -		struct trace_array *tr = &global_trace;
> -		int i;
> -
> -		/*
> -		 * Run a selftest on this tracer.
> -		 * Here we reset the trace buffer, and set the current
> -		 * tracer to be this tracer. The tracer can then run some
> -		 * internal tracing to verify that everything is in order.
> -		 * If we fail, we do not register this tracer.
> -		 */
> -		for_each_tracing_cpu(i)
> -			tracing_reset(tr, i);
> -
> -		current_trace = type;
> -		/* the test is responsible for initializing and enabling */
> -		pr_info("Testing tracer %s: ", type->name);
> -		ret = type->selftest(type, tr);
> -		/* the test is responsible for resetting too */
> -		current_trace = saved_tracer;
> -		if (ret) {
> -			printk(KERN_CONT "FAILED!\n");
> -			goto out;
> -		}
> -		/* Only reset on passing, to avoid touching corrupted buffers */
> -		for_each_tracing_cpu(i)
> -			tracing_reset(tr, i);
> -
> -		printk(KERN_CONT "PASSED\n");
> -	}
> -#endif
> +	ret = trace_selftest(type);
> +	if (ret != 0)
> +		goto out_mutex_unlock;
>  
>  	type->next = trace_types;
>  	trace_types = type;
> @@ -540,29 +573,12 @@ __acquires(kernel_lock)
>  	if (len > max_tracer_type_len)
>  		max_tracer_type_len = len;
>  
> - out:
> -	tracing_selftest_running = false;
> +out_mutex_unlock:
>  	mutex_unlock(&trace_types_lock);
>  
> -	if (ret || !default_bootup_tracer)
> -		goto out_unlock;
> -
> -	if (strncmp(default_bootup_tracer, type->name, BOOTUP_TRACER_SIZE))
> -		goto out_unlock;
> -
> -	printk(KERN_INFO "Starting tracer '%s'\n", type->name);
> -	/* Do we want this tracer to start on bootup? */
> -	tracing_set_tracer(type->name);
> -	default_bootup_tracer = NULL;
> -	/* disable other selftests, since this will break it. */
> -	tracing_selftest_disabled = 1;
> -#ifdef CONFIG_FTRACE_STARTUP_TEST
> -	printk(KERN_INFO "Disabling FTRACE selftests due to running tracer '%s'\n",
> -	       type->name);
> -#endif
> +	if (ret == 0 && default_bootup_tracer)
> +		trace_boot(type);
>  
> - out_unlock:
> -	lock_kernel();
>  	return ret;
>  }
>  
> -- 
> 1.6.0.6
> 

Yes, this is better like that.
Thanks.