Received-SPF: pass (google.com: domain of linux-kernel+bounces-38780-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 25 Jan 2024 14:53:40 +0000
From: Vincent Donnefort <vdonnefort@google.com>
To: Masami Hiramatsu <mhiramat@kernel.org>
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: <ZbJ19CF2Zv4d0R_Z@google.com>
References: <20240123110757.3657908-1-vdonnefort@google.com>
 <20240123110757.3657908-4-vdonnefort@google.com>
 <20240125001149.364c0b08237e8b7f0a69bd56@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240125001149.364c0b08237e8b7f0a69bd56@kernel.org>

Hi Masami,

Thanks for taking the time to look at those changes.

On Thu, Jan 25, 2024 at 12:11:49AM +0900, Masami Hiramatsu wrote:
> 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.

Duh, yes it must depends on TRACER_MAX_TRACE :-\

> 
> >  			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)

Will add something for the __unregister_trigger flavour.

> 
> > +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.

It should indeed be 

 ret = tracing_arm_snapshot()
 if (ret)
     return ret;

> 
> 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>