[igt-dev] [PATCH igt] igt/perf_pmu: Disable all cpus

[Tvrtko Ursulin]

On 21/02/2018 09:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-21 09:11:15)
>>
>> On 20/02/2018 21:40, Chris Wilson wrote:
>>> Rather than iteratively disable and then immediately reenable a CPU,
>>> turn off each in turn, forcing the PMU events onto the next CPU without
>>> allowing them to retreat back to CPU0 after the first. If this fails,
>>
>> Hm, interesting and I think it possibly makes sense to test both
>> migration patterns.
> 
> Yup.
> 
>>> @@ -988,35 +999,43 @@ static void cpu_hotplug(int gem_fd)
>>>         */
>>>        igt_fork(child, 1) {
>>>                int cpu = 0;
>>> +             int cpufd;
>>> +             int err;
>>>    
>>>                close(link[0]);
>>>    
>>> +             /* Offline each cpu in turn */
>>>                for (;;) {
>>> -                     char name[128];
>>> -                     int cpufd;
>>> -
>>> -                     igt_assert_lt(snprintf(name, sizeof(name),
>>> -                                            "/sys/devices/system/cpu/cpu%d/online",
>>> -                                            cpu), sizeof(name));
>>> -                     cpufd = open(name, O_WRONLY);
>>> -                     if (cpufd == -1) {
>>> -                             igt_assert(cpu > 0);
>>> -                             /*
>>> -                              * Signal parent that we cycled through all
>>> -                              * CPUs and we are done.
>>> -                              */
>>> -                             igt_assert_eq(write(link[1], "*", 1), 1);
>>> +                     cpufd = open_cpu_online(cpu);
>>> +                     igt_assert(cpufd != -1);
>>> +
>>> +                     err = write(cpufd, "0", 2);
>>> +                     close(cpufd);
>>> +                     if (err < 0)
>>>                                break;
>>
>> Keep off-lining until no more CPUs to offline? I had to try it! :) Ok,
>> last one will fail to offline. But I think it needs a comment.
> 
> I thought that was a fun trick to try and offline the last cpu :)
> 
>>> -                     }
>>>    
>>> -                     /* Offline followed by online a CPU. */
>>> -                     igt_assert_eq(write(cpufd, "0", 2), 2);
>>>                        usleep(1e6);
>>> -                     igt_assert_eq(write(cpufd, "1", 2), 2);
>>> +                     cpu++;
>>> +             }
>>>    
>>> +             /* Then bring them back online */
>>> +             while (cpu--) {
>>> +                     cpufd = open_cpu_online(cpu);
>>> +                     err = write(cpufd, "1", 2);
>>>                        close(cpufd);
>>
>> Need to online in the same order or the PMU will stay on some higher CPU
>> making the subsequent tests fail. Or I need to improve the helpers to
>> hunt for the correct CPU, as perf tool does.
> 
> Ah. I was expecting everytime we onlined a new cpu, the notifier would
> move the pmu. Why do the subsequent tests fail? In my naivety I expected
> one CPU is as good as any other for pmu. Do we need to put a trivial
> test inside the online/offline loops?

It only moves it to the first available CPU once it gets kicked out from 
the one it was on.

So with the above pattern of offline all and online in reverse, it will 
happily stay on the last CPU. And IGTs only try to open on the first CPU 
so it fails to open it from then on.

As I said, it would be easy to support opening our PMU regardless on 
which CPU it currently lives on by wrapping the "try the next cpu" logic 
in the perf open wrappers. In kernel perf tool for instance does that 
already.

I could also change i915 to always try to move to CPU0 if it is 
available, on any online events. But I am not sure that's in the spirit 
of things.

I think making IGT perf open wrapper more robust makes most sense.

Regards,

Tvrtko