Re: [Intel-gfx] [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 20/02/2018 21:40, Chris Wilson wrote:
> Rather than iteratively disable and then immediately reenable a CPU,
> turn off each in turn, forcing the PMU events onto the next CPU without
> allowing them to retreat back to CPU0 after the first. If this fails,

Hm, interesting and I think it possibly makes sense to test both 
migration patterns.

> immediately reboot the system.

Yes, we already agreed on this one and I was planning to do it so thanks 
for beating me to it.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   lib/Makefile.sources |  2 ++
>   lib/igt_sysrq.c      | 20 ++++++++++++++++++
>   lib/igt_sysrq.h      | 30 +++++++++++++++++++++++++++
>   lib/meson.build      |  2 ++
>   tests/perf_pmu.c     | 57 ++++++++++++++++++++++++++++++++++------------------
>   5 files changed, 92 insertions(+), 19 deletions(-)
>   create mode 100644 lib/igt_sysrq.c
>   create mode 100644 lib/igt_sysrq.h
> 
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 86fbfeef..e4a9b059 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -33,6 +33,8 @@ lib_source_list =	 	\
>   	igt_stats.h		\
>   	igt_sysfs.c		\
>   	igt_sysfs.h		\
> +	igt_sysrq.c		\
> +	igt_sysrq.h		\
>   	igt_x86.h		\
>   	igt_x86.c		\
>   	igt_vgem.c		\
> diff --git a/lib/igt_sysrq.c b/lib/igt_sysrq.c
> new file mode 100644
> index 00000000..32fb4a39
> --- /dev/null
> +++ b/lib/igt_sysrq.c
> @@ -0,0 +1,20 @@
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <sys/reboot.h>
> +
> +#include "igt_sysrq.h"
> +
> +void igt_sysrq_reboot(void)
> +{
> +	sync();
> +
> +	/* Try to be nice at first, and if that fails pull the trigger */
> +	if (reboot(RB_AUTOBOOT)) {
> +		int fd = open("/proc/sysrq-trigger", O_WRONLY);
> +		write(fd, "b", 2);
> +		close(fd);
> +	}
> +
> +	abort();
> +}
> diff --git a/lib/igt_sysrq.h b/lib/igt_sysrq.h
> new file mode 100644
> index 00000000..422473d2
> --- /dev/null
> +++ b/lib/igt_sysrq.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright © 2018 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_SYSRQ_H__
> +#define __IGT_SYSRQ_H__
> +
> +void igt_sysrq_reboot(void) __attribute__((noreturn));
> +
> +#endif /* __IGT_SYSRQ_H__ */
> diff --git a/lib/meson.build b/lib/meson.build
> index 94ea0799..2c611348 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -21,6 +21,7 @@ lib_headers = [
>   	'igt_stats.h',
>   	'igt_syncobj.h',
>   	'igt_sysfs.h',
> +	'igt_sysrq.h',
>   	'igt_x86.h',
>   	'igt_vgem.h',
>   	'instdone.h',
> @@ -67,6 +68,7 @@ lib_sources = [
>   	'igt_stats.c',
>   	'igt_syncobj.c',
>   	'igt_sysfs.c',
> +	'igt_sysrq.c',
>   	'igt_vgem.c',
>   	'igt_x86.c',
>   	'instdone.c',
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index 7fab73e2..1421fd9a 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -41,6 +41,7 @@
>   #include "igt_core.h"
>   #include "igt_perf.h"
>   #include "igt_sysfs.h"
> +#include "igt_sysrq.h"
>   #include "igt_pm.h"
>   #include "sw_sync.h"
>   
> @@ -957,6 +958,16 @@ static bool cpu0_hotplug_support(void)
>   	return access("/sys/devices/system/cpu/cpu0/online", W_OK) == 0;
>   }
>   
> +static int open_cpu_online(int cpu)
> +{
> +	char name[128];
> +
> +	igt_assert_lt(snprintf(name, sizeof(name),
> +			       "/sys/devices/system/cpu/cpu%d/online",
> +			       cpu), sizeof(name));
> +	return open(name, O_WRONLY);
> +}
> +
>   static void cpu_hotplug(int gem_fd)
>   {
>   	igt_spin_t *spin[2];
> @@ -988,35 +999,43 @@ static void cpu_hotplug(int gem_fd)
>   	 */
>   	igt_fork(child, 1) {
>   		int cpu = 0;
> +		int cpufd;
> +		int err;
>   
>   		close(link[0]);
>   
> +		/* Offline each cpu in turn */
>   		for (;;) {
> -			char name[128];
> -			int cpufd;
> -
> -			igt_assert_lt(snprintf(name, sizeof(name),
> -					       "/sys/devices/system/cpu/cpu%d/online",
> -					       cpu), sizeof(name));
> -			cpufd = open(name, O_WRONLY);
> -			if (cpufd == -1) {
> -				igt_assert(cpu > 0);
> -				/*
> -				 * Signal parent that we cycled through all
> -				 * CPUs and we are done.
> -				 */
> -				igt_assert_eq(write(link[1], "*", 1), 1);
> +			cpufd = open_cpu_online(cpu);
> +			igt_assert(cpufd != -1);
> +
> +			err = write(cpufd, "0", 2);
> +			close(cpufd);
> +			if (err < 0)
>   				break;

Keep off-lining until no more CPUs to offline? I had to try it! :) Ok, 
last one will fail to offline. But I think it needs a comment.

> -			}
>   
> -			/* Offline followed by online a CPU. */
> -			igt_assert_eq(write(cpufd, "0", 2), 2);
>   			usleep(1e6);
> -			igt_assert_eq(write(cpufd, "1", 2), 2);
> +			cpu++;
> +		}
>   
> +		/* Then bring them back online */
> +		while (cpu--) {
> +			cpufd = open_cpu_online(cpu);
> +			err = write(cpufd, "1", 2);
>   			close(cpufd);

Need to online in the same order or the PMU will stay on some higher CPU 
making the subsequent tests fail. Or I need to improve the helpers to 
hunt for the correct CPU, as perf tool does.

> -			cpu++;
> +
> +			if (err < 0) {
> +				igt_warn("Failed to bring CPU%d back online\n",
> +					 cpu);
> +				igt_sysrq_reboot(); > +			}
>   		}
> +
> +		/*
> +		 * Signal parent that we cycled through all
> +		 * CPUs and we are done.
> +		 */
> +		igt_assert_eq(write(link[1], "*", 1), 1);
>   	}
>   
>   	close(link[1]);
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx