[V2,1/6] rtla/osnoise: Add helper functions to manipulate osnoise/options

Message ID 930c4ef71c7bcb1158d2a8cad905f4de425b8d1e.1675181734.git.bristot@kernel.org
State New
Headers
Series rtla: Add hwnoise tool |

Commit Message

Daniel Bristot de Oliveira Jan. 31, 2023, 4:30 p.m. UTC
  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(+)
  

Comments

Steven Rostedt Feb. 1, 2023, 7:23 p.m. UTC | #1
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>
> ---
>
  
Steven Rostedt Feb. 1, 2023, 7:25 p.m. UTC | #2
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
  
Daniel Bristot de Oliveira Feb. 7, 2023, 8:04 p.m. UTC | #3
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>
>> ---
>>
  

Patch

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