Re: [PATCH V5 06/20] trace/osnoise: Allow multiple instances of the same tracer

On Tue, 26 Oct 2021 10:38:27 +0200
Daniel Bristot de Oliveira [off-list ref] wrote:

quoted

quoted

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

No need.

quoted

quoted

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

My eyes missed that you removed the osnoise_has_registered_instances() for
the osnoise_instance_registered(tr), and thought you were doing the same
test twice.

My mistake. I need to not review patches at the end of the day when I'm
ready to go to sleep.

-- Steve