All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH igt] igt/perf_pmu: Disable all cpus
@ 2018-02-20 21:40 ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-20 21:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev

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,
immediately reboot the system.

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;
-			}
 
-			/* 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);
-			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]);
-- 
2.16.1

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

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

* [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
@ 2018-02-20 21:40 ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-20 21:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: igt-dev, Tvrtko Ursulin

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,
immediately reboot the system.

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;
-			}
 
-			/* 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);
-			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]);
-- 
2.16.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] ✓ Fi.CI.BAT: success for igt/perf_pmu: Disable all cpus
  2018-02-20 21:40 ` [igt-dev] " Chris Wilson
  (?)
@ 2018-02-20 22:16 ` Patchwork
  -1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-02-20 22:16 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: igt/perf_pmu: Disable all cpus
URL   : https://patchwork.freedesktop.org/series/38635/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
4340b479e7769eb619249600328b916fb16949da igt/gem_fenced_exec_thrash: Use fixed durations

with latest DRM-Tip kernel build CI_DRM_3813
f727568c3b37 drm-tip: 2018y-02m-20d-20h-39m-03s UTC integration manifest

No testlist changes.

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:421s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:485s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:286s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:479s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:468s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:568s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:283s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:510s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:385s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:442s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:413s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:491s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:491s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:600s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:424s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:522s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:404s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:428s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:539s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:398s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_965/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* [igt-dev] ✗ Fi.CI.IGT: failure for igt/perf_pmu: Disable all cpus
  2018-02-20 21:40 ` [igt-dev] " Chris Wilson
  (?)
  (?)
@ 2018-02-21  7:11 ` Patchwork
  -1 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-02-21  7:11 UTC (permalink / raw)
  To: Chris Wilson; +Cc: igt-dev

== Series Details ==

Series: igt/perf_pmu: Disable all cpus
URL   : https://patchwork.freedesktop.org/series/38635/
State : failure

== Summary ==

Test gem_eio:
        Subgroup in-flight-contexts:
                fail       -> PASS       (shard-hsw) fdo#104676 +1
Test kms_flip:
        Subgroup 2x-plain-flip-fb-recreate:
                pass       -> FAIL       (shard-hsw) fdo#100368 +1
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                fail       -> PASS       (shard-snb) fdo#103925 +1
Test kms_vblank:
        Subgroup pipe-c-wait-idle:
                dmesg-warn -> PASS       (shard-hsw)
Test perf_pmu:
        Subgroup busy-idle-bcs0:
                pass       -> FAIL       (shard-snb)
        Subgroup other-init-2:
                pass       -> FAIL       (shard-snb)
                pass       -> FAIL       (shard-hsw)
        Subgroup other-init-1:
                pass       -> FAIL       (shard-snb)
                pass       -> FAIL       (shard-hsw)
        Subgroup all-busy-check-all:
                pass       -> FAIL       (shard-snb)
                pass       -> FAIL       (shard-hsw)
        Subgroup init-wait-vecs0:
                pass       -> FAIL       (shard-snb)
                pass       -> FAIL       (shard-hsw)
        Subgroup busy-start-bcs0:
                pass       -> FAIL       (shard-snb)
                pass       -> FAIL       (shard-hsw)
        Subgroup render-node-busy-idle-vcs0:
                pass       -> FAIL       (shard-snb)
                pass       -> FAIL       (shard-hsw)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-pri-indfb-multidraw:
                pass       -> FAIL       (shard-snb) fdo#103167
Test perf:
        Subgroup enable-disable:
                pass       -> FAIL       (shard-apl) fdo#103715

fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103715 https://bugs.freedesktop.org/show_bug.cgi?id=103715

shard-apl        total:3389 pass:1775 dwarn:1   dfail:0   fail:10  skip:1602 time:12046s
shard-hsw        total:3369 pass:1719 dwarn:1   dfail:0   fail:9   skip:1638 time:11212s
shard-snb        total:3429 pass:1342 dwarn:1   dfail:0   fail:10  skip:2076 time:6606s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_965/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
  2018-02-20 21:40 ` [igt-dev] " Chris Wilson
@ 2018-02-21  9:11   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-02-21  9:11 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


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

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

* Re: [Intel-gfx] [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
@ 2018-02-21  9:11   ` Tvrtko Ursulin
  0 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-02-21  9:11 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


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

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

* Re: [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
  2018-02-21  9:11   ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-02-21  9:17     ` Chris Wilson
  -1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-21  9:17 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2018-02-21 09:11:15)
> 
> 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.

Yup.

> > @@ -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.

I thought that was a fun trick to try and offline the last cpu :)

> > -                     }
> >   
> > -                     /* 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.

Ah. I was expecting everytime we onlined a new cpu, the notifier would
move the pmu. Why do the subsequent tests fail? In my naivety I expected
one CPU is as good as any other for pmu. Do we need to put a trivial
test inside the online/offline loops?
-Chris

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

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

* Re: [Intel-gfx] [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
@ 2018-02-21  9:17     ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-21  9:17 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2018-02-21 09:11:15)
> 
> 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.

Yup.

> > @@ -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.

I thought that was a fun trick to try and offline the last cpu :)

> > -                     }
> >   
> > -                     /* 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.

Ah. I was expecting everytime we onlined a new cpu, the notifier would
move the pmu. Why do the subsequent tests fail? In my naivety I expected
one CPU is as good as any other for pmu. Do we need to put a trivial
test inside the online/offline loops?
-Chris

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

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

* Re: [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
  2018-02-21  9:17     ` [Intel-gfx] " Chris Wilson
@ 2018-02-21  9:24       ` Tvrtko Ursulin
  -1 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-02-21  9:24 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev


On 21/02/2018 09:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-21 09:11:15)
>>
>> 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.
> 
> Yup.
> 
>>> @@ -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.
> 
> I thought that was a fun trick to try and offline the last cpu :)
> 
>>> -                     }
>>>    
>>> -                     /* 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.
> 
> Ah. I was expecting everytime we onlined a new cpu, the notifier would
> move the pmu. Why do the subsequent tests fail? In my naivety I expected
> one CPU is as good as any other for pmu. Do we need to put a trivial
> test inside the online/offline loops?

It only moves it to the first available CPU once it gets kicked out from 
the one it was on.

So with the above pattern of offline all and online in reverse, it will 
happily stay on the last CPU. And IGTs only try to open on the first CPU 
so it fails to open it from then on.

As I said, it would be easy to support opening our PMU regardless on 
which CPU it currently lives on by wrapping the "try the next cpu" logic 
in the perf open wrappers. In kernel perf tool for instance does that 
already.

I could also change i915 to always try to move to CPU0 if it is 
available, on any online events. But I am not sure that's in the spirit 
of things.

I think making IGT perf open wrapper more robust makes most sense.

Regards,

Tvrtko




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

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

* Re: [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
@ 2018-02-21  9:24       ` Tvrtko Ursulin
  0 siblings, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2018-02-21  9:24 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: igt-dev, Tvrtko Ursulin


On 21/02/2018 09:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-21 09:11:15)
>>
>> 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.
> 
> Yup.
> 
>>> @@ -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.
> 
> I thought that was a fun trick to try and offline the last cpu :)
> 
>>> -                     }
>>>    
>>> -                     /* 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.
> 
> Ah. I was expecting everytime we onlined a new cpu, the notifier would
> move the pmu. Why do the subsequent tests fail? In my naivety I expected
> one CPU is as good as any other for pmu. Do we need to put a trivial
> test inside the online/offline loops?

It only moves it to the first available CPU once it gets kicked out from 
the one it was on.

So with the above pattern of offline all and online in reverse, it will 
happily stay on the last CPU. And IGTs only try to open on the first CPU 
so it fails to open it from then on.

As I said, it would be easy to support opening our PMU regardless on 
which CPU it currently lives on by wrapping the "try the next cpu" logic 
in the perf open wrappers. In kernel perf tool for instance does that 
already.

I could also change i915 to always try to move to CPU0 if it is 
available, on any online events. But I am not sure that's in the spirit 
of things.

I think making IGT perf open wrapper more robust makes most sense.

Regards,

Tvrtko




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
  2018-02-21  9:24       ` Tvrtko Ursulin
@ 2018-02-21  9:30         ` Chris Wilson
  -1 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-21  9:30 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev

Quoting Tvrtko Ursulin (2018-02-21 09:24:46)
> As I said, it would be easy to support opening our PMU regardless on 
> which CPU it currently lives on by wrapping the "try the next cpu" logic 
> in the perf open wrappers. In kernel perf tool for instance does that 
> already.
> 
> I could also change i915 to always try to move to CPU0 if it is 
> available, on any online events. But I am not sure that's in the spirit 
> of things.

Ok, I got lost at why i915 perf requires a specific CPU, it is just that
is part of the perf ABI, right?

> I think making IGT perf open wrapper more robust makes most sense.

Yeah, if it's part of the ABI we shouldn't just expect CPU0. Then
throwing a test of the robust perf open in the midst of online/offline
seems sensible.

I guess we will will end up with a perf open before and after each
stage, and keep the pmu fd around until the end.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus
@ 2018-02-21  9:30         ` Chris Wilson
  0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2018-02-21  9:30 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev, Tvrtko Ursulin

Quoting Tvrtko Ursulin (2018-02-21 09:24:46)
> As I said, it would be easy to support opening our PMU regardless on 
> which CPU it currently lives on by wrapping the "try the next cpu" logic 
> in the perf open wrappers. In kernel perf tool for instance does that 
> already.
> 
> I could also change i915 to always try to move to CPU0 if it is 
> available, on any online events. But I am not sure that's in the spirit 
> of things.

Ok, I got lost at why i915 perf requires a specific CPU, it is just that
is part of the perf ABI, right?

> I think making IGT perf open wrapper more robust makes most sense.

Yeah, if it's part of the ABI we shouldn't just expect CPU0. Then
throwing a test of the robust perf open in the midst of online/offline
seems sensible.

I guess we will will end up with a perf open before and after each
stage, and keep the pmu fd around until the end.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

end of thread, other threads:[~2018-02-21  9:30 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-20 21:40 [PATCH igt] igt/perf_pmu: Disable all cpus Chris Wilson
2018-02-20 21:40 ` [igt-dev] " Chris Wilson
2018-02-20 22:16 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2018-02-21  7:11 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
2018-02-21  9:11 ` [igt-dev] [PATCH igt] " Tvrtko Ursulin
2018-02-21  9:11   ` [Intel-gfx] " Tvrtko Ursulin
2018-02-21  9:17   ` Chris Wilson
2018-02-21  9:17     ` [Intel-gfx] " Chris Wilson
2018-02-21  9:24     ` Tvrtko Ursulin
2018-02-21  9:24       ` Tvrtko Ursulin
2018-02-21  9:30       ` Chris Wilson
2018-02-21  9:30         ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.