Re: [RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 11 Feb 2025 00:44:17 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> @@ -9857,8 +9861,10 @@ static int __remove_instance(struct trace_array *tr)
>  	int i;
>  
>  	/* Reference counter for a newly created trace array = 1. */
> -	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref))
> +	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref)) {
> +		pr_info("Instance is busy: ref %d trace_ref %d\n", tr->ref, tr->trace_ref);

I don't think we need the print. The function will error with -EBUSY
letting the user know that it is busy. This is a very common error, where I
found that I had a program that didn't completely die, and still had a file
descriptor open on an instance, and when I go to remove it, I get:

  # cd /sys/kernel/tracing/
  # mkdir instances/foo
  # bash 5< instances/foo/events/enable
  # rmdir instances/foo
  rmdir: failed to remove 'instances/foo': Device or resource busy
  # exit
  # rmdir instances/foo
  #

If you only have it for debugging that is fine, but we should remove it
before we accept it.

-- Steve

>  		return -EBUSY;
> +	}
>  
>  	list_del(&tr->list);
>  
> @@ -9881,6 +9887,11 @@ static int __remove_instance(struct trace_array *tr)
>  	free_trace_buffers(tr);
>  	clear_tracing_err_log(tr);
>  
> +	if (tr->range_name) {
> +		reserved_mem_release_by_name(tr->range_name);
> +		kfree(tr->range_name);
> +	}
> +
>  	for (i = 0; i < tr->nr_topts; i++) {
>  		kfree(tr->topts[i].topts);
>  	}
> @@ -10740,6 +10751,7 @@ __init static void enable_instances(void)
>  		bool traceoff = false;
>  		char *flag_delim;
>  		char *addr_delim;
> +		char *rname __free(kfree) = NULL;
>  
>  		tok = strsep(&curr_str, ",");
>  
> @@ -10796,6 +10808,7 @@ __init static void enable_instances(void)
>  				pr_warn("Failed to map boot instance %s to %s\n", name, tok);
>  				continue;
>  			}
> +			rname = kstrdup(tok, GFP_KERNEL);
>  		}
>  
>  		if (start) {
> @@ -10832,7 +10845,7 @@ __init static void enable_instances(void)
>  		 */
>  		if (start) {
>  			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
> -			tr->ref++;
> +			tr->range_name = no_free_ptr(rname);
>  		}
>  
>  		while ((tok = strsep(&curr_str, ","))) {
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 47c0742fe9ec..ae8f7fac6592 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -348,6 +348,7 @@ struct trace_array {
>  	unsigned int		mapped;
>  	unsigned long		range_addr_start;
>  	unsigned long		range_addr_size;
> +	char			*range_name;
>  	long			text_delta;
>  	int			nr_modules;
>  	long			*module_delta;