[igt-dev] [PATCH v5 0/2] Add a new test for driver/device hot reload

[igt-dev] [PATCH v5 0/2] Add a new test for driver/device hot reload

[Janusz Krzysztofik]

Janusz Krzysztofik (2):
  tests: Add a new test for driver/device hot reload
  tests/core_hot_reload: Accept external workload

 tests/Makefile.sources  |   1 +
 tests/core_hot_reload.c | 303 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build       |   1 +
 3 files changed, 305 insertions(+)
 create mode 100644 tests/core_hot_reload.c


Changelog:
v4->v5:
- move workload back from subtests to initial fixture and run it from
  an igt helper subprocess so workload crash does not affect test
  results and resources consumed by the workload are cleaned up
  automatically, also without affecting test results,
- re-add the second patch which extends the test with an option for
  using an external command as a workload.

v3->v4 (first public submission):
- run dummy_workolad from inside subtests,
- try to restore the device to a working state after each subtest.

v2->v3:
- run dummy_workload from the test process directly,
- drop the patch for running external workload.

v1->v2:
- run a subprocess with dummy_workload instead of external command,
- keep use of external workload command as an option, move that to a
  separate patch.
-- 
2.20.1

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

[igt-dev] [PATCH v5 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>

Run a dummy load in background to put some workload on a device, then try
to either remove (unplug) the device from its bus, or unbind the device's
driver from it, depending on which subtest has been selected.  If
succeeded, unload the driver, rescan the device's bus if needed and
perform health checks on the device with the driver reloaded.

The dummy load is run from igt_fixture and in a sub-process, not directly
from subtests,  as it is expected to fail and it's more simple to ignore
igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
and exits, resources it was using are freed automatically so there is no
need to do any cleanups required for smooth module unload from the test
level itself.  Those cleanups might also make the subtests fail if simply
using igt library functions for that instead of reimplementing their safe
parts only.

The driver hot unbind / device hot unplug operation is expected to succeed
and the background workload sub-process to die in a reasonable time,
however long timeouts are used to let kernel level timeouts pop up first
if hit by a bug.

The dummy load works only on i915 driver.  The test is skipped on other
hardware unless they provide their implementation of igt_spin_batch_new()
and friends.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
---
 tests/Makefile.sources  |   1 +
 tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build       |   1 +
 3 files changed, 258 insertions(+)
 create mode 100644 tests/core_hot_reload.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 214698da..d2c0941d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -16,6 +16,7 @@ TESTS_progs = \
 	core_getclient \
 	core_getstats \
 	core_getversion \
+	core_hot_reload \
 	core_setmaster_vs_auth \
 	debugfs_test \
 	drm_import_export \
diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
new file mode 100644
index 00000000..d862c99c
--- /dev/null
+++ b/tests/core_hot_reload.c
@@ -0,0 +1,256 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+#include "igt_device.h"
+#include "igt_dummyload.h"
+#include "igt_kmod.h"
+#include "igt_sysfs.h"
+
+#include <getopt.h>
+#include <limits.h>
+#include <string.h>
+#include <unistd.h>
+
+
+typedef int (*action_t)(int dir);
+typedef void (*workload_wait_t)(void *priv);
+typedef void (*workload_t)(int device, const void *priv);
+
+
+/*
+ * Actions
+ *
+ * Purpose: make the device disappear
+ *
+ * @dir: file descriptor of an open device sysfs directory
+ *
+ * Return value: file descriptor of an open device bus' sysfs directory
+ * 		 or -1 if no bus rescan is needed
+ */
+
+/* Unbind the driver from the device */
+static int driver_unbind(int dir)
+{
+	char path[PATH_MAX], *dev_bus_addr;
+	int len;
+
+	len = readlinkat(dir, "device", path, sizeof(path) - 1);
+	path[len] = '\0';
+	dev_bus_addr = strrchr(path, '/') + 1;
+
+	igt_set_timeout(60, "Driver unbind timeout!");
+	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
+
+	/* No need for bus rescan */
+	return -1;
+}
+
+/* Remove (virtually unplug) the device from its bus */
+static int device_unplug(int dir)
+{
+	int bus;
+
+	bus = openat(dir, "device/subsystem", O_DIRECTORY);
+	igt_assert(bus >= 0);
+
+	igt_set_timeout(60, "Device unplug timeout!");
+	igt_sysfs_set(dir, "device/remove", "1");
+
+	return bus;
+}
+
+
+/*
+ * Workloads
+ *
+ * Purpose: Put some long lasting load on the device
+ *
+ * @device: open device file descriptor,
+ * @priv: pointer to an optional argument passed to the workload
+ *
+ * Return value: none
+ */
+
+/* Workload using igt_spin_batch_run() */
+
+static void spin_batch(int device, const void *priv)
+{
+	igt_spin_t *spin;
+
+	/* submit the job */
+	spin = igt_spin_batch_new(device);
+
+	/* wait for the job to crash */
+	gem_sync(device, spin->handle);
+
+	/* clean up if still possible */
+	igt_spin_batch_free(device, spin);
+}
+
+
+/*
+ * Skeleton
+ */
+
+static void healthcheck(int chipset)
+{
+	if (chipset == DRIVER_INTEL) {
+		/*
+		 * We have it perfectly implemented in i915_module_load,
+		 * just use it.
+		 */
+		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
+			   == IGT_EXIT_SUCCESS);
+	} else {
+		/*
+		 * We don't know how to check an unidentified device for health,
+		 * device reopen must suffice.
+		 */
+	}
+}
+
+static void driver_unload(int chipset, char *driver)
+{
+	if (chipset == DRIVER_INTEL)
+		igt_assert(igt_i915_driver_unload() == IGT_EXIT_SUCCESS);
+	else
+		igt_assert(igt_kmod_unload(driver, 0) == 0);
+}
+
+static int operation(int device, action_t action, workload_wait_t workload_wait,
+		     void *workload_priv)
+{
+	int dir, bus;
+
+	dir = igt_sysfs_open(device);
+
+	bus = action(dir);
+	close(dir);
+
+	if (workload_wait)
+		workload_wait(workload_priv);
+	igt_reset_timeout();
+
+	return bus;
+}
+
+
+static void __subtest(int device, int chipset, char *driver, action_t action,
+		      workload_wait_t workload_wait, void *workload_priv)
+{
+	int bus = operation(device, action, workload_wait, workload_priv);
+
+	close(device);
+	driver_unload(chipset, driver);
+
+	/* Valid file descriptor indicates we should rescan the bus */
+	if (bus >= 0) {
+		igt_sysfs_set(bus, "rescan", "1");
+		close(bus);
+	}
+}
+
+static void run_subtest(int *device, int chipset, char *driver, action_t action,
+			workload_wait_t workload_wait, void *workload_priv)
+{
+	__subtest(*device, chipset, driver, action, workload_wait, workload_priv);
+
+	healthcheck(chipset);
+
+	*device = __drm_open_driver(chipset);
+	igt_assert(*device >= 0);
+}
+
+static void __workload(workload_t workload, int device, const void *priv,
+		      struct igt_helper_process *proc)
+{
+	igt_fork_helper(proc)
+		workload(device, priv);
+	/* let the background process start doing its job */
+	sleep(2);
+}
+
+static void __workload_wait(void *priv)
+{
+	struct igt_helper_process *proc = priv;
+
+	/* wait until the workload has crashed */
+	igt_wait_helper(proc);
+}
+
+
+igt_main {
+	int device, chipset;
+	char *driver;
+	struct igt_helper_process proc = {};
+	workload_wait_t workload_wait;
+	void *workload_priv;
+
+	igt_fixture {
+		char path[PATH_MAX];
+		int dir, len;
+
+		/*
+		 * Since the test depends on successful unload of driver module,
+		 * don't use drm_open_driver() as it keeps a file descriptor
+		 * open for exit handler use that effectively locks the module.
+		 */
+		device = __drm_open_driver(DRIVER_ANY);
+		igt_assert(device >= 0);
+
+		if (is_i915_device(device)) {
+			chipset = DRIVER_INTEL;
+			driver = strdup("i915");
+		} else {
+			chipset = DRIVER_ANY;
+
+			/* Capture module name to be unloaded */
+			dir = igt_sysfs_open(device);
+			len = readlinkat(dir, "device/driver/module", path,
+					 sizeof(path) - 1);
+			close(dir);
+			path[len] = '\0';
+			driver = strdup(strrchr(path, '/') + 1);
+		}
+		igt_info("Running the test on driver \"%s\", chipset mask %#0x\n",
+			 driver, chipset);
+
+		workload_wait = __workload_wait;
+		workload_priv = &proc;
+		__workload(spin_batch, device, NULL, &proc);
+	}
+
+	igt_subtest("unplug")
+		run_subtest(&device, chipset, driver, device_unplug,
+			    workload_wait, workload_priv);
+
+	igt_subtest("unbind")
+		run_subtest(&device, chipset, driver, driver_unbind,
+			    workload_wait, workload_priv);
+
+	igt_fixture {
+		free(driver);
+		close(device);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 5167a6cc..1b91e5a2 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -3,6 +3,7 @@ test_progs = [
 	'core_getclient',
 	'core_getstats',
 	'core_getversion',
+	'core_hot_reload',
 	'core_setmaster_vs_auth',
 	'debugfs_test',
 	'drm_import_export',
-- 
2.20.1

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

Re: [igt-dev] [PATCH v5 1/2] tests: Add a new test for driver/device hot reload

[Katarzyna Dec]

On Tue, Apr 09, 2019 at 01:10:58PM +0200, Janusz Krzysztofik wrote:
> From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> 
> Run a dummy load in background to put some workload on a device, then try
> to either remove (unplug) the device from its bus, or unbind the device's
> driver from it, depending on which subtest has been selected.  If
> succeeded, unload the driver, rescan the device's bus if needed and
> perform health checks on the device with the driver reloaded.
> 
> The dummy load is run from igt_fixture and in a sub-process, not directly
> from subtests,  as it is expected to fail and it's more simple to ignore
> igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
> and exits, resources it was using are freed automatically so there is no
> need to do any cleanups required for smooth module unload from the test
> level itself.  Those cleanups might also make the subtests fail if simply
> using igt library functions for that instead of reimplementing their safe
> parts only.
> 
> The driver hot unbind / device hot unplug operation is expected to succeed
> and the background workload sub-process to die in a reasonable time,
> however long timeouts are used to let kernel level timeouts pop up first
> if hit by a bug.
> 
> The dummy load works only on i915 driver.  The test is skipped on other
> hardware unless they provide their implementation of igt_spin_batch_new()
> and friends.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> ---
>  tests/Makefile.sources  |   1 +
>  tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build       |   1 +
>  3 files changed, 258 insertions(+)
>  create mode 100644 tests/core_hot_reload.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 214698da..d2c0941d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -16,6 +16,7 @@ TESTS_progs = \
>  	core_getclient \
>  	core_getstats \
>  	core_getversion \
> +	core_hot_reload \
>  	core_setmaster_vs_auth \
>  	debugfs_test \
>  	drm_import_export \
> diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> new file mode 100644
> index 00000000..d862c99c
> --- /dev/null
> +++ b/tests/core_hot_reload.c
> @@ -0,0 +1,256 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +#include "igt_device.h"
> +#include "igt_dummyload.h"
> +#include "igt_kmod.h"
> +#include "igt_sysfs.h"
> +
> +#include <getopt.h>
> +#include <limits.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +
> +typedef int (*action_t)(int dir);
> +typedef void (*workload_wait_t)(void *priv);
> +typedef void (*workload_t)(int device, const void *priv);
> +
> +
> +/*
> + * Actions
> + *
> + * Purpose: make the device disappear
> + *
> + * @dir: file descriptor of an open device sysfs directory
> + *
> + * Return value: file descriptor of an open device bus' sysfs directory
> + * 		 or -1 if no bus rescan is needed
> + */
> +
> +/* Unbind the driver from the device */
I am glad that more documentation had come. I would only prefer to have it
somehow consistent for functions in whole binary - please have either:
/**
 * name:
 * variables
 * 
 * Return values
 */
or simple comments /* */
But this is just a tiny thing :)
> +static int driver_unbind(int dir)
> +{
> +	char path[PATH_MAX], *dev_bus_addr;
> +	int len;
> +
> +	len = readlinkat(dir, "device", path, sizeof(path) - 1);
> +	path[len] = '\0';
> +	dev_bus_addr = strrchr(path, '/') + 1;
> +
> +	igt_set_timeout(60, "Driver unbind timeout!");
> +	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
> +
> +	/* No need for bus rescan */
> +	return -1;
In doc above you wrote that this functions returns -1 or fd of the device. It is
possible that I am wrong, but I do not see fd returned (at least not
explicitly).
^^^^ So this was my comment before I read carefully later on. So it looks like
doc is wrong, and you return -1 and nothing else. Why do we need to return a
const here? Maybe we can return value from igt_sysfs_set?
> +}
> +
> +/* Remove (virtually unplug) the device from its bus */
> +static int device_unplug(int dir)
> +{
> +	int bus;
> +
> +	bus = openat(dir, "device/subsystem", O_DIRECTORY);
> +	igt_assert(bus >= 0);
> +
> +	igt_set_timeout(60, "Device unplug timeout!");
> +	igt_sysfs_set(dir, "device/remove", "1");
> +
> +	return bus;
> +}
> +
> +
> +/*
> + * Workloads
> + *
> + * Purpose: Put some long lasting load on the device
> + *
> + * @device: open device file descriptor,
> + * @priv: pointer to an optional argument passed to the workload
> + *
> + * Return value: none
> + */
> +
> +/* Workload using igt_spin_batch_run() */
> +
> +static void spin_batch(int device, const void *priv)
> +{
> +	igt_spin_t *spin;
> +
> +	/* submit the job */
> +	spin = igt_spin_batch_new(device);
> +
> +	/* wait for the job to crash */
> +	gem_sync(device, spin->handle);
> +
> +	/* clean up if still possible */
> +	igt_spin_batch_free(device, spin);
> +}
> +
> +
> +/*
> + * Skeleton
> + */
> +
> +static void healthcheck(int chipset)
> +{
> +	if (chipset == DRIVER_INTEL) {
> +		/*
> +		 * We have it perfectly implemented in i915_module_load,
> +		 * just use it.
> +		 */
> +		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
> +			   == IGT_EXIT_SUCCESS);
> +	} else {
> +		/*
> +		 * We don't know how to check an unidentified device for health,
> +		 * device reopen must suffice.
> +		 */
> +	}
Just a question - maybe we can add something like 'igt_skip_on(!DRIVER_INTEL)?
with same comment as in else statement?
It is only another approach :)
> +}
> +
> +static void driver_unload(int chipset, char *driver)
> +{
> +	if (chipset == DRIVER_INTEL)
> +		igt_assert(igt_i915_driver_unload() == IGT_EXIT_SUCCESS);
> +	else
> +		igt_assert(igt_kmod_unload(driver, 0) == 0);
> +}
> +
> +static int operation(int device, action_t action, workload_wait_t workload_wait,
> +		     void *workload_priv)
> +{
> +	int dir, bus;
> +
> +	dir = igt_sysfs_open(device);
> +
> +	bus = action(dir);
> +	close(dir);
> +
> +	if (workload_wait)
> +		workload_wait(workload_priv);
> +	igt_reset_timeout();
> +
> +	return bus;
> +}
> +
> +
> +static void __subtest(int device, int chipset, char *driver, action_t action,
> +		      workload_wait_t workload_wait, void *workload_priv)
> +{
> +	int bus = operation(device, action, workload_wait, workload_priv);
> +
> +	close(device);
> +	driver_unload(chipset, driver);
> +
> +	/* Valid file descriptor indicates we should rescan the bus */
> +	if (bus >= 0) {
> +		igt_sysfs_set(bus, "rescan", "1");
> +		close(bus);
> +	}
> +}
> +
> +static void run_subtest(int *device, int chipset, char *driver, action_t action,
> +			workload_wait_t workload_wait, void *workload_priv)
> +{
> +	__subtest(*device, chipset, driver, action, workload_wait, workload_priv);
> +
> +	healthcheck(chipset);
> +
> +	*device = __drm_open_driver(chipset);
> +	igt_assert(*device >= 0);
> +}
> +
> +static void __workload(workload_t workload, int device, const void *priv,
> +		      struct igt_helper_process *proc)
> +{
> +	igt_fork_helper(proc)
> +		workload(device, priv);
> +	/* let the background process start doing its job */
> +	sleep(2);
> +}
> +
> +static void __workload_wait(void *priv)
> +{
> +	struct igt_helper_process *proc = priv;
> +
> +	/* wait until the workload has crashed */
> +	igt_wait_helper(proc);
> +}
> +
> +
> +igt_main {
> +	int device, chipset;
> +	char *driver;
> +	struct igt_helper_process proc = {};
> +	workload_wait_t workload_wait;
> +	void *workload_priv;
> +
> +	igt_fixture {
> +		char path[PATH_MAX];
> +		int dir, len;
> +
> +		/*
> +		 * Since the test depends on successful unload of driver module,
> +		 * don't use drm_open_driver() as it keeps a file descriptor
> +		 * open for exit handler use that effectively locks the module.
> +		 */
> +		device = __drm_open_driver(DRIVER_ANY);
> +		igt_assert(device >= 0);
> +
> +		if (is_i915_device(device)) {
> +			chipset = DRIVER_INTEL;
> +			driver = strdup("i915");
> +		} else {
> +			chipset = DRIVER_ANY;
> +
> +			/* Capture module name to be unloaded */
> +			dir = igt_sysfs_open(device);
> +			len = readlinkat(dir, "device/driver/module", path,
> +					 sizeof(path) - 1);
> +			close(dir);
> +			path[len] = '\0';
> +			driver = strdup(strrchr(path, '/') + 1);
> +		}
> +		igt_info("Running the test on driver \"%s\", chipset mask %#0x\n",
> +			 driver, chipset);
> +
> +		workload_wait = __workload_wait;
> +		workload_priv = &proc;
> +		__workload(spin_batch, device, NULL, &proc);
> +	}
> +
> +	igt_subtest("unplug")
> +		run_subtest(&device, chipset, driver, device_unplug,
> +			    workload_wait, workload_priv);
> +
> +	igt_subtest("unbind")
> +		run_subtest(&device, chipset, driver, driver_unbind,
> +			    workload_wait, workload_priv);
> +
> +	igt_fixture {
> +		free(driver);
> +		close(device);
> +	}
> +}
Generally everything looks ok. Few fixes and this can be it.
I really like you approach with test building blocks.

