Received-SPF: pass (google.com: domain of linux-kernel+bounces-37222-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 25 Jan 2024 00:11:49 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Vincent Donnefort <vdonnefort@google.com>
Cc: rostedt@goodmis.org, linux-kernel@vger.kernel.org,
 linux-trace-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
 kernel-team@android.com
Subject: Re: [PATCH v12 3/6] tracing: Add snapshot refcount
Message-Id: <20240125001149.364c0b08237e8b7f0a69bd56@kernel.org>
In-Reply-To: <20240123110757.3657908-4-vdonnefort@google.com>
References: <20240123110757.3657908-1-vdonnefort@google.com>
	<20240123110757.3657908-4-vdonnefort@google.com>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 23 Jan 2024 11:07:54 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:

[...]
> @@ -6592,8 +6641,11 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>  
>  	if (t->init) {
>  		ret = tracer_init(t, tr);
> -		if (ret)
> +		if (ret) {
> +			if (t->use_max_tr)
> +				tracing_disarm_snapshot_locked(tr);

This part is out of CONFIG_TRACER_MAX_TRACE, so it may cause a compile error
if CONFIG_TRACER_MAX_TRACE is not set.

>  			goto out;
> +		}
>  	}
>  
>  	tr->current_trace = t;
[...]
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 46439e3bcec4..d41bf64741e2 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -597,20 +597,9 @@ static int register_trigger(char *glob,
>  	return ret;
>  }
>  
> -/**
> - * unregister_trigger - Generic event_command @unreg implementation
> - * @glob: The raw string used to register the trigger
> - * @test: Trigger-specific data used to find the trigger to remove
> - * @file: The trace_event_file associated with the event
> - *
> - * Common implementation for event trigger unregistration.
> - *
> - * Usually used directly as the @unreg method in event command
> - * implementations.
> - */
> -static void unregister_trigger(char *glob,
> -			       struct event_trigger_data *test,
> -			       struct trace_event_file *file)

OK, so __unregister_trigger returns true if data exists, but
unregister_trigger() ignores results. (I want some comment here)

> +static bool __unregister_trigger(char *glob,
> +				 struct event_trigger_data *test,
> +				 struct trace_event_file *file)
>  {
>  	struct event_trigger_data *data = NULL, *iter;
>  
> @@ -626,8 +615,32 @@ static void unregister_trigger(char *glob,
>  		}
>  	}
>  
> -	if (data && data->ops->free)
> -		data->ops->free(data);
> +	if (data) {
> +		if (data->ops->free)
> +			data->ops->free(data);
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * unregister_trigger - Generic event_command @unreg implementation
> + * @glob: The raw string used to register the trigger
> + * @test: Trigger-specific data used to find the trigger to remove
> + * @file: The trace_event_file associated with the event
> + *
> + * Common implementation for event trigger unregistration.
> + *
> + * Usually used directly as the @unreg method in event command
> + * implementations.
> + */
> +static void unregister_trigger(char *glob,
> +			       struct event_trigger_data *test,
> +			       struct trace_event_file *file)
> +{
> +	__unregister_trigger(glob, test, file);
>  }
>  
>  /*
> @@ -1470,12 +1483,20 @@ register_snapshot_trigger(char *glob,
>  			  struct event_trigger_data *data,
>  			  struct trace_event_file *file)
>  {
> -	if (tracing_alloc_snapshot_instance(file->tr) != 0)
> +	if (tracing_arm_snapshot(file->tr))
>  		return 0;

BTW, is this return value correct? It seems that the register_*_trigger()
will return error code when it fails.

Thanks,

>  
>  	return register_trigger(glob, data, file);
>  }
>  
> +static void unregister_snapshot_trigger(char *glob,
> +					struct event_trigger_data *data,
> +					struct trace_event_file *file)
> +{
> +	if (__unregister_trigger(glob, data, file))
> +		tracing_disarm_snapshot(file->tr);
> +}
> +
>  static int
>  snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data)
>  {
> @@ -1508,7 +1529,7 @@ static struct event_command trigger_snapshot_cmd = {
>  	.trigger_type		= ETT_SNAPSHOT,
>  	.parse			= event_trigger_parse,
>  	.reg			= register_snapshot_trigger,
> -	.unreg			= unregister_trigger,
> +	.unreg			= unregister_snapshot_trigger,
>  	.get_trigger_ops	= snapshot_get_trigger_ops,
>  	.set_filter		= set_trigger_filter,
>  };
> -- 
> 2.43.0.429.g432eaa2c6b-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>