Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <f30262dc-f1cf-4945-5a7d-5ecf5a0b5cc2@kernel.org>
Date:   Tue, 26 Oct 2021 10:38:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH V5 06/20] trace/osnoise: Allow multiple instances of the
 same tracer
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Tom Zanussi <zanussi@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Clark Williams <williams@redhat.com>,
        John Kacur <jkacur@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        linux-rt-users@vger.kernel.org, linux-trace-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <cover.1635181938.git.bristot@kernel.org>
 <69cbbd98cce2515c84127c8827d733dc87b04823.1635181938.git.bristot@kernel.org>
 <20211025220856.7fef7581@rorschach.local.home>
From:   Daniel Bristot de Oliveira <bristot@kernel.org>
In-Reply-To: <20211025220856.7fef7581@rorschach.local.home>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/26/21 04:08, Steven Rostedt wrote:
> On Mon, 25 Oct 2021 19:40:31 +0200
> Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
> 
>>  kernel/trace/trace_osnoise.c | 101 +++++++++++++++++++++++++++--------
>>  1 file changed, 78 insertions(+), 23 deletions(-)
>>
>> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
>> index 3db506f49a90..8681ffc3817b 100644
>> --- a/kernel/trace/trace_osnoise.c
>> +++ b/kernel/trace/trace_osnoise.c
>> @@ -64,6 +64,24 @@ static bool osnoise_has_registered_instances(void)
>>  					list);
>>  }
>>  
>> +/*
>> + * osnoise_instance_registered - check if a tr is already registered
>> + */
>> +static int osnoise_instance_registered(struct trace_array *tr)
>> +{
>> +	struct osnoise_instance *inst;
>> +	int found = 0;
>> +
>> +	rcu_read_lock();
>> +	list_for_each_entry_rcu(inst, &osnoise_instances, list) {
>> +		if (inst->tr == tr)
>> +			found = 1;
>> +	}
>> +	rcu_read_unlock();
>> +
>> +	return found;
>> +}
>> +
>>  /*
>>   * osnoise_register_instance - register a new trace instance
>>   *
>> @@ -2048,6 +2066,16 @@ static int osnoise_workload_start(void)
>>  {
>>  	int retval;
>>  
>> +	/*
>> +	 * Instances need to be registered after calling workload
>> +	 * start. Hence, if there is already an instance, the
>> +	 * workload was already registered. Otherwise, this
>> +	 * code is on the way to register the first instance,
>> +	 * and the workload will start.
>> +	 */
>> +	if (osnoise_has_registered_instances())
>> +		return 0;
> 
> Looking at how this is checked before being called, it really should
> return -1, as it is an error if this is called with instances active.

Hum.... maybe my explanation is not good enough. It is not a problem if it is
called with active instances. It would be an error if the same instance was
already registered at this point, but that was checked before. Here it is
checking for other instances that should have enabled the workload.

Does updating the comment with the one below helps?

---- %< ----
Instances need to be registered after calling workload
start. Hence, if there is already a registered instance,
this instance is not the first one to enable the workload,
and a previous instance has already started the workload.

Otherwise, this code is on the way to register the
first instance, and the workload needs to start.
---- >% ----

?


> -- Steve
> 
>> +
>>  	osn_var_reset_all();
>>  
>>  	retval = osnoise_hook_events();
>> @@ -2075,6 +2103,13 @@ static int osnoise_workload_start(void)
>>   */
>>  static void osnoise_workload_stop(void)
>>  {
>> +	/*
>> +	 * Instances need to be unregistered before calling
>> +	 * stop. Hence, if there is a registered instance, more
>> +	 * than one instance is running, and the workload will not
>> +	 * yet stop. Otherwise, this code is on the way to disable
>> +	 * the last instance, and the workload can stop.
>> +	 */
>>  	if (osnoise_has_registered_instances())
>>  		return;
>>  
>> @@ -2096,7 +2131,11 @@ static void osnoise_tracer_start(struct trace_array *tr)
>>  {
>>  	int retval;
>>  
>> -	if (osnoise_has_registered_instances())
>> +	/*
>> +	 * If the instance is already registered, there is no need to
>> +	 * register it again.
>> +	 */
>> +	if (osnoise_instance_registered(tr))
>>  		return;
>>  
>>  	retval = osnoise_workload_start();