Re: [igt-dev] [PATH i-g-t 04/13] gem_wsim: Check sleep times

Tvrtko Ursulin
2018-09-07T08:37:00Z

On 05/09/2018 15:09, Ville Syrjälä wrote:
> On Wed, Sep 05, 2018 at 02:49:30PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Notice in more places if we are running behind.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>> benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index 25af4d678ba4..b05e9760f419 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
>> return synced;
>> }
>>
>> +static unsigned int measured_usleep(unsigned int usec)
>> +{
>> + struct timespec ts = { };
>> + unsigned int slept;
>> +
>> + slept = igt_nsec_elapsed(&ts);
>> + igt_assert(slept == 0);
>> + do {
>> + usleep(usec - slept);
>> + slept = igt_nsec_elapsed(&ts) / 1000;
>> + } while (slept < usec);
>
> clock_nanosleep(ABS)?

Hm I think I see what you mean. Rather than a relative sleep trying to
hit the loop period, ask from the kernel (or glibc, I don't know who
implements it) to sleep until an absolute target. This totally makes
sense and would simplify the code from one angle, I am just not sure if
absolute sleep can be relied upon any better to not oversleep. Well,
actually for scheduling delays not to affect the caller. However maybe
it doesn't matter since AFAIR my main problem were dropped period due
GPU activity (the first pair of warning messages in the patch), and
again AFAIR, it was quite hard to hit the second ones.

I'll revisit it to remind myself for sure.

Thanks for the drive-by, it was extremely surprising you looked into
this obscure for most series. :)

Regards,

Tvrtko

>> +
>> + return igt_nsec_elapsed(&ts);
>> +}
>> +
>> static void *run_workload(void *data)
>> {
>> struct workload *wrk = (struct workload *)data;
>> @@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
>> count++) {
>> unsigned int cur_seqno = wrk->sync_seqno;
>>
>> - clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
>> + igt_gettime(&wrk->repeat_start);
>>
>> for (i = 0, w = wrk->steps; wrk->run && (i < wrk->nr_steps);
>> i++, w++) {
>> @@ -1751,13 +1766,14 @@ static void *run_workload(void *data)
>> } else if (w->type == PERIOD) {
>> struct timespec now;
>>
>> - clock_gettime(CLOCK_MONOTONIC, &now);
>> + igt_gettime(&now);
>> do_sleep = w->period -
>> elapsed_us(&wrk->repeat_start, &now);
>> if (do_sleep < 0) {
>> - if (verbose > 1)
>> - printf("%u: Dropped period @ %u/%u (%dus late)!\n",
>> - wrk->id, count, i, do_sleep);
>> + if (verbose > 1 &&
>> + -do_sleep > 10 * w->period / 100)
>> + printf("%u: Missed period @ %u/%u (%dus late)!\n",
>> + wrk->id, count, i, -do_sleep);
>> continue;
>> }
>> } else if (w->type == SYNC) {
>> @@ -1793,7 +1809,31 @@ static void *run_workload(void *data)
>> }
>>
>> if (do_sleep || w->type == PERIOD) {
>> - usleep(do_sleep);
>> + long slept = measured_usleep(do_sleep) / 1000;
>> +
>> + if ( w->type == PERIOD) {
>> + struct timespec now;
>> +
>> + igt_gettime(&now);
>> +
>> + slept = elapsed_us(&wrk->repeat_start,
>> + &now);
>> +
>> + if (verbose > 1 &&
>> + slept > (110 * w->period / 100))
>> + printf("%u: Overslept period by %ldus @ %u/%u! (%uus)\n",
>> + wrk->id,
>> + -slept, count,
>> + i, w->period);
>> + } else {
>> + if (verbose > 1 &&
>> + slept > (110 * do_sleep / 100))
>> + printf("%u: Overslept by %ldus @ %u/%u! (%uus)\n",
>> + wrk->id,
>> + slept - do_sleep, count,
>> + i, do_sleep);
>> + }
>> +
>> continue;
>> }
>>
>> --
>> 2.17.1
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>