Mailing List Archive

[PATCH V2 1/6] rtla/osnoise: Add helper functions to manipulate osnoise/options
Add some helper functions to read and set the on/off osnoise/options.
No usage in this patch.

In preparation for hwnoise tool.

Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
Cc: Daniel Bristot de Oliveira <bristot@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Jonathan Corbet <corbet@lwn.net>
---
tools/tracing/rtla/src/osnoise.c | 108 +++++++++++++++++++++++++++++++
1 file changed, 108 insertions(+)

diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
index 4dee343909b1..050a9997191c 100644
--- a/tools/tracing/rtla/src/osnoise.c
+++ b/tools/tracing/rtla/src/osnoise.c
@@ -734,6 +734,114 @@ void osnoise_put_tracing_thresh(struct osnoise_context *context)
context->orig_tracing_thresh = OSNOISE_OPTION_INIT_VAL;
}

+static int osnoise_options_get_option(char *option)
+{
+ char *options = tracefs_instance_file_read(NULL, "osnoise/options", NULL);
+ char no_option[128];
+ int retval = 0;
+ char *opt;
+
+ if (!options)
+ return OSNOISE_OPTION_INIT_VAL;
+
+ /*
+ * Check first if the option is disabled.
+ */
+ snprintf(no_option, sizeof(no_option), "NO_%s", option);
+
+ opt = strstr(options, no_option);
+ if (opt)
+ goto out_free;
+
+ /*
+ * Now that it is not disabled, if the string is there, it is
+ * enabled. If the string is not there, the option does not exist.
+ */
+ opt = strstr(options, option);
+ if (opt)
+ retval = 1;
+ else
+ retval = OSNOISE_OPTION_INIT_VAL;
+
+out_free:
+ free(options);
+ return retval;
+}
+
+static int osnoise_options_set_option(char *option, bool onoff)
+{
+ char no_option[128];
+
+ if (onoff)
+ return tracefs_instance_file_write(NULL, "osnoise/options", option);
+
+ snprintf(no_option, sizeof(no_option), "NO_%s", option);
+
+ return tracefs_instance_file_write(NULL, "osnoise/options", no_option);
+}
+
+#define OSNOISE_OPTION(name, option_str) \
+static int osnoise_get_##name(struct osnoise_context *context) \
+{ \
+ if (context->opt_##name != OSNOISE_OPTION_INIT_VAL) \
+ return context->opt_##name; \
+ \
+ if (context->orig_opt_##name != OSNOISE_OPTION_INIT_VAL) \
+ return context->orig_opt_##name; \
+ \
+ context->orig_opt_##name = osnoise_options_get_option(option_str); \
+ \
+ return context->orig_opt_##name; \
+} \
+ \
+int osnoise_set_##name(struct osnoise_context *context, bool onoff) \
+{ \
+ int opt_##name = osnoise_get_##name(context); \
+ int retval; \
+ \
+ if (opt_##name == OSNOISE_OPTION_INIT_VAL) \
+ return -1; \
+ \
+ if (opt_##name == onoff) \
+ return 0; \
+ \
+ retval = osnoise_options_set_option(option_str, onoff); \
+ if (retval < 0) \
+ return -1; \
+ \
+ context->opt_##name = onoff; \
+ \
+ return 0; \
+} \
+ \
+static void osnoise_restore_##name(struct osnoise_context *context) \
+{ \
+ int retval; \
+ \
+ if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
+ return; \
+ \
+ if (context->orig_opt_##name == context->opt_##name) \
+ goto out_done; \
+ \
+ retval = osnoise_options_set_option(option_str, context->orig_opt_##name); \
+ if (retval < 0) \
+ err_msg("Could not restore original osnoise " #option_str " option\n"); \
+ \
+out_done: \
+ context->orig_opt_##name = OSNOISE_OPTION_INIT_VAL; \
+} \
+ \
+static void osnoise_put_##name(struct osnoise_context *context) \
+{ \
+ osnoise_restore_##name(context); \
+ \
+ if (context->orig_opt_##name == OSNOISE_OPTION_INIT_VAL) \
+ return; \
+ \
+ context->orig_opt_##name = OSNOISE_OPTION_INIT_VAL; \
+}
+
/*
* enable_osnoise - enable osnoise tracer in the trace_instance
*/
--
2.38.1
Re: [PATCH V2 1/6] rtla/osnoise: Add helper functions to manipulate osnoise/options [ In reply to ]
On Tue, 31 Jan 2023 17:30:02 +0100
Daniel Bristot de Oliveira <bristot@kernel.org> wrote:

> Add some helper functions to read and set the on/off osnoise/options.
> No usage in this patch.
>
> In preparation for hwnoise tool.

Honestly, I don't see why patches 1-5 isn't a single patch. It's not that
big of a change, and everything in 1-5 is to do what 5 does. Breaking it up
this fine grain isn't helpful in reviewing, as I found that I had to apply
1-5 and then do a diff from where I started to make sense of any of it.

-- Steve


>
> Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
> Cc: Daniel Bristot de Oliveira <bristot@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> ---
>
Re: [PATCH V2 1/6] rtla/osnoise: Add helper functions to manipulate osnoise/options [ In reply to ]
On Tue, 31 Jan 2023 17:30:02 +0100
Daniel Bristot de Oliveira <bristot@kernel.org> wrote:

> +#define OSNOISE_OPTION(name, option_str) \
> +static int osnoise_get_##name(struct osnoise_context *context) \
> +{ \
> + if (context->opt_##name != OSNOISE_OPTION_INIT_VAL) \
> + return context->opt_##name; \
> + \
> + if (context->orig_opt_##name != OSNOISE_OPTION_INIT_VAL) \
> + return context->orig_opt_##name; \
> + \
> + context->orig_opt_##name = osnoise_options_get_option(option_str); \
> + \
> + return context->orig_opt_##name; \
> +} \
> + \

What you could have done is not make this into a super macro (as there's
only one instance of it). And then add a patch that turns it into this
macro as the first patch of a series that adds another user.

Because I don't understand why this exists when it only has one user.

-- Steve
Re: [PATCH V2 1/6] rtla/osnoise: Add helper functions to manipulate osnoise/options [ In reply to ]
On 2/1/23 20:23, Steven Rostedt wrote:
> On Tue, 31 Jan 2023 17:30:02 +0100
> Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
>
>> Add some helper functions to read and set the on/off osnoise/options.
>> No usage in this patch.
>>
>> In preparation for hwnoise tool.
>
> Honestly, I don't see why patches 1-5 isn't a single patch. It's not that
> big of a change, and everything in 1-5 is to do what 5 does. Breaking it up
> this fine grain isn't helpful in reviewing, as I found that I had to apply
> 1-5 and then do a diff from where I started to make sense of any of it.

Maybe what is missing is a clear:

In preparation for hwnoise tool.

IMHO, it is easier to understand by using small "logical" pieces in preparation
for the "conclusion." But I see your point, and it does not hurt :-).

I will reduce the number of patches.

-- Daniel
>
> -- Steve
>
>
>>
>> Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org>
>> Cc: Daniel Bristot de Oliveira <bristot@kernel.org>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> ---
>>