Kasia
> diff --git a/tests/meson.build b/tests/meson.build
> index 5167a6cc..1b91e5a2 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -3,6 +3,7 @@ test_progs = [
>  	'core_getclient',
>  	'core_getstats',
>  	'core_getversion',
> +	'core_hot_reload',
>  	'core_setmaster_vs_auth',
>  	'debugfs_test',
>  	'drm_import_export',
> -- 
> 2.20.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v5 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

On Tuesday, April 9, 2019 4:50:49 PM CEST Katarzyna Dec wrote:
> On Tue, Apr 09, 2019 at 01:10:58PM +0200, Janusz Krzysztofik wrote:
> > From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > 
> > Run a dummy load in background to put some workload on a device, then try
> > to either remove (unplug) the device from its bus, or unbind the device's
> > driver from it, depending on which subtest has been selected.  If
> > succeeded, unload the driver, rescan the device's bus if needed and
> > perform health checks on the device with the driver reloaded.
> > 
> > The dummy load is run from igt_fixture and in a sub-process, not directly
> > from subtests,  as it is expected to fail and it's more simple to ignore
> > igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
> > and exits, resources it was using are freed automatically so there is no
> > need to do any cleanups required for smooth module unload from the test
> > level itself.  Those cleanups might also make the subtests fail if simply
> > using igt library functions for that instead of reimplementing their safe
> > parts only.
> > 
> > The driver hot unbind / device hot unplug operation is expected to succeed
> > and the background workload sub-process to die in a reasonable time,
> > however long timeouts are used to let kernel level timeouts pop up first
> > if hit by a bug.
> > 
> > The dummy load works only on i915 driver.  The test is skipped on other
> > hardware unless they provide their implementation of igt_spin_batch_new()
> > and friends.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > ---
> > 
> >  tests/Makefile.sources  |   1 +
> >  tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
> >  tests/meson.build       |   1 +
> >  3 files changed, 258 insertions(+)
> >  create mode 100644 tests/core_hot_reload.c
> > 
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 214698da..d2c0941d 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -16,6 +16,7 @@ TESTS_progs = \
> > 
> >  	core_getclient \
> >  	core_getstats \
> >  	core_getversion \
> > 
> > +	core_hot_reload \
> > 
> >  	core_setmaster_vs_auth \
> >  	debugfs_test \
> >  	drm_import_export \
> > 
> > diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> > new file mode 100644
> > index 00000000..d862c99c
> > --- /dev/null
> > +++ b/tests/core_hot_reload.c
> > @@ -0,0 +1,256 @@
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining
> > a
> > + * copy of this software and associated documentation files (the
> > "Software"), + * to deal in the Software without restriction, including
> > without limitation + * the rights to use, copy, modify, merge, publish,
> > distribute, sublicense, + * and/or sell copies of the Software, and to
> > permit persons to whom the + * Software is furnished to do so, subject to
> > the following conditions: + *
> > + * The above copyright notice and this permission notice (including the
> > next + * paragraph) shall be included in all copies or substantial
> > portions of the + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND
> > NONINFRINGEMENT.  IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS
> > BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN
> > ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN
> > CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE
> > SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +#include "igt_device.h"
> > +#include "igt_dummyload.h"
> > +#include "igt_kmod.h"
> > +#include "igt_sysfs.h"
> > +
> > +#include <getopt.h>
> > +#include <limits.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +
> > +typedef int (*action_t)(int dir);
> > +typedef void (*workload_wait_t)(void *priv);
> > +typedef void (*workload_t)(int device, const void *priv);
> > +
> > +
> > +/*
> > + * Actions
> > + *
> > + * Purpose: make the device disappear
> > + *
> > + * @dir: file descriptor of an open device sysfs directory
> > + *
> > + * Return value: file descriptor of an open device bus' sysfs directory
> > + * 		 or -1 if no bus rescan is needed
> > + */

The above comment provides information not on a particular function but rather 
on a patter that should be followed by functions performing particular test 
actions.  Definition of two such functions follow.  Their callers expect an fd 
of a sysfs directory corresponding to a bus to be rescanned as a return value, 
or -1 if no bus rescan is needed.

> > +
> > +/* Unbind the driver from the device */
> 
> I am glad that more documentation had come. I would only prefer to have it
> somehow consistent for functions in whole binary - please have either:
> /**
>  * name:
>  * variables
>  *
>  * Return values
>  */
> or simple comments /* */
> But this is just a tiny thing :)

