[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 at 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 at 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")