Re: [PATCH] libtracefs: Add support for setting tracers

[Steven Rostedt <rostedt@goodmis.org>]

Hi Sameer,

Remember, when submitting a new patch, you should always use -v2 (or
whatever the next version is). That way it's obvious that this is a new
version of the patch.

On Mon, 17 May 2021 14:54:06 +0530
Sameeruddin shaik <sameeross1994@gmail.com> wrote:

> tracefs_set_tracer - set the tracer
> tracefs_stop_tracer - clear the tracer

Actually, let's change the names to be:

	tracefs_tracer_set()
	tracefs_tracer_clear()

The "tracefs_tracer_" keeps all the functions related to tracers stating
with the same text.

"stop" is misleading, because you are not really stopping the tracer, and
"stop" does not match with "set". "clear" better term, and you even used
that in your description of the trace above.


> 
> Signed-off-by: Sameeruddin shaik <sameeross1994@gmail.com>
> 
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 55ee867..0270a9e 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -173,4 +173,23 @@ int tracefs_function_filter(struct tracefs_instance *instance, const char *filte
>  int tracefs_function_notrace(struct tracefs_instance *instance, const char *filter,
>  			     const char *module, unsigned int flags);
>  
> +enum tracefs_tracers {
> +	TRACEFS_TRACER_NOP = 0,
> +	TRACEFS_TRACER_FUNCTION,
> +	TRACEFS_TRACER_FUNCTION_GRAPH,
> +	TRACEFS_TRACER_IRQSOFF,
> +	TRACEFS_TRACER_PREEMPTOFF,
> +	TRACEFS_TRACER_PREEMPTIRQSOFF,
> +	TRACEFS_TRACER_WAKEUP,
> +	TRACEFS_TRACER_WAKEUP_RT,
> +	TRACEFS_TRACER_WAKEUP_DL,
> +	TRACEFS_TRACER_MMIOTRACE,
> +	TRACEFS_TRACER_HWLAT,
> +	TRACEFS_TRACER_BRANCH,
> +	TRACEFS_TRACER_BLOCK,
> +};
> +
> +int tracefs_set_tracer(struct tracefs_instance *instance, enum tracefs_tracers tracer);
> +
> +int tracefs_stop_tracer(struct tracefs_instance *instance);
>  #endif /* _TRACE_FS_H */
> diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
> index 6ef17f6..d772f93 100644
> --- a/src/tracefs-tools.c
> +++ b/src/tracefs-tools.c
> @@ -25,6 +25,30 @@ __hidden pthread_mutex_t toplevel_lock = PTHREAD_MUTEX_INITIALIZER;
>  #define TRACE_FILTER		"set_ftrace_filter"
>  #define TRACE_NOTRACE		"set_ftrace_notrace"
>  #define TRACE_FILTER_LIST	"available_filter_functions"
> +#define CUR_TRACER		"current_tracer"
> +
> +#define TRACERS \
> +	C(NOP,                  "nop"),			\
> +	C(FUNCTION,             "function"),            \
> +	C(FUNCTION_GRAPH,       "function_graph"),      \
> +	C(IRQSOFF,              "irqsoff"),             \
> +	C(PREEMPTOFF,           "preemptoff"),          \
> +	C(PREEMPTIRQSOFF,       "preemptirqsoff"),      \
> +	C(WAKEUP,               "wakeup"),              \
> +	C(WAKEUP_RT,            "wakeup_rt"),	\
> +	C(WAKEUP_DL,            "wakeup_dl"),           \
> +	C(MMIOTRACE,            "mmiotrace"),           \
> +	C(HWLAT,                "hwlat"),               \
> +	C(BRANCH,               "branch"),              \
> +	C(BLOCK,                "block")
> +
> +#undef C
> +#define C(a, b) b
> +const char *tracers[] = { TRACERS };
> +
> +#undef C
> +#define C(a, b) TRACEFS_TRACER_##a
> +const int tracer_enums[] = { TRACERS };
>  
>  /* File descriptor for Top level set_ftrace_filter  */
>  static int ftrace_filter_fd = -1;
> @@ -910,3 +934,68 @@ int tracefs_function_notrace(struct tracefs_instance *instance, const char *filt
>  	tracefs_put_tracing_file(filter_path);
>  	return ret;
>  }
> +
> +int write_tracer(int fd, const char *tracer)
> +{
> +	int ret;
> +
> +	ret = write(fd, tracer, strlen(tracer));
> +	if (ret < strlen(tracer))
> +		return -1;
> +	return ret;
> +}
> +
> +/**
> + * tracefs_set_tracer - function to set the tracer
> + * @instance: ftrace instance, can be NULL for top tracing instance
> + * @tracer: Tracer that has to be set, which can be integer from 0 - 13
> + * or enum value
> + */
> +
> +int tracefs_set_tracer(struct tracefs_instance *instance, enum tracefs_tracers tracer)
> +{
> +	char *tracer_path = NULL;
> +	const char *t = NULL;
> +	int ret = -1;
> +	int fd = -1;
> +	int i;
> +
> +	tracer_path = tracefs_instance_get_file(instance, CUR_TRACER);
> +	if (!tracer_path)
> +		return -1;
> +
> +	fd = open(tracer_path, O_WRONLY);
> +	if (fd < 0) {
> +		errno = -ENOENT;
> +		goto out;
> +	}
> +
> +	if (tracer < 0 || tracer > ARRAY_SIZE(tracers)) {
> +		errno = -ENODEV;
> +		goto out;
> +	}

Space needed here, as well as a comment.

> +	if (tracer == tracer_enums[tracer])
> +		t = tracers[tracer];
> +	else {
> +		for (i = 0; i < ARRAY_SIZE(tracer_enums); i++) {
> +			if (tracer == tracer_enums[i]) {
> +				t = tracers[i];
> +				break;
> +			}
> +		}
> +	}
> +	if (!t) {
> +		errno = -EINVAL;
> +		goto out;
> +	}
> +	ret = write_tracer(fd, t);
> + out:
> +	tracefs_put_tracing_file(tracer_path);
> +	close(fd);
> +	return ret;

BTW, when a reviewer of your code gives a code example of a better
implementation, you should express that in your change log. I usually use:

 Suggested-by: ...

So you should have:

 Suggested-by: Steven Rostedt <rostedt@goodmis.org>

as the above is pretty much exact copy of the code snippet I posted.

Here's an example of what I have done when I do the same:

 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.12-rc5&id=ceaaa12904df07d07ea8975abbf04c4d60e46956

-- Steve

> +}
> +
> +int  tracefs_stop_tracer(struct tracefs_instance *instance)
> +{
> +	return tracefs_set_tracer(instance, TRACEFS_TRACER_NOP);
> +}