I hope the explanation above, though more oriented on clarification of that -1 
case, also clarifies your concern on comments.

> > +static int driver_unbind(int dir)
> > +{
> > +	char path[PATH_MAX], *dev_bus_addr;
> > +	int len;
> > +
> > +	len = readlinkat(dir, "device", path, sizeof(path) - 1);
> > +	path[len] = '\0';
> > +	dev_bus_addr = strrchr(path, '/') + 1;
> > +
> > +	igt_set_timeout(60, "Driver unbind timeout!");
> > +	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
> > +
> > +	/* No need for bus rescan */
> > +	return -1;
> 
> In doc above you wrote that this functions returns -1 or fd of the device.
> It is possible that I am wrong, but I do not see fd returned (at least not
> explicitly).
> ^^^^ So this was my comment before I read carefully later on. So it looks
> like doc is wrong, and you return -1 and nothing else. Why do we need to
> return a const here? Maybe we can return value from igt_sysfs_set?

This particular test action function above always returns -1 because no bus 
rescan is needed.  Its caller will know what -1 means.

Please note that next test action function below returns a bus fd becase in 
that case bus rescan is required.

> 
> > +}
> > +
> > +/* Remove (virtually unplug) the device from its bus */
> > +static int device_unplug(int dir)
> > +{
> > +	int bus;
> > +
> > +	bus = openat(dir, "device/subsystem", O_DIRECTORY);
> > +	igt_assert(bus >= 0);
> > +
> > +	igt_set_timeout(60, "Device unplug timeout!");
> > +	igt_sysfs_set(dir, "device/remove", "1");
> > +
> > +	return bus;
> > +}
> > +

