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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/02/2018 21:43, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-02 18:37:54)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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)

Gcc complains about shadowing ret. :I I did most of the other 
suggestions in v2.

Regards,

Tvrtko

> 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
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev