| Message ID | 20180905134939.2942-5-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tracing & workload simulation misc patches | expand |
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)? > + > + 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
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 >
Quoting Tvrtko Ursulin (2018-09-07 09:37:00) > > 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. Right, it removes the loop but we still want to keep the measurement. -Chris
On Fri, Sep 07, 2018 at 09:45:14AM +0100, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-07 09:37:00) > > > > 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. > > Right, it removes the loop but we still want to keep the measurement. I guess we still want a loop if we're worried about signals? Not sure why else we'd need a loop anyway. But the loop could be just something like 'while (clock_nanosleep()) ;'
On 07/09/2018 15:13, Ville Syrjälä wrote: > On Fri, Sep 07, 2018 at 09:45:14AM +0100, Chris Wilson wrote: >> Quoting Tvrtko Ursulin (2018-09-07 09:37:00) >>> >>> 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. >> >> Right, it removes the loop but we still want to keep the measurement. > > I guess we still want a loop if we're worried about signals? Not sure > why else we'd need a loop anyway. But the loop could be just something > like 'while (clock_nanosleep()) ;' In this case just a case of copying measured_usleep verbatim from another test because it was easy. Otherwise I think it's fine what you suggest, especially since Chris also complained about using igt helpers from outside tests/. So I'll respin to that effect. Regards, Tvrtko
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); + + 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; }