No more test action functions, only those two above, one requiring bus rescan, 
the other - not.  :-)

> > +
> > +/*
> > + * Workloads
> > + *
> > + * Purpose: Put some long lasting load on the device
> > + *
> > + * @device: open device file descriptor,
> > + * @priv: pointer to an optional argument passed to the workload
> > + *
> > + * Return value: none
> > + */

Like in case of test action functions, the comment above describes a pattern 
that should be followed by functions which apply workload.  Only one such 
function, the one below, is implemented.

> > +
> > +/* Workload using igt_spin_batch_run() */
> > +
> > +static void spin_batch(int device, const void *priv)
> > +{
> > +	igt_spin_t *spin;
> > +
> > +	/* submit the job */
> > +	spin = igt_spin_batch_new(device);
> > +
> > +	/* wait for the job to crash */
> > +	gem_sync(device, spin->handle);
> > +
> > +	/* clean up if still possible */
> > +	igt_spin_batch_free(device, spin);
> > +}
> > +
> > +
> > +/*
> > + * Skeleton
> > + */
> > +
> > +static void healthcheck(int chipset)
> > +{
> > +	if (chipset == DRIVER_INTEL) {
> > +		/*
> > +		 * We have it perfectly implemented in 
i915_module_load,
> > +		 * just use it.
> > +		 */
> > +		igt_assert(igt_system_quiet("i915_module_load --run-
subtest reload")
> > +			   == IGT_EXIT_SUCCESS);
> > +	} else {
> > +		/*
> > +		 * We don't know how to check an unidentified device 
for health,
> > +		 * device reopen must suffice.
> > +		 */
> > +	}
> 
> Just a question - maybe we can add something like
> 'igt_skip_on(!DRIVER_INTEL)? with same comment as in else statement?
> It is only another approach :)

I think non-Intel people would better decide if they want the test skipped or 
still run with no exhaustive health checking.

> > +}
> > +
> > +static void driver_unload(int chipset, char *driver)
> > +{
> > +	if (chipset == DRIVER_INTEL)
> > +		igt_assert(igt_i915_driver_unload() == 
IGT_EXIT_SUCCESS);
> > +	else
> > +		igt_assert(igt_kmod_unload(driver, 0) == 0);
> > +}
> > +
> > +static int operation(int device, action_t action, workload_wait_t
> > workload_wait, +		     void *workload_priv)
> > +{
> > +	int dir, bus;
> > +
> > +	dir = igt_sysfs_open(device);
> > +
> > +	bus = action(dir);
> > +	close(dir);
> > +
> > +	if (workload_wait)
> > +		workload_wait(workload_priv);
> > +	igt_reset_timeout();
> > +
> > +	return bus;
> > +}
> > +
> > +
> > +static void __subtest(int device, int chipset, char *driver, action_t
> > action, +		      workload_wait_t workload_wait, void 
*workload_priv)
> > +{
> > +	int bus = operation(device, action, workload_wait, workload_priv);
> > +
> > +	close(device);
> > +	driver_unload(chipset, driver);
> > +
> > +	/* Valid file descriptor indicates we should rescan the bus */
> > +	if (bus >= 0) {
> > +		igt_sysfs_set(bus, "rescan", "1");
> > +		close(bus);
> > +	}
> > +}

