[igt-dev] [PATCH i-g-t 1/6] runner: Use a bigger buffer for receiving test outputs

[igt-dev] [PATCH i-g-t 1/6] runner: Use a bigger buffer for receiving test outputs

[Petri Latvala]

Stream-based receiving was able to use buffers of any size for read(),
but with datagrams we actually need to prepare for actual sizes. In
practice, some file dumps from tests come as single log packets of a
couple of kB.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Arkadiusz Hiler <arek@hiler.eu>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 runner/executor.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index a79a34e0..1fcc9afe 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -857,7 +857,8 @@ static int monitor_output(pid_t child,
 			  char **abortreason)
 {
 	fd_set set;
-	char buf[2048];
+	char *buf;
+	size_t bufsize;
 	char *outbuf = NULL;
 	size_t outbufsize = 0;
 	char current_subtest[256] = {};
@@ -910,6 +911,9 @@ static int monitor_output(pid_t child,
 		}
 	}
 
+	bufsize = 262144; /* 256 kiB */
+	buf = malloc(bufsize);
+
 	while (outfd >= 0 || errfd >= 0 || sigfd >= 0) {
 		const char *timeout_reason;
 		struct timeval tv = { .tv_sec = interval_length };
@@ -942,7 +946,7 @@ static int monitor_output(pid_t child,
 
 			time_last_activity = time_now;
 
-			s = read(outfd, buf, sizeof(buf));
+			s = read(outfd, buf, bufsize);
 			if (s <= 0) {
 				if (s < 0) {
 					errf("Error reading test's stdout: %m\n");
@@ -1037,7 +1041,7 @@ static int monitor_output(pid_t child,
 		if (errfd >= 0 && FD_ISSET(errfd, &set)) {
 			time_last_activity = time_now;
 
-			s = read(errfd, buf, sizeof(buf));
+			s = read(errfd, buf, bufsize);
 			if (s <= 0) {
 				if (s < 0) {
 					errf("Error reading test's stderr: %m\n");
@@ -1060,7 +1064,7 @@ static int monitor_output(pid_t child,
 
 			/* Fully drain everything */
 			while (true) {
-				s = recv(socketfd, buf, sizeof(buf), MSG_DONTWAIT);
+				s = recv(socketfd, buf, bufsize, MSG_DONTWAIT);
 
 				if (s < 0) {
 					if (errno == EAGAIN)
@@ -1394,6 +1398,7 @@ static int monitor_output(pid_t child,
 					fdatasync(outputs[_F_DMESG]);
 
 				close_watchdogs(settings);
+				free(buf);
 				free(outbuf);
 				close(outfd);
 				close(errfd);
@@ -1418,6 +1423,7 @@ static int monitor_output(pid_t child,
 	if (settings->sync)
 		fdatasync(outputs[_F_DMESG]);
 
+	free(buf);
 	free(outbuf);
 	close(outfd);
 	close(errfd);
-- 
2.30.2

[igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms

[Petri Latvala]

Especially with file dumps a single log packet could exceed the max
size of a UNIX datagram. Split too long log chunks instead.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Arkadiusz Hiler <arek@hiler.eu>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_core.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 3941c528..d4bef161 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -457,6 +457,23 @@ static void _igt_log_buffer_reset(void)
 	pthread_mutex_unlock(&log_buffer_mutex);
 }
 
+static void _log_to_runner_split(int stream, const char *str)
+{
+	size_t limit = 4096;
+	char *buf;
+
+	if (strlen(str) > limit) {
+		buf = calloc(limit + 1, 1);
+		strncpy(buf, str, limit);
+		send_to_runner(runnerpacket_log(stream, buf));
+		free(buf);
+
+		_log_to_runner_split(stream, str + limit);
+	} else {
+		send_to_runner(runnerpacket_log(stream, str));
+	}
+}
+
 __attribute__((format(printf, 2, 3)))
 static void _log_line_fprintf(FILE* stream, const char *format, ...)
 {
@@ -467,7 +484,7 @@ static void _log_line_fprintf(FILE* stream, const char *format, ...)
 
 	if (runner_connected()) {
 		vasprintf(&str, format, ap);
-		send_to_runner(runnerpacket_log(fileno(stream), str));
+		_log_to_runner_split(fileno(stream), str);
 		free(str);
 	} else {
 		vfprintf(stream, format, ap);
-- 
2.30.2

[igt-dev] [PATCH i-g-t 3/6] runner: Continue using socket comms when getting an invalid packet

[Petri Latvala]

If a packet of invalid size is received, inject a message in the dump,
override result to warn, and continue grabbing packets.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Arkadiusz Hiler <arek@hiler.eu>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 runner/executor.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index 1fcc9afe..c6389e25 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1079,10 +1079,21 @@ static int monitor_output(pid_t child,
 
 				packet = (struct runnerpacket *)buf;
 				if (s < sizeof(*packet) || s != packet->size) {
+					struct runnerpacket *message, *override;
+
 					errf("Socket communication error: Received %zd bytes, expected %zd\n",
 					     s, s >= sizeof(packet->size) ? packet->size : sizeof(*packet));
-					close(socketfd);
-					socketfd = -1;
+					message = runnerpacket_log(STDOUT_FILENO,
+								   "\nrunner: Socket communication error, invalid packet size. "
+								   "Packet is discarded, test result and logs might be incorrect.\n");
+					write_packet_with_canary(outputs[_F_SOCKET], message, false);
+					free(message);
+
+					override = runnerpacket_resultoverride("warn");
+					write_packet_with_canary(outputs[_F_SOCKET], override, settings->sync);
+					free(override);
+
+					/* Continue using socket comms, hope for the best. */
 					goto socket_end;
 				}
 
-- 
2.30.2

[igt-dev] [PATCH i-g-t 4/6] igt_core: Make sure test result gets to runner when test has no subtests

[Petri Latvala]

One more raw printf, converted to the wrapper that sends to runner if
connected.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Arkadiusz Hiler <arek@hiler.eu>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/igt_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index d4bef161..30b43715 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -2260,8 +2260,8 @@ void igt_exit(void)
 				result = "FAIL";
 		}
 
-		printf("%s (%.3fs)\n",
-		       result, igt_time_elapsed(&subtest_time, &now));
+		_log_line_fprintf(stdout, "%s (%.3fs)\n",
+				  result, igt_time_elapsed(&subtest_time, &now));
 	}
 
 	exit(igt_exitcode);
-- 
2.30.2

[igt-dev] [PATCH i-g-t 5/6] lib/runnercomms: Report empty comms dump as empty

[Petri Latvala]

A mistake that went unnoticed because it's only hit if igt_runner is
killed immediately after initializing the execution but before
executing the test binary.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Arkadiusz Hiler <arek@hiler.eu>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 lib/runnercomms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/runnercomms.c b/lib/runnercomms.c
index 344312bd..034ba0a2 100644
--- a/lib/runnercomms.c
+++ b/lib/runnercomms.c
@@ -515,7 +515,7 @@ int comms_read_dump(int fd, struct comms_visitor *visitor)
 		return COMMSPARSE_ERROR;
 
 	if (statbuf.st_size == 0)
-		return COMMSPARSE_ERROR;
+		return COMMSPARSE_EMPTY;
 
 	buf = mmap(NULL, statbuf.st_size, PROT_READ, MAP_SHARED, fd, 0);
 	if (buf == MAP_FAILED)
-- 
2.30.2

[igt-dev] [PATCH i-g-t 6/6] Revert "runner: Disable socket communications for now"

[Petri Latvala]

With a couple of fixes, using socket comms is now suitably ready for
prime time.

This reverts commit 372c56225e12578a7a4a6bcc5b79eb40b643fcde.

Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Arkadiusz Hiler <arek@hiler.eu>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
---
 runner/executor.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index c6389e25..4881f71a 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1676,14 +1676,7 @@ static int execute_next_entry(struct execute_state *state,
 
 		sigprocmask(SIG_UNBLOCK, sigmask, NULL);
 
-		if (socketfd >= 0 && !getenv("IGT_RUNNER_DISABLE_SOCKET_COMMUNICATION") &&
-		    getenv("IGT_RUNNER_ENABLE_SOCKET_COMMUNICATION_FIXME")) {
-			/*
-			 * The variable for enabling socket comms is
-			 * temporary. Do not use unless you really
-			 * know what you're doing. Requiring it will
-			 * go away later.
-			 */
+		if (socketfd >= 0 && !getenv("IGT_RUNNER_DISABLE_SOCKET_COMMUNICATION")) {
 			snprintf(envstring, sizeof(envstring), "%d", socketfd);
 			setenv("IGT_RUNNER_SOCKET_FD", envstring, 1);
 		}
-- 
2.30.2

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/6] runner: Use a bigger buffer for receiving test outputs

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12953 bytes --]

== Series Details ==

Series: series starting with [i-g-t,1/6] runner: Use a bigger buffer for receiving test outputs
URL   : https://patchwork.freedesktop.org/series/110597/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12349 -> IGTPW_8053
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8053 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8053, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/index.html

Participating hosts (41 -> 40)
------------------------------

  Additional (3): fi-kbl-soraka fi-cml-u2 fi-kbl-x1275 
  Missing    (4): fi-ctg-p8600 fi-bdw-samus fi-tgl-dsi fi-pnv-d510 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_8053:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@slpc:
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-soraka/igt@i915_selftest@live@slpc.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@mman:
    - {bat-rpls-1}:       [PASS][2] -> [TIMEOUT][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-rpls-1/igt@i915_selftest@live@mman.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-rpls-1/igt@i915_selftest@live@mman.html

  
Known issues
------------

  Here are the changes found in IGTPW_8053 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fence@basic-busy@bcs0:
    - fi-cml-u2:          NOTRUN -> [SKIP][4] ([i915#1208]) +1 similar issue
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@gem_exec_fence@basic-busy@bcs0.html

  * igt@gem_exec_gttfill@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][5] ([fdo#109271]) +8 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-soraka/igt@gem_exec_gttfill@basic.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#2190])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html
    - fi-cml-u2:          NOTRUN -> [SKIP][7] ([i915#2190])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@gem_huc_copy@huc-copy.html
    - fi-kbl-x1275:       NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#2190])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-x1275/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][9] ([fdo#109271] / [i915#4613]) +3 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-adlp-4:         NOTRUN -> [SKIP][10] ([i915#4613]) +3 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@gem_lmem_swapping@parallel-random-engines.html
    - fi-cml-u2:          NOTRUN -> [SKIP][11] ([i915#4613]) +3 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@verify-random:
    - fi-kbl-x1275:       NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#4613]) +3 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-x1275/igt@gem_lmem_swapping@verify-random.html

  * igt@i915_pm_rps@basic-api:
    - bat-adlp-4:         NOTRUN -> [SKIP][13] ([i915#6621])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][14] ([i915#1886])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-bsw-kefka:       NOTRUN -> [SKIP][15] ([fdo#109271] / [fdo#111827])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-bsw-kefka/igt@kms_chamelium@common-hpd-after-suspend.html
    - bat-adlp-4:         NOTRUN -> [SKIP][16] ([fdo#111827])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-cml-u2:          NOTRUN -> [SKIP][17] ([fdo#109284] / [fdo#111827]) +8 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_chamelium@dp-hpd-fast:
    - fi-kbl-x1275:       NOTRUN -> [SKIP][18] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-x1275/igt@kms_chamelium@dp-hpd-fast.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][19] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-soraka/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - fi-cml-u2:          NOTRUN -> [SKIP][20] ([i915#4213])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       [PASS][21] -> [FAIL][22] ([i915#6298])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-cml-u2:          NOTRUN -> [SKIP][23] ([fdo#109285])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-cml-u2:          NOTRUN -> [DMESG-WARN][24] ([i915#402])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-adlp-4:         NOTRUN -> [SKIP][25] ([i915#3546])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@kms_pipe_crc_basic@suspend-read-crc.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-cml-u2:          NOTRUN -> [SKIP][26] ([i915#3555])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-userptr:
    - fi-kbl-x1275:       NOTRUN -> [SKIP][27] ([fdo#109271]) +11 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-kbl-x1275/igt@prime_vgem@basic-userptr.html
    - bat-adlp-4:         NOTRUN -> [SKIP][28] ([fdo#109295] / [i915#3301] / [i915#3708])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@prime_vgem@basic-userptr.html
    - fi-cml-u2:          NOTRUN -> [SKIP][29] ([fdo#109295] / [i915#3301])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-cml-u2/igt@prime_vgem@basic-userptr.html

  * igt@prime_vgem@basic-write:
    - bat-adlp-4:         NOTRUN -> [SKIP][30] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@fbdev@read:
    - {bat-rpls-2}:       [SKIP][31] ([i915#2582]) -> [PASS][32] +4 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-rpls-2/igt@fbdev@read.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-rpls-2/igt@fbdev@read.html

  * igt@gem_exec_suspend@basic-s0@lmem0:
    - {bat-dg2-11}:       [DMESG-WARN][33] ([i915#6816]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-dg2-11/igt@gem_exec_suspend@basic-s0@lmem0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-dg2-11/igt@gem_exec_suspend@basic-s0@lmem0.html

  * igt@gem_huc_copy@huc-copy:
    - {bat-dg2-11}:       [FAIL][35] ([i915#7029]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-dg2-11/igt@gem_huc_copy@huc-copy.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-dg2-11/igt@gem_huc_copy@huc-copy.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - bat-adlp-4:         [DMESG-WARN][37] ([i915#7077]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-adlp-4/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-bxt-dsi:         [DMESG-FAIL][39] ([i915#5334] / [i915#7433]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hangcheck:
    - {bat-rplp-1}:       [INCOMPLETE][41] -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-rplp-1/igt@i915_selftest@live@hangcheck.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-rplp-1/igt@i915_selftest@live@hangcheck.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-dp-3:
    - {bat-dg2-11}:       [FAIL][43] ([i915#6818]) -> [PASS][44] +3 similar issues
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12349/bat-dg2-11/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-dp-3.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/bat-dg2-11/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-dp-3.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1208]: https://gitlab.freedesktop.org/drm/intel/issues/1208
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5153]: https://gitlab.freedesktop.org/drm/intel/issues/5153
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6816]: https://gitlab.freedesktop.org/drm/intel/issues/6816
  [i915#6818]: https://gitlab.freedesktop.org/drm/intel/issues/6818
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7029]: https://gitlab.freedesktop.org/drm/intel/issues/7029
  [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
  [i915#7433]: https://gitlab.freedesktop.org/drm/intel/issues/7433
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7046 -> IGTPW_8053

  CI-20190529: 20190529
  CI_DRM_12349: 04ebc380927464e5630f267402846c5a8c0b3b9b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8053: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/index.html
  IGT_7046: c58d96d0fe237474b074e3472ce09c57c830d5de @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8053/index.html

[-- Attachment #2: Type: text/html, Size: 15391 bytes --]

Re: [igt-dev] [PATCH i-g-t 4/6] igt_core: Make sure test result gets to runner when test has no subtests

[Kamil Konieczny]

On 2022-11-07 at 14:01:49 +0200, Petri Latvala wrote:
> One more raw printf, converted to the wrapper that sends to runner if
> connected.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arek@hiler.eu>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  lib/igt_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index d4bef161..30b43715 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -2260,8 +2260,8 @@ void igt_exit(void)
>  				result = "FAIL";
>  		}
>  
> -		printf("%s (%.3fs)\n",
> -		       result, igt_time_elapsed(&subtest_time, &now));
> +		_log_line_fprintf(stdout, "%s (%.3fs)\n",
> +				  result, igt_time_elapsed(&subtest_time, &now));
>  	}
>  
>  	exit(igt_exitcode);
> -- 
> 2.30.2
> 

Re: [igt-dev] [PATCH i-g-t 5/6] lib/runnercomms: Report empty comms dump as empty

[Kamil Konieczny]

On 2022-11-07 at 14:01:50 +0200, Petri Latvala wrote:
> A mistake that went unnoticed because it's only hit if igt_runner is
> killed immediately after initializing the execution but before
> executing the test binary.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arek@hiler.eu>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  lib/runnercomms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/runnercomms.c b/lib/runnercomms.c
> index 344312bd..034ba0a2 100644
> --- a/lib/runnercomms.c
> +++ b/lib/runnercomms.c
> @@ -515,7 +515,7 @@ int comms_read_dump(int fd, struct comms_visitor *visitor)
>  		return COMMSPARSE_ERROR;
>  
>  	if (statbuf.st_size == 0)
> -		return COMMSPARSE_ERROR;
> +		return COMMSPARSE_EMPTY;
>  
>  	buf = mmap(NULL, statbuf.st_size, PROT_READ, MAP_SHARED, fd, 0);
>  	if (buf == MAP_FAILED)
> -- 
> 2.30.2
> 

Re: [igt-dev] [PATCH i-g-t 6/6] Revert "runner: Disable socket communications for now"

[Kamil Konieczny]

On 2022-11-07 at 14:01:51 +0200, Petri Latvala wrote:
> With a couple of fixes, using socket comms is now suitably ready for
> prime time.
> 
> This reverts commit 372c56225e12578a7a4a6bcc5b79eb40b643fcde.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arek@hiler.eu>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  runner/executor.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index c6389e25..4881f71a 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1676,14 +1676,7 @@ static int execute_next_entry(struct execute_state *state,
>  
>  		sigprocmask(SIG_UNBLOCK, sigmask, NULL);
>  
> -		if (socketfd >= 0 && !getenv("IGT_RUNNER_DISABLE_SOCKET_COMMUNICATION") &&
> -		    getenv("IGT_RUNNER_ENABLE_SOCKET_COMMUNICATION_FIXME")) {
> -			/*
> -			 * The variable for enabling socket comms is
> -			 * temporary. Do not use unless you really
> -			 * know what you're doing. Requiring it will
> -			 * go away later.
> -			 */
> +		if (socketfd >= 0 && !getenv("IGT_RUNNER_DISABLE_SOCKET_COMMUNICATION")) {
>  			snprintf(envstring, sizeof(envstring), "%d", socketfd);
>  			setenv("IGT_RUNNER_SOCKET_FD", envstring, 1);
>  		}
> -- 
> 2.30.2
> 

Re: [igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms

[Kamil Konieczny]

Hi Petri,

On 2022-11-07 at 14:01:47 +0200, Petri Latvala wrote:
> Especially with file dumps a single log packet could exceed the max
> size of a UNIX datagram. Split too long log chunks instead.

imho this is needed when you check packet size in other patch,
it's network layer decision to split or not packets, so on local
machine no split happens (well, I may be wrong here as I do not
know much about network communication). The other way around
would be to collect packet up to sended size.

> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arek@hiler.eu>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  lib/igt_core.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 3941c528..d4bef161 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -457,6 +457,23 @@ static void _igt_log_buffer_reset(void)
>  	pthread_mutex_unlock(&log_buffer_mutex);
>  }
>  
> +static void _log_to_runner_split(int stream, const char *str)
> +{
> +	size_t limit = 4096;
> +	char *buf;
> +
> +	if (strlen(str) > limit) {
> +		buf = calloc(limit + 1, 1);
> +		strncpy(buf, str, limit);
> +		send_to_runner(runnerpacket_log(stream, buf));
> +		free(buf);
> +
> +		_log_to_runner_split(stream, str + limit);
> +	} else {
> +		send_to_runner(runnerpacket_log(stream, str));
> +	}
> +}
> +

There are many places which calls send_to_runner, maybe it would
be better to split it there ?
Or use _log_to_runner instead of send_to_runner ?
Btw a while loop would be better than recursive call.

Kamil

>  __attribute__((format(printf, 2, 3)))
>  static void _log_line_fprintf(FILE* stream, const char *format, ...)
>  {
> @@ -467,7 +484,7 @@ static void _log_line_fprintf(FILE* stream, const char *format, ...)
>  
>  	if (runner_connected()) {
>  		vasprintf(&str, format, ap);
> -		send_to_runner(runnerpacket_log(fileno(stream), str));
> +		_log_to_runner_split(fileno(stream), str);
>  		free(str);
>  	} else {
>  		vfprintf(stream, format, ap);
> -- 
> 2.30.2
> 

Re: [igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms

[Petri Latvala]

On Mon, Nov 07, 2022 at 03:16:18PM +0100, Kamil Konieczny wrote:
> Hi Petri,
> 
> On 2022-11-07 at 14:01:47 +0200, Petri Latvala wrote:
> > Especially with file dumps a single log packet could exceed the max
> > size of a UNIX datagram. Split too long log chunks instead.
> 
> imho this is needed when you check packet size in other patch,
> it's network layer decision to split or not packets, so on local
> machine no split happens (well, I may be wrong here as I do not
> know much about network communication). The other way around
> would be to collect packet up to sended size.

With SOCK_STREAM, sure, but runnercomms is using SOCK_DGRAM. Datagram
packets are full-or-nothing atomically.


> 
> > 
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > Cc: Arkadiusz Hiler <arek@hiler.eu>
> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > ---
> >  lib/igt_core.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 3941c528..d4bef161 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -457,6 +457,23 @@ static void _igt_log_buffer_reset(void)
> >  	pthread_mutex_unlock(&log_buffer_mutex);
> >  }
> >  
> > +static void _log_to_runner_split(int stream, const char *str)
> > +{
> > +	size_t limit = 4096;
> > +	char *buf;
> > +
> > +	if (strlen(str) > limit) {
> > +		buf = calloc(limit + 1, 1);
> > +		strncpy(buf, str, limit);
> > +		send_to_runner(runnerpacket_log(stream, buf));
> > +		free(buf);
> > +
> > +		_log_to_runner_split(stream, str + limit);
> > +	} else {
> > +		send_to_runner(runnerpacket_log(stream, str));
> > +	}
> > +}
> > +
> 
> There are many places which calls send_to_runner, maybe it would
> be better to split it there ?

That would complicate send_to_runner a bit too much to my
taste. send_to_runner() is for all packet types, not just for log
packets. It would have to check for packet type log, extract the data
from that packet, and then create new ones...

> Or use _log_to_runner instead of send_to_runner ?
> Btw a while loop would be better than recursive call.

Yeah, you're right. I'll change that to a while loop.


-- 
Petri Latvala


> 
> Kamil
> 
> >  __attribute__((format(printf, 2, 3)))
> >  static void _log_line_fprintf(FILE* stream, const char *format, ...)
> >  {
> > @@ -467,7 +484,7 @@ static void _log_line_fprintf(FILE* stream, const char *format, ...)
> >  
> >  	if (runner_connected()) {
> >  		vasprintf(&str, format, ap);
> > -		send_to_runner(runnerpacket_log(fileno(stream), str));
> > +		_log_to_runner_split(fileno(stream), str);
> >  		free(str);
> >  	} else {
> >  		vfprintf(stream, format, ap);
> > -- 
> > 2.30.2
> > 

Re: [igt-dev] [PATCH i-g-t 3/6] runner: Continue using socket comms when getting an invalid packet

[Kamil Konieczny]

On 2022-11-07 at 14:01:48 +0200, Petri Latvala wrote:
> If a packet of invalid size is received, inject a message in the dump,
> override result to warn, and continue grabbing packets.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arek@hiler.eu>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  runner/executor.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index 1fcc9afe..c6389e25 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1079,10 +1079,21 @@ static int monitor_output(pid_t child,
>  
>  				packet = (struct runnerpacket *)buf;
>  				if (s < sizeof(*packet) || s != packet->size) {
> +					struct runnerpacket *message, *override;
> +
>  					errf("Socket communication error: Received %zd bytes, expected %zd\n",
>  					     s, s >= sizeof(packet->size) ? packet->size : sizeof(*packet));
> -					close(socketfd);
> -					socketfd = -1;
> +					message = runnerpacket_log(STDOUT_FILENO,
> +								   "\nrunner: Socket communication error, invalid packet size. "
> +								   "Packet is discarded, test result and logs might be incorrect.\n");
> +					write_packet_with_canary(outputs[_F_SOCKET], message, false);
> +					free(message);
> +
> +					override = runnerpacket_resultoverride("warn");
> +					write_packet_with_canary(outputs[_F_SOCKET], override, settings->sync);
> +					free(override);
> +
> +					/* Continue using socket comms, hope for the best. */
>  					goto socket_end;
>  				}
>  
> -- 
> 2.30.2
> 

Re: [igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms

[Kamil Konieczny]

Hi Petri,

On 2022-11-07 at 17:14:57 +0200, Petri Latvala wrote:
> On Mon, Nov 07, 2022 at 03:16:18PM +0100, Kamil Konieczny wrote:
> > Hi Petri,
> > 
> > On 2022-11-07 at 14:01:47 +0200, Petri Latvala wrote:
> > > Especially with file dumps a single log packet could exceed the max
> > > size of a UNIX datagram. Split too long log chunks instead.
> > 
> > imho this is needed when you check packet size in other patch,
> > it's network layer decision to split or not packets, so on local
> > machine no split happens (well, I may be wrong here as I do not
> > know much about network communication). The other way around
> > would be to collect packet up to sended size.
> 
> With SOCK_STREAM, sure, but runnercomms is using SOCK_DGRAM. Datagram
> packets are full-or-nothing atomically.
> 
> 

Ah, ok, but then you risk getting your datagrams in out-of-order
and when you will fix this you reimplement stream. If it is to
be used on local machine maybe it should not happen.

Kamil

> > 
> > > 
> > > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > > Cc: Arkadiusz Hiler <arek@hiler.eu>
> > > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > ---
> > >  lib/igt_core.c | 19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index 3941c528..d4bef161 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -457,6 +457,23 @@ static void _igt_log_buffer_reset(void)
> > >  	pthread_mutex_unlock(&log_buffer_mutex);
> > >  }
> > >  
> > > +static void _log_to_runner_split(int stream, const char *str)
> > > +{
> > > +	size_t limit = 4096;
> > > +	char *buf;
> > > +
> > > +	if (strlen(str) > limit) {
> > > +		buf = calloc(limit + 1, 1);
> > > +		strncpy(buf, str, limit);
> > > +		send_to_runner(runnerpacket_log(stream, buf));
> > > +		free(buf);
> > > +
> > > +		_log_to_runner_split(stream, str + limit);
> > > +	} else {
> > > +		send_to_runner(runnerpacket_log(stream, str));
> > > +	}
> > > +}
> > > +
> > 
> > There are many places which calls send_to_runner, maybe it would
> > be better to split it there ?
> 
> That would complicate send_to_runner a bit too much to my
> taste. send_to_runner() is for all packet types, not just for log
> packets. It would have to check for packet type log, extract the data
> from that packet, and then create new ones...
> 
> > Or use _log_to_runner instead of send_to_runner ?
> > Btw a while loop would be better than recursive call.
> 
> Yeah, you're right. I'll change that to a while loop.
> 
> 
> -- 
> Petri Latvala
> 
> 
> > 
> > Kamil
> > 
> > >  __attribute__((format(printf, 2, 3)))
> > >  static void _log_line_fprintf(FILE* stream, const char *format, ...)
> > >  {
> > > @@ -467,7 +484,7 @@ static void _log_line_fprintf(FILE* stream, const char *format, ...)
> > >  
> > >  	if (runner_connected()) {
> > >  		vasprintf(&str, format, ap);
> > > -		send_to_runner(runnerpacket_log(fileno(stream), str));
> > > +		_log_to_runner_split(fileno(stream), str);
> > >  		free(str);
> > >  	} else {
> > >  		vfprintf(stream, format, ap);
> > > -- 
> > > 2.30.2
> > > 

Re: [igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms

[Petri Latvala]

On Mon, Nov 07, 2022 at 05:40:06PM +0100, Kamil Konieczny wrote:
> Hi Petri,
> 
> On 2022-11-07 at 17:14:57 +0200, Petri Latvala wrote:
> > On Mon, Nov 07, 2022 at 03:16:18PM +0100, Kamil Konieczny wrote:
> > > Hi Petri,
> > > 
> > > On 2022-11-07 at 14:01:47 +0200, Petri Latvala wrote:
> > > > Especially with file dumps a single log packet could exceed the max
> > > > size of a UNIX datagram. Split too long log chunks instead.
> > > 
> > > imho this is needed when you check packet size in other patch,
> > > it's network layer decision to split or not packets, so on local
> > > machine no split happens (well, I may be wrong here as I do not
> > > know much about network communication). The other way around
> > > would be to collect packet up to sended size.
> > 
> > With SOCK_STREAM, sure, but runnercomms is using SOCK_DGRAM. Datagram
> > packets are full-or-nothing atomically.
> > 
> > 
> 
> Ah, ok, but then you risk getting your datagrams in out-of-order
> and when you will fix this you reimplement stream. If it is to
> be used on local machine maybe it should not happen.

AF_UNIX + SOCK_DGRAM guarantees same-order delivery.

-- 
Petri Latvala


> 
> Kamil
> 
> > > 
> > > > 
> > > > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > > > Cc: Arkadiusz Hiler <arek@hiler.eu>
> > > > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > > ---
> > > >  lib/igt_core.c | 19 ++++++++++++++++++-
> > > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > index 3941c528..d4bef161 100644
> > > > --- a/lib/igt_core.c
> > > > +++ b/lib/igt_core.c
> > > > @@ -457,6 +457,23 @@ static void _igt_log_buffer_reset(void)
> > > >  	pthread_mutex_unlock(&log_buffer_mutex);
> > > >  }
> > > >  
> > > > +static void _log_to_runner_split(int stream, const char *str)
> > > > +{
> > > > +	size_t limit = 4096;
> > > > +	char *buf;
> > > > +
> > > > +	if (strlen(str) > limit) {
> > > > +		buf = calloc(limit + 1, 1);
> > > > +		strncpy(buf, str, limit);
> > > > +		send_to_runner(runnerpacket_log(stream, buf));
> > > > +		free(buf);
> > > > +
> > > > +		_log_to_runner_split(stream, str + limit);
> > > > +	} else {
> > > > +		send_to_runner(runnerpacket_log(stream, str));
> > > > +	}
> > > > +}
> > > > +
> > > 
> > > There are many places which calls send_to_runner, maybe it would
> > > be better to split it there ?
> > 
> > That would complicate send_to_runner a bit too much to my
> > taste. send_to_runner() is for all packet types, not just for log
> > packets. It would have to check for packet type log, extract the data
> > from that packet, and then create new ones...
> > 
> > > Or use _log_to_runner instead of send_to_runner ?
> > > Btw a while loop would be better than recursive call.
> > 
> > Yeah, you're right. I'll change that to a while loop.
> > 
> > 
> > -- 
> > Petri Latvala
> > 
> > 
> > > 
> > > Kamil
> > > 
> > > >  __attribute__((format(printf, 2, 3)))
> > > >  static void _log_line_fprintf(FILE* stream, const char *format, ...)
> > > >  {
> > > > @@ -467,7 +484,7 @@ static void _log_line_fprintf(FILE* stream, const char *format, ...)
> > > >  
> > > >  	if (runner_connected()) {
> > > >  		vasprintf(&str, format, ap);
> > > > -		send_to_runner(runnerpacket_log(fileno(stream), str));
> > > > +		_log_to_runner_split(fileno(stream), str);
> > > >  		free(str);
> > > >  	} else {
> > > >  		vfprintf(stream, format, ap);
> > > > -- 
> > > > 2.30.2
> > > > 

Re: [igt-dev] [PATCH i-g-t 1/6] runner: Use a bigger buffer for receiving test outputs

[Kamil Konieczny]

Hi Petri,

On 2022-11-07 at 14:01:46 +0200, Petri Latvala wrote:
> Stream-based receiving was able to use buffers of any size for read(),
> but with datagrams we actually need to prepare for actual sizes. In
> practice, some file dumps from tests come as single log packets of a
> couple of kB.
> 
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arek@hiler.eu>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  runner/executor.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index a79a34e0..1fcc9afe 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -857,7 +857,8 @@ static int monitor_output(pid_t child,
>  			  char **abortreason)
>  {
>  	fd_set set;
> -	char buf[2048];
> +	char *buf;
> +	size_t bufsize;
>  	char *outbuf = NULL;
>  	size_t outbufsize = 0;
>  	char current_subtest[256] = {};
> @@ -910,6 +911,9 @@ static int monitor_output(pid_t child,
>  		}
>  	}
>  
> +	bufsize = 262144; /* 256 kiB */

imho it is better to use define and also write this as 256*1024
and put it in header.

If you increase this size maybe you can drop splitting ?

Kamil

> +	buf = malloc(bufsize);
> +
>  	while (outfd >= 0 || errfd >= 0 || sigfd >= 0) {
>  		const char *timeout_reason;
>  		struct timeval tv = { .tv_sec = interval_length };
> @@ -942,7 +946,7 @@ static int monitor_output(pid_t child,
>  
>  			time_last_activity = time_now;
>  
> -			s = read(outfd, buf, sizeof(buf));
> +			s = read(outfd, buf, bufsize);
>  			if (s <= 0) {
>  				if (s < 0) {
>  					errf("Error reading test's stdout: %m\n");
> @@ -1037,7 +1041,7 @@ static int monitor_output(pid_t child,
>  		if (errfd >= 0 && FD_ISSET(errfd, &set)) {
>  			time_last_activity = time_now;
>  
> -			s = read(errfd, buf, sizeof(buf));
> +			s = read(errfd, buf, bufsize);
>  			if (s <= 0) {
>  				if (s < 0) {
>  					errf("Error reading test's stderr: %m\n");
> @@ -1060,7 +1064,7 @@ static int monitor_output(pid_t child,
>  
>  			/* Fully drain everything */
>  			while (true) {
> -				s = recv(socketfd, buf, sizeof(buf), MSG_DONTWAIT);
> +				s = recv(socketfd, buf, bufsize, MSG_DONTWAIT);
>  
>  				if (s < 0) {
>  					if (errno == EAGAIN)
> @@ -1394,6 +1398,7 @@ static int monitor_output(pid_t child,
>  					fdatasync(outputs[_F_DMESG]);
>  
>  				close_watchdogs(settings);
> +				free(buf);
>  				free(outbuf);
>  				close(outfd);
>  				close(errfd);
> @@ -1418,6 +1423,7 @@ static int monitor_output(pid_t child,
>  	if (settings->sync)
>  		fdatasync(outputs[_F_DMESG]);
>  
> +	free(buf);
>  	free(outbuf);
>  	close(outfd);
>  	close(errfd);
> -- 
> 2.30.2
> 

Re: [igt-dev] [PATCH i-g-t 1/6] runner: Use a bigger buffer for receiving test outputs

[Petri Latvala]

On Mon, Nov 07, 2022 at 05:45:26PM +0100, Kamil Konieczny wrote:
> Hi Petri,
> 
> On 2022-11-07 at 14:01:46 +0200, Petri Latvala wrote:
> > Stream-based receiving was able to use buffers of any size for read(),
> > but with datagrams we actually need to prepare for actual sizes. In
> > practice, some file dumps from tests come as single log packets of a
> > couple of kB.
> > 
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > Cc: Arkadiusz Hiler <arek@hiler.eu>
> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > ---
> >  runner/executor.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/runner/executor.c b/runner/executor.c
> > index a79a34e0..1fcc9afe 100644
> > --- a/runner/executor.c
> > +++ b/runner/executor.c
> > @@ -857,7 +857,8 @@ static int monitor_output(pid_t child,
> >  			  char **abortreason)
> >  {
> >  	fd_set set;
> > -	char buf[2048];
> > +	char *buf;
> > +	size_t bufsize;
> >  	char *outbuf = NULL;
> >  	size_t outbufsize = 0;
> >  	char current_subtest[256] = {};
> > @@ -910,6 +911,9 @@ static int monitor_output(pid_t child,
> >  		}
> >  	}
> >  
> > +	bufsize = 262144; /* 256 kiB */
> 
> imho it is better to use define and also write this as 256*1024
> and put it in header.

Sure.

> If you increase this size maybe you can drop splitting ?

Splitting is only done for log packets. Currently there's no obvious
way for other packet types to be larger than reasonable but for
futureproofing it's good to be defensive in both ends.

Having e.g. assertion failure reason fields (when that is stuffed into
the subtest result packet) go multiple kilobytes sounds wild but who
knows what kind of autogenerated igt_assert macros people will attempt
to write...


-- 
Petri Latvala