diff mbox series

libtracefs: Add support for setting tracers

Message ID 1621243446-7402-1-git-send-email-sameeross1994@gmail.com (mailing list archive)
State Superseded
Headers show
Series libtracefs: Add support for setting tracers | expand

Commit Message

Sameeruddin shaik May 17, 2021, 9:24 a.m. UTC 
tracefs_set_tracer - set the tracer
tracefs_stop_tracer - clear the tracer

Signed-off-by: Sameeruddin shaik <sameeross1994@gmail.com>

Comments

Steven Rostedt May 17, 2021, 1:05 p.m. UTC | #1 
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);
> +}
Steven Rostedt May 17, 2021, 3:21 p.m. UTC | #2 
On Tue, 18 May 2021 20:48:35 +0530
sameeruddin shaik <sameeross1994@gmail.com> wrote:

> > 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.  
> okay steve,
> since i don't know, about this suggested-by, i haven't included. otherwise
> i could have done.

No problem. I'm just educating you ;-)

-- Steve
Sameeruddin shaik May 18, 2021, 3:18 p.m. UTC | #3 
On Mon, May 17, 2021 at 6:35 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> 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.
okay steve,
since i don't know, about this suggested-by, i haven't included. otherwise
i could have done.

Thanks,
sameer
diff mbox series

Patch

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;
+	}
+	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;
+}
+
+int  tracefs_stop_tracer(struct tracefs_instance *instance)
+{
+	return tracefs_set_tracer(instance, TRACEFS_TRACER_NOP);
+}