And here above we have the recipient of those values returned by test action 
functions.  Based on such return value, it may perform rescan of a bus 
represented by that value.

> > +
> > +static void run_subtest(int *device, int chipset, char *driver, action_t
> > action, +			workload_wait_t workload_wait, 
void *workload_priv)
> > +{
> > +	__subtest(*device, chipset, driver, action, workload_wait,
> > workload_priv); +
> > +	healthcheck(chipset);
> > +
> > +	*device = __drm_open_driver(chipset);
> > +	igt_assert(*device >= 0);
> > +}
> > +
> > +static void __workload(workload_t workload, int device, const void *priv,
> > +		      struct igt_helper_process *proc)
> > +{
> > +	igt_fork_helper(proc)
> > +		workload(device, priv);
> > +	/* let the background process start doing its job */
> > +	sleep(2);
> > +}
> > +
> > +static void __workload_wait(void *priv)
> > +{
> > +	struct igt_helper_process *proc = priv;
> > +
> > +	/* wait until the workload has crashed */
> > +	igt_wait_helper(proc);
> > +}
> > +
> > +
> > +igt_main {
> > +	int device, chipset;
> > +	char *driver;
> > +	struct igt_helper_process proc = {};
> > +	workload_wait_t workload_wait;
> > +	void *workload_priv;
> > +
> > +	igt_fixture {
> > +		char path[PATH_MAX];
> > +		int dir, len;
> > +
> > +		/*
> > +		 * Since the test depends on successful unload of 
driver module,
> > +		 * don't use drm_open_driver() as it keeps a file 
descriptor
> > +		 * open for exit handler use that effectively locks the 
module.
> > +		 */
> > +		device = __drm_open_driver(DRIVER_ANY);
> > +		igt_assert(device >= 0);
> > +
> > +		if (is_i915_device(device)) {
> > +			chipset = DRIVER_INTEL;
> > +			driver = strdup("i915");
> > +		} else {
> > +			chipset = DRIVER_ANY;
> > +
> > +			/* Capture module name to be unloaded */
> > +			dir = igt_sysfs_open(device);
> > +			len = readlinkat(dir, "device/driver/
module", path,
> > +					 sizeof(path) - 1);
> > +			close(dir);
> > +			path[len] = '\0';
> > +			driver = strdup(strrchr(path, '/') + 1);
> > +		}
> > +		igt_info("Running the test on driver \"%s\", chipset 
mask %#0x\n",
> > +			 driver, chipset);
> > +
> > +		workload_wait = __workload_wait;
> > +		workload_priv = &proc;
> > +		__workload(spin_batch, device, NULL, &proc);
> > +	}
> > +
> > +	igt_subtest("unplug")
> > +		run_subtest(&device, chipset, driver, device_unplug,
> > +			    workload_wait, workload_priv);
> > +
> > +	igt_subtest("unbind")
> > +		run_subtest(&device, chipset, driver, driver_unbind,
> > +			    workload_wait, workload_priv);
> > +
> > +	igt_fixture {
> > +		free(driver);
> > +		close(device);
> > +	}
> > +}
> 
> Generally everything looks ok. Few fixes and this can be it.
> I really like you approach with test building blocks.

