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: <8ba373fa-2649-db63-546d-d300b9205947@kernel.org>
Date:   Fri, 29 Oct 2021 18:03:30 +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 V6 09/20] rtla: Add osnoise tool
Content-Language: en-US
To:     Tao Zhou <tao.zhou@linux.dev>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        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.1635284863.git.bristot@kernel.org>
 <a389dec45805f3221fe1a8e86c725ae24d16ec82.1635284863.git.bristot@kernel.org>
 <YXl+krtOjd5CzDRc@geo.homenetwork>
From:   Daniel Bristot de Oliveira <bristot@kernel.org>
In-Reply-To: <YXl+krtOjd5CzDRc@geo.homenetwork>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/27/21 18:30, Tao Zhou wrote:
> On Wed, Oct 27, 2021 at 12:06:20AM +0200, Daniel Bristot de Oliveira wrote:
> 
>> +/*
>> + * osnoise_set_cpus - configure osnoise to run on *cpus
>> + *
>> + * "osnoise/cpus" file is used to set the cpus in which osnoise/timerlat
>> + * will run. This function opens this file, saves the current value,
>> + * and set the cpus passed as argument.
>> + */
>> +int osnoise_set_cpus(struct osnoise_context *context, char *cpus)
>> +{
>> +	char *osnoise_cpus = tracefs_get_tracing_file("osnoise/cpus");
>> +	char curr_cpus[1024];
>> +	int retval;
>> +
>> +	context->cpus_fd = open(osnoise_cpus, O_RDWR);
>> +	if (!context->cpus_fd)
>> +		goto out_err;
> The above check should be "context->cpus_fd < 0".

Revisited all open/read/write!

>> +	retval = read(context->cpus_fd, &curr_cpus, sizeof(curr_cpus));
>> +	if (!retval)
>> +		goto out_close;
>> +	context->orig_cpus = strdup(curr_cpus);
>> +	if (!context->orig_cpus)
>> +		goto out_err;
> Need to close ->cpus_fd;
> 
>   if (!context->orig_cpus)
>     goto out_close;
> 
>> +	retval = write(context->cpus_fd, cpus, strlen(cpus) + 1);
>> +	if (!retval)
>> +		goto out_err;
> Same as above. Use "goto out_close" instead.

yep! fixed in the next version.

>> +	tracefs_put_tracing_file(osnoise_cpus);
>> +
>> +	return 0;
>> +
>> +out_close:
>> +	close(context->cpus_fd);
>> +	context->cpus_fd = -1;
>> +out_err:
>> +	tracefs_put_tracing_file(osnoise_cpus);
>> +	return 1;
>> +}
>> +
>> +/*
>> + * osnoise_restore_cpus - restore the original "osnoise/cpus"
>> + *
>> + * osnoise_set_cpus() saves the original data for the "osnoise/cpus"
>> + * file. This function restore the original config it was previously
>> + * modified.
>> + */
>> +void osnoise_restore_cpus(struct osnoise_context *context)
>> +{
>> +	int retval;
>> +
>> +	if (!context->orig_cpus)
>> +		return;
>> +
>> +	retval = write(context->cpus_fd, context->orig_cpus, strlen(context->orig_cpus));
> __osnoise_write_runtime() check "context->cpus_fd == -1".
> Is it possible here we need to check "context->cpus_fd == -1".
> 

So, yeah, this was inconsistent. In some parts I checked the fd, on other I
checked if the original value was load, so the file was opened. In the next
version I am checking if the original value was loaded, and then using it to
define if the fd is open.

Thanks for your review Tao!
-- Daniel