linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] libtracefs: Add support for setting tracers
@ 2021-05-17  9:24 Sameeruddin shaik
  2021-05-17 13:05 ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: Sameeruddin shaik @ 2021-05-17  9:24 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Sameeruddin shaik

tracefs_set_tracer - set the tracer
tracefs_stop_tracer - clear the tracer

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;
+	}
+	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);
+}
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libtracefs: Add support for setting tracers
  2021-05-17  9:24 [PATCH] libtracefs: Add support for setting tracers Sameeruddin shaik
@ 2021-05-17 13:05 ` Steven Rostedt
  2021-05-18 15:18   ` sameeruddin shaik
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2021-05-17 13:05 UTC (permalink / raw)
  To: Sameeruddin shaik; +Cc: linux-trace-devel


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);
> +}


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libtracefs: Add support for setting tracers
  2021-05-18 15:18   ` sameeruddin shaik
@ 2021-05-17 15:21     ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2021-05-17 15:21 UTC (permalink / raw)
  To: sameeruddin shaik; +Cc: linux-trace-devel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libtracefs: Add support for setting tracers
  2021-05-17 13:05 ` Steven Rostedt
@ 2021-05-18 15:18   ` sameeruddin shaik
  2021-05-17 15:21     ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: sameeruddin shaik @ 2021-05-18 15:18 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] libtracefs: Add support for setting tracers
@ 2021-05-18 15:26 Sameeruddin shaik
  2021-05-17 15:32 ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: Sameeruddin shaik @ 2021-05-18 15:26 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, tz.stoyanov, Sameeruddin shaik

tracefs_tracer_set - set the tracer
tracefs_tracer_clear - clear the tracer

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Sameeruddin shaik <sameeross1994@gmail.com>

diff --git a/include/tracefs.h b/include/tracefs.h
index 55ee867..e261b8a 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_tracer_set(struct tracefs_instance *instance, enum tracefs_tracers tracer);
+
+int tracefs_tracer_clear(struct tracefs_instance *instance);
 #endif /* _TRACE_FS_H */
diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 6ef17f6..f535231 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 - 12
+ * or enum value
+ */
+
+int tracefs_tracer_set(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_tracer_clear(struct tracefs_instance *instance)
+{
+	return tracefs_tracer_set(instance, TRACEFS_TRACER_NOP);
+}
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libtracefs: Add support for setting tracers
  2021-05-18 15:26 Sameeruddin shaik
@ 2021-05-17 15:32 ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2021-05-17 15:32 UTC (permalink / raw)
  To: Sameeruddin shaik; +Cc: linux-trace-devel, tz.stoyanov

On Tue, 18 May 2021 20:56:42 +0530
Sameeruddin shaik <sameeross1994@gmail.com> wrote:

> tracefs_tracer_set - set the tracer
> tracefs_tracer_clear - clear the tracer
> 
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Sameeruddin shaik <sameeross1994@gmail.com>
> 
> 

Thanks Sameer,


The subject still needs to include the version:

"[PATCH v3] libtracefs: Add support for setting tracers"

You don't need to send another version just to fix this. But next time,
please label the versions of any patch that is replacing another patch.

I'm going on PTO tomorrow for almost two weeks, and will not be able to
process this till I get back. I probably wont be able to take this until
June.

-- Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] libtracefs: Add support for setting tracers
@ 2021-05-16 14:59 Sameeruddin shaik
  2021-05-15 16:40 ` Steven Rostedt
  0 siblings, 1 reply; 8+ messages in thread
From: Sameeruddin shaik @ 2021-05-16 14:59 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Sameeruddin shaik

tracefs_set_tracer - set the tracer
tracefs_stop_tracer - clear the tracer

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

diff --git a/include/tracefs.h b/include/tracefs.h
index 55ee867..47d3282 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -173,4 +173,26 @@ 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);
 
+/*
+ * Tracers
+ */
+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..ceddeda 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -25,6 +25,7 @@ __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 TRACER			"current_tracer"
 
 /* File descriptor for Top level set_ftrace_filter  */
 static int ftrace_filter_fd = -1;
@@ -910,3 +911,88 @@ 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 - fucntion 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 and enum value
+*/
+
+int tracefs_set_tracer(struct tracefs_instance *instance, enum tracefs_tracers tracer)
+{
+	char *tracer_path;
+	int ret = -1;
+	int fd = -1;
+
+	tracer_path = tracefs_instance_get_file(instance, TRACER);
+	if (!tracer_path)
+		return -1;
+
+	fd = open(tracer_path, O_WRONLY);
+	if (fd < 0)
+		goto out;
+
+	switch(tracer) {
+	case 0:
+		ret = write_tracer(fd , "nop");
+		break;
+	case 1:
+		ret = write_tracer(fd, "function");
+		break;
+	case 2:
+		ret = write_tracer(fd, "function_graph");
+		break;
+	case 3:
+		ret = write_tracer(fd, "irqsoff");
+		break;
+	case 4:
+		ret = write_tracer(fd, "preemptoff");
+		break;
+	case 5:
+		ret = write_tracer(fd, "preemptirqsoff");
+		break;
+	case 6:
+		ret = write_tracer(fd, "wakeup");
+		break;
+	case 7:
+		ret = write_tracer(fd, "wakeup_rt");
+		break;
+	case 8:
+		ret = write_tracer(fd, "wakeup_dl");
+		break;
+	case 9:
+		ret = write_tracer(fd, "mmiotrace");
+		break;
+	case 10:
+		ret = write_tracer(fd, "hwlat");
+		break;
+	case 11:
+		ret = write_tracer(fd, "branch");
+		break;
+	case 12:
+		ret = write_tracer(fd, "blk");
+		break;
+	default:
+		ret = -1;
+	}
+
+ out:
+	tracefs_put_tracing_file(tracer_path);
+	return ret;
+}
+
+int  tracefs_stop_tracer(struct tracefs_instance *instance)
+{
+	return tracefs_set_tracer(instance, TRACEFS_TRACER_NOP);
+}
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libtracefs: Add support for setting tracers
  2021-05-16 14:59 Sameeruddin shaik