I'm glad to hear that, and thanks for your review.

Janusz

> 
> Kasia
> 
> > diff --git a/tests/meson.build b/tests/meson.build
> > index 5167a6cc..1b91e5a2 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -3,6 +3,7 @@ test_progs = [
> > 
> >  	'core_getclient',
> >  	'core_getstats',
> >  	'core_getversion',
> > 
> > +	'core_hot_reload',
> > 
> >  	'core_setmaster_vs_auth',
> >  	'debugfs_test',
> >  	'drm_import_export',




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

[igt-dev] [PATCH v5 2/2] tests/core_hot_reload: Accept external workload

[Janusz Krzysztofik]

From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>

Run a user specified command, possibly one of existing tests, in
background instead of the default dummy load to put some alternative
workload on a device while trying to either remove (unplug) the device
from its bus, or unbind the device's driver from it, depending on which
subtest has been selected.

The command selected as the alternative workload should make real use of
the device as much as possible and its execution should take
significantly more than 2 seconds in order to get reliable results from
the test.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
---
 tests/core_hot_reload.c | 51 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
index d862c99c..0d0795dc 100644
--- a/tests/core_hot_reload.c
+++ b/tests/core_hot_reload.c
@@ -33,6 +33,9 @@
 #include <unistd.h>
 
 
+#define OPT_WORKLOAD_CMD	'c'
+
+
 typedef int (*action_t)(int dir);
 typedef void (*workload_wait_t)(void *priv);
 typedef void (*workload_t)(int device, const void *priv);
@@ -108,6 +111,16 @@ static void spin_batch(int device, const void *priv)
 	igt_spin_batch_free(device, spin);
 }
 
