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

[sameeruddin shaik <sameeross1994@gmail.com>]

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