@ 2021-05-15 16:40 ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2021-05-15 16:40 UTC (permalink / raw)
  To: Sameeruddin shaik; +Cc: linux-trace-devel


Hi Sameer,


On Sun, 16 May 2021 20:29:19 +0530
Sameeruddin shaik <sameeross1994@gmail.com> wrote:

> tracefs_set_tracer - set the tracer
> tracefs_stop_tracer - clear the tracer
> 
> Signed-off-by: Sameeruddin shaik <sameeross1994@gmail.com>
> 
> diff --git a/include/tracefs.h b/include/tracefs.h
> index 55ee867..47d3282 100644
> --- a/include/tracefs.h
> +++ b/include/tracefs.h
> @@ -173,4 +173,26 @@ 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);
>  
> +/*
> + * Tracers
> + */
> +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..ceddeda 100644
> --- a/src/tracefs-tools.c
> +++ b/src/tracefs-tools.c
> @@ -25,6 +25,7 @@ __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 TRACER			"current_tracer"
>  
>  /* File descriptor for Top level set_ftrace_filter  */
>  static int ftrace_filter_fd = -1;
> @@ -910,3 +911,88 @@ 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 - fucntion 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 and enum value
> +*/
> +
> +int tracefs_set_tracer(struct tracefs_instance *instance, enum tracefs_tracers tracer)
> +{
> +	char *tracer_path;
> +	int ret = -1;
> +	int fd = -1;
> +
> +	tracer_path = tracefs_instance_get_file(instance, TRACER);
> +	if (!tracer_path)
> +		return -1;
> +
> +	fd = open(tracer_path, O_WRONLY);
> +	if (fd < 0)
> +		goto out;
> +
> +	switch(tracer) {
> +	case 0:
> +		ret = write_tracer(fd , "nop");
> +		break;
> +	case 1:
> +		ret = write_tracer(fd, "function");
> +		break;
> +	case 2:
> +		ret = write_tracer(fd, "function_graph");
> +		break;
> +	case 3:
> +		ret = write_tracer(fd, "irqsoff");
> +		break;
> +	case 4:
> +		ret = write_tracer(fd, "preemptoff");
> +		break;
> +	case 5:
> +		ret = write_tracer(fd, "preemptirqsoff");
> +		break;
> +	case 6:
> +		ret = write_tracer(fd, "wakeup");
> +		break;
> +	case 7:
> +		ret = write_tracer(fd, "wakeup_rt");
> +		break;
> +	case 8:
> +		ret = write_tracer(fd, "wakeup_dl");
> +		break;
> +	case 9:
> +		ret = write_tracer(fd, "mmiotrace");
> +		break;
> +	case 10:
> +		ret = write_tracer(fd, "hwlat");
> +		break;
> +	case 11:
> +		ret = write_tracer(fd, "branch");
> +		break;
> +	case 12:
> +		ret = write_tracer(fd, "blk");
> +		break;
> +	default:

Note, hardcoded numbers above is frowned upon in computer science in
general. But you already have the enums, why didn't you use them?

But besides the point, you don't even need a switch statement.

#define TRACERS \
	C(NOP,			"nop"),			\
	C(FUNCTION,		"function"),		\
	C(FUNCTION_GRAPH,	"function_graph"),	\
	C(IRQSOFF,		"irqsoff"),		\
	C(PREEMPTOFF,		"preemptoff"),		\
	C(PREEMPTIQRSOFF,	"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 };

#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))

{
	const char *t = NULL;
	[..]

	if (tracer < 0 || tracer >= ARRAY_SIZE(tracers)) {
		errno = -ENODEV;
		return -1;
	}

	/* if we didn't make a mistake in our mapping */
	if (tracer == tracer_enums[tracer]) {
		t = tracers[tracer];
	} else {
		for (i = 0; i < ARRAY_SIZE(tracer_enums)) {
			if (tracer == tracer_enums[tracer]) {
				t = tracers[i];
				break;
			}
		}
	}

	if (!t) {
		/* Something went horribly wrong */
		errno = -EINVAL;
		return -1;
	}

	ret = write_tracer(fd, t);

Or something like the above. Much more robust, much more maintainable.

-- Steve



> +		ret = -1;
> +	}
> +
> + out:
> +	tracefs_put_tracing_file(tracer_path);
> +	return ret;
> +}
> +
> +int  tracefs_stop_tracer(struct tracefs_instance *instance)
> +{
> +	return tracefs_set_tracer(instance, TRACEFS_TRACER_NOP);
> +}


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-05-17 16:34 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17  9:24 [PATCH] libtracefs: Add support for setting tracers Sameeruddin shaik
2021-05-17 13:05 ` Steven Rostedt
2021-05-18 15:18   ` sameeruddin shaik
2021-05-17 15:21     ` Steven Rostedt
  -- strict thread matches above, loose matches on Subject: below --
2021-05-18 15:26 Sameeruddin shaik
2021-05-17 15:32 ` Steven Rostedt
2021-05-16 14:59 Sameeruddin shaik
2021-05-15 16:40 ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).