+/* Workload using external command */
+
+static void ext_cmd(int device, const void *priv)
+{
+	const char *cmd = priv;
+
+	/* just run the user provided command line */
+	igt_system_quiet(cmd);
+}
+
 
 /*
  * Skeleton
@@ -200,12 +213,40 @@ static void __workload_wait(void *priv)
 }
 
 
-igt_main {
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	const char **cmd = data;
+
+	switch (opt) {
+	case 'c':
+		*cmd = strdup(optarg);
+		break;
+	}
+
+	return 0;
+}
+
+int main (int argc, char **argv)
+{
 	int device, chipset;
 	char *driver;
 	struct igt_helper_process proc = {};
 	workload_wait_t workload_wait;
 	void *workload_priv;
+	const struct option long_opts[] = {
+		{ "workload-cmd", required_argument, 0, OPT_WORKLOAD_CMD },
+		{ 0, 0, 0, 0 }
+	};
+	const char *help_str =
+		"  --workload-cmd, -c\n"
+		"\t\tCommand line to run in backgroud as an optional workload, e.g.,\n"
+		"\t\tan existing test name, possibly with options/arguments, quoted.\n"
+		"\t\tExecution must take way more than 2 seconds for reliable results.\n"
+		"\t\tNo default - internal dummy load is used if external not specified.\n";
+	const char *cmd = NULL;
+
+	igt_subtest_init_parse_opts(&argc, argv, "", long_opts, help_str,
+                                    opt_handler, &cmd);
 
 	igt_fixture {
 		char path[PATH_MAX];
@@ -238,7 +279,13 @@ igt_main {
 
 		workload_wait = __workload_wait;
 		workload_priv = &proc;
-		__workload(spin_batch, device, NULL, &proc);
+
+		if (cmd) {
+			igt_device_drop_master(device);
+			__workload(ext_cmd, -1, cmd, &proc);
+		} else {
+			__workload(spin_batch, device, NULL, &proc);
+		}
 	}
 
 	igt_subtest("unplug")
-- 
2.20.1

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

Re: [igt-dev] [PATCH v5 2/2] tests/core_hot_reload: Accept external workload

[Katarzyna Dec]

On Tue, Apr 09, 2019 at 01:10:59PM +0200, Janusz Krzysztofik wrote:
> From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> 
> Run a user specified command, possibly one of existing tests, in
> background instead of the default dummy load to put some alternative
> workload on a device while trying to either remove (unplug) the device
> from its bus, or unbind the device's driver from it, depending on which
> subtest has been selected.
> 
> The command selected as the alternative workload should make real use of
> the device as much as possible and its execution should take
> significantly more than 2 seconds in order to get reliable results from
> the test.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> ---
>  tests/core_hot_reload.c | 51 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> index d862c99c..0d0795dc 100644
> --- a/tests/core_hot_reload.c
> +++ b/tests/core_hot_reload.c
> @@ -33,6 +33,9 @@
>  #include <unistd.h>
>  
>  
> +#define OPT_WORKLOAD_CMD	'c'
> +
> +
>  typedef int (*action_t)(int dir);
>  typedef void (*workload_wait_t)(void *priv);
>  typedef void (*workload_t)(int device, const void *priv);
> @@ -108,6 +111,16 @@ static void spin_batch(int device, const void *priv)
>  	igt_spin_batch_free(device, spin);
>  }
>  
> +/* Workload using external command */
Can you provide examples of such commands? 
> +
> +static void ext_cmd(int device, const void *priv)
> +{
> +	const char *cmd = priv;
> +
> +	/* just run the user provided command line */
> +	igt_system_quiet(cmd);
> +}
> +
>  
>  /*
>   * Skeleton
> @@ -200,12 +213,40 @@ static void __workload_wait(void *priv)
>  }
>  
>  
> -igt_main {
> +static int opt_handler(int opt, int opt_index, void *data)
> +{
> +	const char **cmd = data;
> +
> +	switch (opt) {
> +	case 'c':
> +		*cmd = strdup(optarg);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +int main (int argc, char **argv)
> +{
>  	int device, chipset;
>  	char *driver;
>  	struct igt_helper_process proc = {};
>  	workload_wait_t workload_wait;
>  	void *workload_priv;
> +	const struct option long_opts[] = {
> +		{ "workload-cmd", required_argument, 0, OPT_WORKLOAD_CMD },
> +		{ 0, 0, 0, 0 }
> +	};
> +	const char *help_str =
> +		"  --workload-cmd, -c\n"
> +		"\t\tCommand line to run in backgroud as an optional workload, e.g.,\n"
> +		"\t\tan existing test name, possibly with options/arguments, quoted.\n"
> +		"\t\tExecution must take way more than 2 seconds for reliable results.\n"
> +		"\t\tNo default - internal dummy load is used if external not specified.\n";
> +	const char *cmd = NULL;
> +
> +	igt_subtest_init_parse_opts(&argc, argv, "", long_opts, help_str,
> +                                    opt_handler, &cmd);
>  
>  	igt_fixture {
>  		char path[PATH_MAX];
> @@ -238,7 +279,13 @@ igt_main {
>  
>  		workload_wait = __workload_wait;
>  		workload_priv = &proc;
> -		__workload(spin_batch, device, NULL, &proc);
> +
> +		if (cmd) {
> +			igt_device_drop_master(device);
Are you sure this will be working as you expect? Have you tested? (Probably it
will)

Kasia :)
> +			__workload(ext_cmd, -1, cmd, &proc);
> +		} else {
> +			__workload(spin_batch, device, NULL, &proc);
> +		}
>  	}
>  
>  	igt_subtest("unplug")
> -- 
> 2.20.1
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v5 2/2] tests/core_hot_reload: Accept external workload

[Janusz Krzysztofik]

On Tuesday, April 9, 2019 4:56:36 PM CEST Katarzyna Dec wrote:
> On Tue, Apr 09, 2019 at 01:10:59PM +0200, Janusz Krzysztofik wrote:
> > From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > 
> > Run a user specified command, possibly one of existing tests, in
> > background instead of the default dummy load to put some alternative
> > workload on a device while trying to either remove (unplug) the device
> > from its bus, or unbind the device's driver from it, depending on which
> > subtest has been selected.
> > 
> > The command selected as the alternative workload should make real use of
> > the device as much as possible and its execution should take
> > significantly more than 2 seconds in order to get reliable results from
> > the test.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > ---
> > 
> >  tests/core_hot_reload.c | 51 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 49 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> > index d862c99c..0d0795dc 100644
> > --- a/tests/core_hot_reload.c
> > +++ b/tests/core_hot_reload.c
> > @@ -33,6 +33,9 @@
> > 
> >  #include <unistd.h>
> > 
> > +#define OPT_WORKLOAD_CMD	'c'
> > +
> > +
> > 
> >  typedef int (*action_t)(int dir);
> >  typedef void (*workload_wait_t)(void *priv);
> >  typedef void (*workload_t)(int device, const void *priv);
> > 
> > @@ -108,6 +111,16 @@ static void spin_batch(int device, const void *priv)
> > 
> >  	igt_spin_batch_free(device, spin);
> >  
> >  }
> > 
> > +/* Workload using external command */
> 
> Can you provide examples of such commands?

One particular test I was able to get a kernel panic with while removing the 
device on the fly was:

    gem_exec_nop --run-subtest basic-range

Run from my test, it looks like that:

    core_hot_reload --run-subtest unplug --workload-cmd "gem_exec_nop --run-
subtest basic-range"

