Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add hooks for enabling ftrace

[Ashutosh Dixit <ashutosh.dixit@intel.com>]

On Sat, 16 Mar 2019 02:25:04 -0700, Chris Wilson wrote:
>
> If the kernel has tracing support builtin, we can enable around
> troublesome tests to obtain greater detail from the kernel that may
> prove invaluable in debugging the problem.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I have a few general comments on the patch. I have submitted a patch [1]
which has some overlap with the functionality enabled here so I am thinking
if it is possible to combine the best parts of both the patches. I am aware
that coming up with a general hard-coded solution for these sort of traces
is difficult.

a. Is the function graph tracer the most useful tracer to use as is done in
   this patch? Or a nop tracer, or it doesn't matter?
b. Do we need to increase the size of the ftrace ring to reduce the
   likelihood of the ftrace ring overflow?
c. This patch dumps out the ftrace buffer when there is a kernel
   oops. However, afaik, the audio issues we are seeing result in test
   failures but there are no oops.
d. Even when there is an oops, ftrace output from this patch will contain
   the output from all previous sub-tests, not just the sub-test which
   caused the oops. I had tried to take care of c. and d. in my patch.

Thanks!

[1] https://patchwork.freedesktop.org/patch/292574/

> ---
>  lib/Makefile.sources |   2 +
>  lib/igt_core.c       |  28 ++----
>  lib/igt_ftrace.c     | 204 +++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_ftrace.h     |  46 ++++++++++
>  lib/meson.build      |   1 +
>  5 files changed, 258 insertions(+), 23 deletions(-)
>  create mode 100644 lib/igt_ftrace.c
>  create mode 100644 lib/igt_ftrace.h
>
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index cf2720981..24ee5f094 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -26,6 +26,8 @@ lib_source_list =		\
>	igt_color_encoding.c	\
>	igt_color_encoding.h	\
>	igt_edid_template.h	\
> +	igt_ftrace.c		\
> +	igt_ftrace.h		\
>	igt_gt.c		\
>	igt_gt.h		\
>	igt_gvt.c		\
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 6eb4798e2..3e69cf66c 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -62,6 +62,7 @@
>  #include "intel_io.h"
>  #include "igt_debugfs.h"
>  #include "igt_dummyload.h"
> +#include "igt_ftrace.h"
>  #include "version.h"
>  #include "config.h"
>
> @@ -490,31 +491,11 @@ void __igt_fixture_end(void)
>	siglongjmp(igt_subtest_jmpbuf, 1);
>  }
>
> -/*
> - * If the test takes out the machine, in addition to the usual dmesg
> - * spam, the kernel may also emit a "death rattle" -- extra debug
> - * information that is overkill for normal successful tests, but
> - * vital for post-mortem analysis.
> - */
> -static void ftrace_dump_on_oops(bool enable)
> -{
> -	int fd;
> -
> -	fd = open("/proc/sys/kernel/ftrace_dump_on_oops", O_WRONLY);
> -	if (fd < 0)
> -		return;
> -
> -	/*
> -	 * If we fail, we do not get the death rattle we wish, but we
> -	 * still want to run the tests anyway.
> -	 */
> -	igt_ignore_warn(write(fd, enable ? "1\n" : "0\n", 2));
> -	close(fd);
> -}
> -
>  bool igt_exit_called;
>  static void common_exit_handler(int sig)
>  {
> +	igt_ftrace_close();
> +
>	if (!igt_only_list_subtests()) {
>		bind_fbcon(true);
>	}
> @@ -812,7 +793,8 @@ out:
>
>		sync();
>		oom_adjust_for_doom();
> -		ftrace_dump_on_oops(true);
> +
> +		igt_ftrace_open();
>	}
>
>	/* install exit handler, to ensure we clean up */
> diff --git a/lib/igt_ftrace.c b/lib/igt_ftrace.c
> new file mode 100644
> index 000000000..428b35132
> --- /dev/null
> +++ b/lib/igt_ftrace.c
> @@ -0,0 +1,204 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "igt.h"
> +#include "igt_ftrace.h"
> +#include "igt_debugfs.h"
> +#include "igt_sysfs.h"
> +
> +#define BIT(x) (1ul << (x))
> +
> +/* Only a single tracer in the kernel, so we can use a singleton */
> +struct igt_ftrace {
> +	int dir;
> +
> +	unsigned long flags;
> +#define PID_SET BIT(0)
> +#define INCLUDE_SET BIT(1)
> +#define EXCLUDE_SET BIT(2)
> +
> +} igt_ftrace = { -1 };
> +
> +/*
> + * If the test takes out the machine, in addition to the usual dmesg
> + * spam, the kernel may also emit a "death rattle" -- extra debug
> + * information that is overkill for normal successful tests, but
> + * vital for post-mortem analysis.
> + */
> +static void ftrace_dump_on_oops(bool enable)
> +{
> +	int fd;
> +
> +	fd = open("/proc/sys/kernel/ftrace_dump_on_oops", O_WRONLY);
> +	if (fd < 0)
> +		return;
> +
> +	/*
> +	 * If we fail, we do not get the death rattle we wish, but we
> +	 * still want to run the tests anyway.
> +	 */
> +	igt_ignore_warn(write(fd, enable ? "1\n" : "0\n", 2));
> +	close(fd);
> +}
> +
> +int igt_ftrace_open(void)
> +{
> +	int dir;
> +	int err;
> +
> +	dir = open(igt_debugfs_mount(), O_RDONLY);
> +	if (dir < 0)
> +		return -errno;
> +
> +	igt_ftrace.dir = openat(dir, "tracing", O_RDONLY);
> +	close(dir);
> +	if (igt_ftrace.dir < 0)
> +		return -errno;
> +
> +	err = igt_ftrace_disable();
> +	if (err)
> +		goto ft_close;
> +
> +	ftrace_dump_on_oops(true);
> +	return 0;
> +
> +ft_close:
> +	close(igt_ftrace.dir);
> +	igt_ftrace.dir = -1;
> +	return err;
> +}
> +
> +int __igt_ftrace_enable(const char *mode,
> +			const struct igt_ftrace_options *opts)
> +{
> +	int err;
> +
> +	if (igt_ftrace.dir < 0)
> +		return -ENODEV;
> +
> +	if (!mode)
> +		return -EINVAL;
> +
> +	err = igt_sysfs_set(igt_ftrace.dir, "current_tracer", mode);
> +	if (err < 0)
> +		return err;
> +
> +	if (opts && opts->pid) {
> +		err = igt_sysfs_printf(igt_ftrace.dir,
> +				       "set_ftrace_pid", "%d",
> +				       opts->pid);
> +		if (err < 0)
> +			goto disable;
> +
> +		igt_ftrace.flags |= PID_SET;
> +	}
> +
> +	if (opts && opts->include) {
> +		err = igt_sysfs_set(igt_ftrace.dir,
> +				    "set_ftrace_filter",
> +				    opts->include);
> +		if (err < 0)
> +			goto disable;
> +
> +		igt_ftrace.flags |= INCLUDE_SET;
> +	}
> +
> +	if (opts && opts->exclude) {
> +		err = igt_sysfs_set(igt_ftrace.dir,
> +				    "set_ftrace_notrace",
> +				    opts->exclude);
> +		if (err < 0)
> +			goto disable;
> +
> +		igt_ftrace.flags |= EXCLUDE_SET;
> +	}
> +
> +	err = igt_sysfs_set(igt_ftrace.dir, "tracer_on", "1");
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +
> +disable:
> +	igt_ftrace_disable();
> +	return err;
> +}
> +
> +int igt_ftrace_disable(void)
> +{
> +	int err;
> +
> +	if (igt_ftrace.dir < 0)
> +		return -ENODEV;
> +
> +	err = igt_sysfs_set(igt_ftrace.dir, "tracer_on", "0");
> +	if (err < 0)
> +		return err;
> +
> +	if (igt_ftrace.flags & PID_SET) {
> +		igt_sysfs_set(igt_ftrace.dir, "set_ftrace_pid", "");
> +		igt_ftrace.flags &= ~PID_SET;
> +	}
> +
> +	if (igt_ftrace.flags & INCLUDE_SET) {
> +		igt_sysfs_set(igt_ftrace.dir, "set_ftrace_filter", "");
> +		igt_ftrace.flags &= ~INCLUDE_SET;
> +	}
> +
> +	if (igt_ftrace.flags & EXCLUDE_SET) {
> +		igt_sysfs_set(igt_ftrace.dir, "set_ftrace_notrace", "");
> +		igt_ftrace.flags &= ~EXCLUDE_SET;
> +	}
> +
> +	return 0;
> +}
> +
> +void igt_ftrace_dump(const char *header)
> +{
> +	char *txt;
> +
> +	if (igt_ftrace.dir < 0)
> +		return;
> +
> +	txt = igt_sysfs_get(igt_ftrace.dir, "trace");
> +	if (!txt)
> +		return;
> +
> +	igt_info("%s:\n%s", header, txt);
> +	free(txt);
> +}
> +
> +void igt_ftrace_close(void)
> +{
> +	if (igt_ftrace.dir < 0)
> +		return;
> +
> +	close(igt_ftrace.dir);
> +	igt_ftrace.dir = -1;
> +}
> diff --git a/lib/igt_ftrace.h b/lib/igt_ftrace.h
> new file mode 100644
> index 000000000..3dd676829
> --- /dev/null
> +++ b/lib/igt_ftrace.h
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef IGT_FTRACE_H
> +#define IGT_FTRACE_H
> +
> +int igt_ftrace_open(void);
> +
> +struct igt_ftrace_options {
> +	int pid;
> +	const char *include;
> +	const char *exclude;
> +};
> +
> +int __igt_ftrace_enable(const char *mode,
> +			const struct igt_ftrace_options *opts);
> +#define igt_ftrace_enable() __igt_ftrace_enable("function_graph", NULL)
> +
> +int igt_ftrace_disable(void);
> +
> +void igt_ftrace_dump(const char *header);
> +
> +void igt_ftrace_close(void);
> +
> +#endif /* IGT_FTRACE_H */
> diff --git a/lib/meson.build b/lib/meson.build
> index 0eb5585d7..2f0f65a08 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -9,6 +9,7 @@ lib_sources = [
>	'igt_debugfs.c',
>	'igt_device.c',
>	'igt_aux.c',
> +	'igt_ftrace.c',
>	'igt_gt.c',
>	'igt_gvt.c',
>	'igt_matrix.c',
> --
> 2.20.1
>
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev