[igt-dev] [PATCH i-g-t 9/9] tests/perf_pmu: Use short batches from hotplug test

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-02 18:37:54)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> This test emits a spin batch which runs roughly for N CPU cores seconds
> As such these can be declared as GPU hangs, so work around that by looping
> with shorter batches.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  tests/perf_pmu.c | 66 +++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> index 201aa0b40068..7f2fa64834d7 100644
> --- a/tests/perf_pmu.c
> +++ b/tests/perf_pmu.c
> @@ -84,15 +84,23 @@ init(int gem_fd, const struct intel_execution_engine2 *e, uint8_t sample)
>         close(fd);
>  }
>  
> -static uint64_t pmu_read_single(int fd)
> +static uint64_t __pmu_read_single(int fd, uint64_t *ts)
>  {
>         uint64_t data[2];
>  
>         igt_assert_eq(read(fd, data, sizeof(data)), sizeof(data));
>  
> +       if (ts)
> +               *ts = data[1];
> +
>         return data[0];
>  }
>  
> +static uint64_t pmu_read_single(int fd)
> +{
> +       return __pmu_read_single(fd, NULL);
> +}
> +
>  static void pmu_read_multi(int fd, unsigned int num, uint64_t *val)
>  {
>         uint64_t buf[2 + num];
> @@ -148,7 +156,7 @@ static void end_spin(int fd, igt_spin_t *spin, unsigned int flags)
>  
>         if (flags & FLAG_SYNC)
>                 gem_sync(fd, spin->handle);
> -       else
> +       else if (flags & TEST_TRAILING_IDLE)
>                 usleep(batch_duration_ns / 5000);
>  }
>  
> @@ -930,27 +938,33 @@ static bool cpu0_hotplug_support(void)
>  
>  static void cpu_hotplug(int gem_fd)
>  {
> -       struct timespec start = { };
> +       struct igt_helper_process cpu_shuffle = { };
>         igt_spin_t *spin;
> -       uint64_t val, ref;
> -       int fd;
> +       uint64_t ts[2];
> +       uint64_t val;
> +       int link[2];
> +       int fd, ret;
>  
>         igt_require(cpu0_hotplug_support());

Now I know why I don't see any ill effects ;)

> -       fd = perf_i915_open(I915_PMU_ENGINE_BUSY(I915_ENGINE_CLASS_RENDER, 0));
> -       igt_assert(fd >= 0);
> +       fd = open_pmu(I915_PMU_ENGINE_BUSY(I915_ENGINE_CLASS_RENDER, 0));
>  
>         spin = igt_spin_batch_new(gem_fd, 0, I915_EXEC_RENDER, 0);
>  
> -       igt_nsec_elapsed(&start);
> +       val = __pmu_read_single(fd, &ts[0]);
> +
> +       ret = pipe2(link, O_NONBLOCK);
> +       igt_assert_eq(ret, 0);
>  
>         /*
>          * Toggle online status of all the CPUs in a child process and ensure
>          * this has not affected busyness stats in the parent.
>          */
> -       igt_fork(child, 1) {
> +       igt_fork_helper(&cpu_shuffle) {
>                 int cpu = 0;
>  
> +               close(link[0]);

Honestly would not bother close()ing the temporary fd in the child, the
child isn't resource hungry and will die in due course.

> +
>                 for (;;) {
>                         char name[128];
>                         int cpufd;
> @@ -960,6 +974,7 @@ static void cpu_hotplug(int gem_fd)
>                         cpufd = open(name, O_WRONLY);
>                         if (cpufd == -1) {
>                                 igt_assert(cpu > 0);
> +                               igt_assert_eq(write(link[1], "*", 1), 1);
>                                 break;
>                         }
>                         igt_assert_eq(write(cpufd, "0", 2), 2);
> @@ -971,19 +986,40 @@ static void cpu_hotplug(int gem_fd)
>                         close(cpufd);
>                         cpu++;
>                 }
> +
> +               /* Wait to be terminated. */
> +               for (;;)
> +                       sleep(1);

Why wait? This process isn't doing at this point, so can just gracefully
die. Oh, is it because fork_helper demands you keep it around. So not
seeing the point of not using a child then.

>         }
>  
> -       igt_waitchildren();
> +       close(link[1]);
>  
> -       ref = igt_nsec_elapsed(&start);
> -       val = pmu_read_single(fd);
> +       /*
> +        * Very long batches can be declared as GPU hangs so emit shorter ones
> +        * until the CPU core shuffler finishes one loop.
> +        */
> +       for (;;) {
> +               char buf;
> +               int ret2;
>  
> -       igt_spin_batch_end(spin);
> -       gem_sync(gem_fd, spin->handle);
> +               usleep(500e3);
> +               end_spin(gem_fd, spin, 0);
> +               ret2 = read(link[0], &buf, 1);
> +               if ( ret2 == 1 || (ret2 < 0 && errno != EAGAIN))
> +                       break;

Ok. Whitespace please :) (ret seems perfectly fine to reuse and less
scary than ret2)

Do you want to create a pair of overlapping spinners to prevent the
temporary idleness? Haven't thought about whether that has any
significance, but that seems to a slight difference wrt the old test.

Consider it's a BUSY PMU and you expect the two to match implies that
you would prefer to keep the engine always busy.
-Chris