Hi Tao
On 11/30/21 16:35, Tao Zhou wrote:
> Hi Daniel,
>
> On Mon, Nov 29, 2021 at 12:07:41PM +0100, Daniel Bristot de Oliveira wrote:
>> +/*
>> + * osnoise_restore_cpus - restore the original "osnoise/cpus"
>> + *
>> + * osnoise_set_cpus() saves the original data for the "osnoise/cpus"
>
> osnoise_set_cpus() --> osnoise_restore_cpus()
This comment is correct... osnoise_set_cpus saves the data (in the tool),
osnoise_restore_cpus() restores it.
>> + * 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;
>> +
>> + if (!context->curr_cpus)
>> + return;
>> +
>> + /* nothing to do? */
>> + if (!strcmp(context->orig_cpus, context->curr_cpus))
>> + goto out_done;
>> +
>> + retval = write(context->cpus_fd, context->orig_cpus, strlen(context->orig_cpus));
>
> 'strlen(context->orig_cpus) + 1' for write size;
>
>> + if (retval < strlen(context->orig_cpus))
>
> Same here. Check 'strlen(context->orig_cpus) + 1'
Fixed in v9.
>> + err_msg("could not restore original osnoise cpus\n");
>> +
>> +out_done:
>> + free(context->curr_cpus);
>> + context->curr_cpus = NULL;
>> +}
>> +
>> +/*
>> + * osnoise_get_runtime - return the original "osnoise/runtime_us" value
>> + *
>> + * It also saves the value to be restored.
>> + */
>> +unsigned long long osnoise_get_runtime(struct osnoise_context *context)
>> +{
>> + char buffer[BUFF_U64_STR_SIZE];
>> + long long runtime_us;
>> + char *runtime_path;
>> + int retval;
>> +
>> + if (context->runtime_us != OSNOISE_TIME_INIT_VAL)
>> + return context->runtime_us;
>> +
>> + if (context->orig_runtime_us != OSNOISE_TIME_INIT_VAL)
>> + return context->orig_runtime_us;
>> +
>> + runtime_path = tracefs_get_tracing_file("osnoise/runtime_us");
>> +
>> + context->runtime_fd = open(runtime_path, O_RDWR);
>> + if (context->runtime_fd < 0)
>> + goto out_err;
>> +
>> + retval = read(context->runtime_fd, &buffer, sizeof(buffer));
>> + if (retval <= 0)
>> + goto out_close;
>> +
>> + runtime_us = get_llong_from_str(buffer);
>> + if (runtime_us < 0)
>> + goto out_close;
>> +
>> + tracefs_put_tracing_file(runtime_path);
>> +
>> + context->orig_runtime_us = runtime_us;
>> + return runtime_us;
>> +
>> +out_close:
>> + close(context->runtime_fd);
>> + context->runtime_fd = CLOSED_FD;
>> +out_err:
>> + tracefs_put_tracing_file(runtime_path);
>> + return 0;
>> +}
>> +/*
>> + * osnoise_get_period - return the original "osnoise/period_us" value
>> + *
>> + * It also saves the value to be restored.
>> + */
>> +unsigned long long osnoise_get_period(struct osnoise_context *context)
>> +{
>> + char buffer[BUFF_U64_STR_SIZE];
>> + char *period_path;
>> + long long period_us;
>> + int retval;
>> +
>> + if (context->period_us != OSNOISE_TIME_INIT_VAL)
>> + return context->period_us;
>> +
>> + if (context->orig_period_us != OSNOISE_TIME_INIT_VAL)
>> + return context->orig_period_us;
>> +
>> + period_path = tracefs_get_tracing_file("osnoise/period_us");
>> +
>> + context->period_fd = open(period_path, O_RDWR);
>> + if (context->period_fd < 0)
>> + goto out_err;
>> +
>> + retval = read(context->period_fd, &buffer, sizeof(buffer));
>> + if (retval <= 0)
>> + goto out_close;
>> +
>> + period_us = get_llong_from_str(buffer);
>> + if (period_us < 0)
>> + goto out_close;
>> +
>> + tracefs_put_tracing_file(period_path);
>> +
>> + context->orig_period_us = period_us;
>> + return period_us;
>> +
>> +out_close:
>> + close(context->period_fd);
>> + context->period_fd = CLOSED_FD;
>> +out_err:
>> + tracefs_put_tracing_file(period_path);
>> + return 0;
>> +}
>
> osnoise_get_period() and osnoise_get_runtime() almost the same.
> Use macro to generate code. Some thing also not sure now. Shame
>
>
> #define osnoise_get_period osnoise_get(period)
> #define osnoise_get_runtime osnoise_get(runtime)
>
> #define osnoise_get(x) \
> unsigned long long osnoise_get_##x(struct osnoise_context *context) \
> { \
> char buffer[BUFF_U64_STR_SIZE]; \
> char * x##_path; \
> long long x##_us; \
> if (context->x##_us != OSNOISE_TIME_INIT_VAL) \
> return context->x##_us; \
> if (context->orig_##x##_us != OSNOISE_TIME_INIT_VAL) \
> return context->orig_##x##_us; \
> x##_path = tracefs_get_tracing_file("osnoise/x##_us"); \
> context->x##_fd = open(x##_path, O_RDWR); \
> if (context->x##_fd < 0) \
> goto out_err; \
> retval = read(context->x##_fd, &buffer, sizeof(buffer)); \
> if (retval <= 0) \
> goto out_close; \
> x##_us = get_llong_from_str(buffer); \
> if (x##_us < 0) \
> goto out_close; \
> tracefs_put_tracing_file(x##_path); \
> context->orig_##x##_us = x##_us; \
> return x##_us; \
> out_close: \
> close(context->x##_fd); \
> context->x##_fd = CLOSED_FD; \
> out_err: \
> tracefs_put_tracing_file(x##_path); \
> return 0; \
> }
I am not sure if it is worth to trade the readability for just two functions. I
will keep this as is foe now, and think about it in a second moment.
[...]
>> +
>> +/*
>> + * osnoise_set_runtime_period - set osnoise runtime and period
>> + *
>> + * Osnoise's runtime and period are related as runtime <= period.
>> + * Thus, this function saves the original values, and then tries
>> + * to set the runtime and period if they are != 0.
>> + */
>> +int osnoise_set_runtime_period(struct osnoise_context *context,
>> + unsigned long long runtime,
>> + unsigned long long period)
>> +{
>> + unsigned long long curr_runtime_us;
>> + unsigned long long curr_period_us;
>> + int retval;
>> +
>> + if (!period && !runtime)
>> + return 0;
>> +
>> + curr_runtime_us = osnoise_get_runtime(context);
>> + curr_period_us = osnoise_get_period(context);
>> +
>> + /* error getting any value? */
>> + if (curr_period_us == -1 || curr_runtime_us == -1)
>> + return -1;
>
> 'curr_period_us' and 'curr_runtime_us' error value should be
> 0(OSNOISE_TIME_INIT_VAL).
>
Right, I am now (in v9) returning the *_INIT_VAL on all errors, and using the
macro to check for errors.
[...]
>> +static long long
>> +osnoise_get_timerlat_period_us(struct osnoise_context *context)
>> +{
>> + char buffer[BUFF_U64_STR_SIZE];
>> + long long timerlat_period_us;
>> + char *stop_path;
>> + int retval;
>> +
>> + if (context->timerlat_period_us != OSNOISE_TIME_INIT_VAL)
>> + return context->timerlat_period_us;
>> +
>> + if (context->orig_timerlat_period_us != OSNOISE_TIME_INIT_VAL)
>> + return context->orig_timerlat_period_us;
>> +
>> + stop_path = tracefs_get_tracing_file("osnoise/timerlat_period_us");
>
> Using timerlat_period_path seems to be straightforward.
>
I am using config_path for all variables like this.
[...]
>> +/*
>> + * osnoise_context_alloc - alloc an osnoise_context
>> + *
>> + * The osnoise context contains the information of the "osnoise/" configs.
>> + * It is used to set and restore the config.
>> + */
>> +struct osnoise_context *osnoise_context_alloc(void)
>> +{
>> + struct osnoise_context *context;
>> +
>> + context = calloc(1, sizeof(*context));
>> + if (!context)
>> + goto out_err;
>> +
>> + context->cpus_fd = CLOSED_FD;
>> + context->runtime_fd = CLOSED_FD;
>> + context->period_fd = CLOSED_FD;
>> + context->stop_us_fd = CLOSED_FD;
>> + context->stop_total_us_fd = CLOSED_FD;
>> + context->timerlat_period_us_fd = CLOSED_FD;
>> + context->print_stack_fd = CLOSED_FD;
>> +
>> + context->orig_stop_us = OSNOISE_OPTION_INIT_VAL;
>> + context->stop_us = OSNOISE_OPTION_INIT_VAL;
>> +
>> + context->orig_stop_total_us = OSNOISE_OPTION_INIT_VAL;
>> + context->stop_total_us = OSNOISE_OPTION_INIT_VAL;
>> +
>> + context->orig_print_stack = OSNOISE_OPTION_INIT_VAL;
>> + context->print_stack = OSNOISE_OPTION_INIT_VAL;
>> +
>> + osnoise_get_context(context);
>> +
>> + return context;
>> +out_err:
>> + if (context)
>> + free(context);
>
> context is NULL here, so no need the check. Just directly return NULL
> when 'if(!context)' is enough.
>
>> + return NULL;
>> +}
In v9, I am removing the goto, returning NULL if (!context).
> Sorry for my slow and not complete reply.. and leave not sure here.
>
> Thanks,
> Tao
Thanks Tao
-- Daniel