> > +
> > +static void ext_cmd(int device, const void *priv)
> > +{
> > +	const char *cmd = priv;
> > +
> > +	/* just run the user provided command line */
> > +	igt_system_quiet(cmd);
> > +}
> > +
> > 
> >  /*
> >  
> >   * Skeleton
> > 
> > @@ -200,12 +213,40 @@ static void __workload_wait(void *priv)
> > 
> >  }
> > 
> > -igt_main {
> > +static int opt_handler(int opt, int opt_index, void *data)
> > +{
> > +	const char **cmd = data;
> > +
> > +	switch (opt) {
> > +	case 'c':
> > +		*cmd = strdup(optarg);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int main (int argc, char **argv)
> > +{
> > 
> >  	int device, chipset;
> >  	char *driver;
> >  	struct igt_helper_process proc = {};
> >  	workload_wait_t workload_wait;
> >  	void *workload_priv;
> > 
> > +	const struct option long_opts[] = {
> > +		{ "workload-cmd", required_argument, 0, 
OPT_WORKLOAD_CMD },
> > +		{ 0, 0, 0, 0 }
> > +	};
> > +	const char *help_str =
> > +		"  --workload-cmd, -c\n"
> > +		"\t\tCommand line to run in backgroud as an optional 
workload, e.g.,\n"
> > +		"\t\tan existing test name, possibly with options/
arguments, quoted.\n"
> > +		"\t\tExecution must take way more than 2 seconds for 
reliable
> > results.\n" +		"\t\tNo default - internal dummy load is 
used if external
> > not specified.\n"; +	const char *cmd = NULL;
> > +
> > +	igt_subtest_init_parse_opts(&argc, argv, "", long_opts, help_str,
> > +                                    opt_handler, &cmd);
> > 
> >  	igt_fixture {
> >  	
> >  		char path[PATH_MAX];
> > 
> > @@ -238,7 +279,13 @@ igt_main {
> > 
> >  		workload_wait = __workload_wait;
> >  		workload_priv = &proc;
> > 
> > -		__workload(spin_batch, device, NULL, &proc);
> > +
> > +		if (cmd) {
> > +			igt_device_drop_master(device);
> 
> Are you sure this will be working as you expect? Have you tested? (Probably
> it will)

Yes, I added that as a result of negative outcomes from using external 
workloads which were complaining about not being able to take over device 
master role, and it helped.

BTW, since I'm this patch replaces igt_main with just main(), an explicitly 
called igt_exit() is missing.  I'm about to submit a new, fixed version.

Thanks,
Janusz


> 
> Kasia :)
> 
> > +			__workload(ext_cmd, -1, cmd, &proc);
> > +		} else {
> > +			__workload(spin_batch, device, NULL, 
&proc);
> > +		}
> > 
> >  	}
> >  	
> >  	igt_subtest("unplug")




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

Re: [igt-dev] [PATCH v5 2/2] tests/core_hot_reload: Accept external workload

[Katarzyna Dec]

On Wed, Apr 10, 2019 at 11:14:46AM +0200, Janusz Krzysztofik wrote:
> On Tuesday, April 9, 2019 4:56:36 PM CEST Katarzyna Dec wrote:
> > On Tue, Apr 09, 2019 at 01:10:59PM +0200, Janusz Krzysztofik wrote:
> > > From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > > 
> > > Run a user specified command, possibly one of existing tests, in
> > > background instead of the default dummy load to put some alternative
> > > workload on a device while trying to either remove (unplug) the device
> > > from its bus, or unbind the device's driver from it, depending on which
> > > subtest has been selected.
> > > 
> > > The command selected as the alternative workload should make real use of
> > > the device as much as possible and its execution should take
> > > significantly more than 2 seconds in order to get reliable results from
> > > the test.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > > ---
> > > 
> > >  tests/core_hot_reload.c | 51 +++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 49 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> > > index d862c99c..0d0795dc 100644
> > > --- a/tests/core_hot_reload.c
> > > +++ b/tests/core_hot_reload.c
> > > @@ -33,6 +33,9 @@
> > > 
> > >  #include <unistd.h>
> > > 
> > > +#define OPT_WORKLOAD_CMD	'c'
> > > +
> > > +
> > > 
> > >  typedef int (*action_t)(int dir);
> > >  typedef void (*workload_wait_t)(void *priv);
> > >  typedef void (*workload_t)(int device, const void *priv);
> > > 
> > > @@ -108,6 +111,16 @@ static void spin_batch(int device, const void *priv)
> > > 
> > >  	igt_spin_batch_free(device, spin);
> > >  
> > >  }
> > > 
> > > +/* Workload using external command */
> > 
> > Can you provide examples of such commands?
> 
> One particular test I was able to get a kernel panic with while removing the 
> device on the fly was:
> 
>     gem_exec_nop --run-subtest basic-range
> 
> Run from my test, it looks like that:
> 
>     core_hot_reload --run-subtest unplug --workload-cmd "gem_exec_nop --run-
> subtest basic-range"
> 
Now I see how it is supposed to work. This example or similar should be at least
a part of commit msg, ideally also reflected in binary. As you see it is not
obvious what kind of workloads we can use.
I saw that you already send v7, please fix that and hopefully we will be able to
have v8 final version.

btw - you can always answer review question or clarify sth on irc :)

Sorry for responding so late.
Kasia :)
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for Add a new test for driver/device hot reload

[Patchwork]

== Series Details ==

Series: Add a new test for driver/device hot reload
URL   : https://patchwork.freedesktop.org/series/59221/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
fef5706e11dcfc72cc759242842d36e7c8dc5e31 tests/kms_plane_scaling: Check supported format for rotation

245/267 testcase check: i915_suspend            OK       0.13 s 
246/267 testcase check: gem_ctx_sseu            OK       0.12 s 
247/267 testcase check: gem_eio                 OK       0.12 s 
248/267 testcase check: gem_mocs_settings       OK       0.11 s 
249/267 testcase check: perf_pmu                OK       0.08 s 
250/267 testcase check: testdisplay             OK       0.12 s 
251/267 testcase check: amdgpu/amd_abm          OK       0.08 s 
252/267 testcase check: amdgpu/amd_basic        OK       0.09 s 
253/267 testcase check: amdgpu/amd_cs_nop       OK       0.09 s 
254/267 testcase check: amdgpu/amd_prime        OK       0.05 s 
255/267 runner                                  OK       1.49 s 
256/267 runner_json                             OK       0.09 s 
257/267 assembler: test/mov                     OK       0.07 s 
258/267 assembler: test/frc                     OK       0.04 s 
259/267 assembler: test/regtype                 OK       0.07 s 
260/267 assembler: test/rndd                    OK       0.05 s 
261/267 assembler: test/rndu                    OK       0.06 s 
262/267 assembler: test/rnde                    OK       0.05 s 
263/267 assembler: test/rnde-intsrc             OK       0.05 s 
264/267 assembler: test/rndz                    OK       0.03 s 
265/267 assembler: test/lzd                     OK       0.05 s 
266/267 assembler: test/not                     OK       0.04 s 
267/267 assembler: test/immediate               OK       0.03 s 

OK:       266
FAIL:       1
SKIP:       0
TIMEOUT:    0


The output from the failed tests:

 22/267 testcase check: core_hot_reload         FAIL     0.28 s (exit status 1)

--- command ---
/home/cidrm/igt-gpu-tools/tests/igt_command_line.sh core_hot_reload
--- stdout ---
tests/core_hot_reload:
  Checking invalid option handling...
  Checking valid option handling...
  Checking subtest enumeration...
FAIL: tests/core_hot_reload
--- stderr ---
core_hot_reload: ../lib/igt_core.c:524: common_exit_handler: Assertion `sig != 0 || igt_exit_called' failed.
-------

Full log written to /home/cidrm/igt-gpu-tools/build/meson-logs/testlog.txt
FAILED: meson-test 
/usr/bin/python3 -u /usr/bin/meson test --no-rebuild --print-errorlogs
ninja: build stopped: subcommand failed.

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