Message ID | 20230314121740.1195358-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [i-g-t] intel_gpu_top: Use actual period when calculating client busyness | expand |
lgtm, Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> On Tue, Mar 14, 2023 at 12:17:40PM +0000, Tvrtko Ursulin wrote: >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >On a slow machine, or with many processes and/or file descriptors to >parse, the period of the scanning loop can drift significantly from the >assumed value. This results in artificially inflated client busyness >percentages. > >To alleviate the issue take some real timestamps and use actual elapsed >time when calculating relative busyness. > >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >--- > tools/intel_gpu_top.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > >diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c >index e13e35b71f4b..af4b350da8e4 100644 >--- a/tools/intel_gpu_top.c >+++ b/tools/intel_gpu_top.c >@@ -43,6 +43,7 @@ > #include <sys/types.h> > #include <unistd.h> > #include <termios.h> >+#include <time.h> > #include <sys/sysmacros.h> > > #include "igt_perf.h" >@@ -2524,6 +2525,38 @@ static void show_help_screen(void) > "\n"); > } > >+static int gettime(struct timespec *ts) >+{ >+ memset(ts, 0, sizeof(*ts)); >+ >+#ifdef CLOCK_MONOTONIC_RAW >+ if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts)) >+ return 0; >+#endif >+#ifdef CLOCK_MONOTONIC_COARSE >+ if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts)) >+ return 0; >+#endif >+ >+ return clock_gettime(CLOCK_MONOTONIC, ts); >+} >+ >+static unsigned long elapsed_us(struct timespec *prev, unsigned int period_us) >+{ >+ unsigned long elapsed; >+ struct timespec now; >+ >+ if (gettime(&now)) >+ return period_us; >+ >+ elapsed = ((now.tv_nsec - prev->tv_nsec) / 1000 + >+ (unsigned long)USEC_PER_SEC * (now.tv_sec - prev->tv_sec)); >+ >+ *prev = now; >+ >+ return elapsed; >+} >+ > int main(int argc, char **argv) > { > unsigned int period_us = DEFAULT_PERIOD_MS * 1000; >@@ -2537,6 +2570,7 @@ int main(int argc, char **argv) > char *pmu_device, *opt_device = NULL; > struct igt_device_card card; > char *codename = NULL; >+ struct timespec ts; > > /* Parse options */ > while ((ch = getopt(argc, argv, "o:s:d:pcJLlh")) != -1) { >@@ -2690,6 +2724,7 @@ int main(int argc, char **argv) > > pmu_sample(engines); > scan_clients(clients, false); >+ gettime(&ts); > codename = igt_device_get_pretty_name(&card, false); > > if (output_mode == JSON) >@@ -2698,6 +2733,7 @@ int main(int argc, char **argv) > while (!stop_top) { > struct clients *disp_clients; > bool consumed = false; >+ unsigned int scan_us; > int j, lines = 0; > struct winsize ws; > struct client *c; >@@ -2720,6 +2756,7 @@ int main(int argc, char **argv) > t = (double)(engines->ts.cur - engines->ts.prev) / 1e9; > > disp_clients = scan_clients(clients, true); >+ scan_us = elapsed_us(&ts, period_us); > > if (stop_top) > break; >@@ -2757,7 +2794,7 @@ int main(int argc, char **argv) > > lines = print_client(c, engines, t, > lines, con_w, >- con_h, period_us, >+ con_h, scan_us, > &class_w); > } > >-- >2.37.2 >
Hi Umesh, On 14/03/2023 18:25, Umesh Nerlige Ramappa wrote: > lgtm, > > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Thanks - I had one second thought though. See below please. > On Tue, Mar 14, 2023 at 12:17:40PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> On a slow machine, or with many processes and/or file descriptors to >> parse, the period of the scanning loop can drift significantly from the >> assumed value. This results in artificially inflated client busyness >> percentages. >> >> To alleviate the issue take some real timestamps and use actual elapsed >> time when calculating relative busyness. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> tools/intel_gpu_top.c | 39 ++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 38 insertions(+), 1 deletion(-) >> >> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c >> index e13e35b71f4b..af4b350da8e4 100644 >> --- a/tools/intel_gpu_top.c >> +++ b/tools/intel_gpu_top.c >> @@ -43,6 +43,7 @@ >> #include <sys/types.h> >> #include <unistd.h> >> #include <termios.h> >> +#include <time.h> >> #include <sys/sysmacros.h> >> >> #include "igt_perf.h" >> @@ -2524,6 +2525,38 @@ static void show_help_screen(void) >> "\n"); >> } >> >> +static int gettime(struct timespec *ts) >> +{ >> + memset(ts, 0, sizeof(*ts)); >> + >> +#ifdef CLOCK_MONOTONIC_RAW >> + if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts)) >> + return 0; >> +#endif >> +#ifdef CLOCK_MONOTONIC_COARSE >> + if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts)) >> + return 0; >> +#endif So I copied this (with some edits) from igt_core.c but I think I should actually remove the CLOCK_MONOTONIC_COARSE option. The usage in intel_gpu_top is not performance sensitive and tick granularity actually defeats to point of this patch. Okay to keep the r-b if I remove it? Regards, Tvrtko >> + >> + return clock_gettime(CLOCK_MONOTONIC, ts); >> +} >> + >> +static unsigned long elapsed_us(struct timespec *prev, unsigned int >> period_us) >> +{ >> + unsigned long elapsed; >> + struct timespec now; >> + >> + if (gettime(&now)) >> + return period_us; >> + >> + elapsed = ((now.tv_nsec - prev->tv_nsec) / 1000 + >> + (unsigned long)USEC_PER_SEC * (now.tv_sec - >> prev->tv_sec)); >> + >> + *prev = now; >> + >> + return elapsed; >> +} >> + >> int main(int argc, char **argv) >> { >> unsigned int period_us = DEFAULT_PERIOD_MS * 1000; >> @@ -2537,6 +2570,7 @@ int main(int argc, char **argv) >> char *pmu_device, *opt_device = NULL; >> struct igt_device_card card; >> char *codename = NULL; >> + struct timespec ts; >> >> /* Parse options */ >> while ((ch = getopt(argc, argv, "o:s:d:pcJLlh")) != -1) { >> @@ -2690,6 +2724,7 @@ int main(int argc, char **argv) >> >> pmu_sample(engines); >> scan_clients(clients, false); >> + gettime(&ts); >> codename = igt_device_get_pretty_name(&card, false); >> >> if (output_mode == JSON) >> @@ -2698,6 +2733,7 @@ int main(int argc, char **argv) >> while (!stop_top) { >> struct clients *disp_clients; >> bool consumed = false; >> + unsigned int scan_us; >> int j, lines = 0; >> struct winsize ws; >> struct client *c; >> @@ -2720,6 +2756,7 @@ int main(int argc, char **argv) >> t = (double)(engines->ts.cur - engines->ts.prev) / 1e9; >> >> disp_clients = scan_clients(clients, true); >> + scan_us = elapsed_us(&ts, period_us); >> >> if (stop_top) >> break; >> @@ -2757,7 +2794,7 @@ int main(int argc, char **argv) >> >> lines = print_client(c, engines, t, >> lines, con_w, >> - con_h, period_us, >> + con_h, scan_us, >> &class_w); >> } >> >> -- >> 2.37.2 >>
On Wed, Mar 15, 2023 at 09:20:49AM +0000, Tvrtko Ursulin wrote: > >Hi Umesh, > >On 14/03/2023 18:25, Umesh Nerlige Ramappa wrote: >>lgtm, >> >>Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > >Thanks - I had one second thought though. See below please. > >>On Tue, Mar 14, 2023 at 12:17:40PM +0000, Tvrtko Ursulin wrote: >>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>>On a slow machine, or with many processes and/or file descriptors to >>>parse, the period of the scanning loop can drift significantly from the >>>assumed value. This results in artificially inflated client busyness >>>percentages. >>> >>>To alleviate the issue take some real timestamps and use actual elapsed >>>time when calculating relative busyness. >>> >>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>--- >>>tools/intel_gpu_top.c | 39 ++++++++++++++++++++++++++++++++++++++- >>>1 file changed, 38 insertions(+), 1 deletion(-) >>> >>>diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c >>>index e13e35b71f4b..af4b350da8e4 100644 >>>--- a/tools/intel_gpu_top.c >>>+++ b/tools/intel_gpu_top.c >>>@@ -43,6 +43,7 @@ >>>#include <sys/types.h> >>>#include <unistd.h> >>>#include <termios.h> >>>+#include <time.h> >>>#include <sys/sysmacros.h> >>> >>>#include "igt_perf.h" >>>@@ -2524,6 +2525,38 @@ static void show_help_screen(void) >>>"\n"); >>>} >>> >>>+static int gettime(struct timespec *ts) >>>+{ >>>+ memset(ts, 0, sizeof(*ts)); >>>+ >>>+#ifdef CLOCK_MONOTONIC_RAW >>>+ if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts)) >>>+ return 0; >>>+#endif >>>+#ifdef CLOCK_MONOTONIC_COARSE >>>+ if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts)) >>>+ return 0; >>>+#endif > >So I copied this (with some edits) from igt_core.c but I think I >should actually remove the CLOCK_MONOTONIC_COARSE option. The usage in >intel_gpu_top is not performance sensitive and tick granularity >actually defeats to point of this patch. > >Okay to keep the r-b if I remove it? Sure, okay to keep the R-b. Regards, Umesh > >Regards, > >Tvrtko > >>>+ >>>+ return clock_gettime(CLOCK_MONOTONIC, ts); >>>+} >>>+ >>>+static unsigned long elapsed_us(struct timespec *prev, unsigned >>>int period_us) >>>+{ >>>+ unsigned long elapsed; >>>+ struct timespec now; >>>+ >>>+ if (gettime(&now)) >>>+ return period_us; >>>+ >>>+ elapsed = ((now.tv_nsec - prev->tv_nsec) / 1000 + >>>+ (unsigned long)USEC_PER_SEC * (now.tv_sec - >>>prev->tv_sec)); >>>+ >>>+ *prev = now; >>>+ >>>+ return elapsed; >>>+} >>>+ >>>int main(int argc, char **argv) >>>{ >>> unsigned int period_us = DEFAULT_PERIOD_MS * 1000; >>>@@ -2537,6 +2570,7 @@ int main(int argc, char **argv) >>> char *pmu_device, *opt_device = NULL; >>> struct igt_device_card card; >>> char *codename = NULL; >>>+ struct timespec ts; >>> >>> /* Parse options */ >>> while ((ch = getopt(argc, argv, "o:s:d:pcJLlh")) != -1) { >>>@@ -2690,6 +2724,7 @@ int main(int argc, char **argv) >>> >>> pmu_sample(engines); >>> scan_clients(clients, false); >>>+ gettime(&ts); >>> codename = igt_device_get_pretty_name(&card, false); >>> >>> if (output_mode == JSON) >>>@@ -2698,6 +2733,7 @@ int main(int argc, char **argv) >>> while (!stop_top) { >>> struct clients *disp_clients; >>> bool consumed = false; >>>+ unsigned int scan_us; >>> int j, lines = 0; >>> struct winsize ws; >>> struct client *c; >>>@@ -2720,6 +2756,7 @@ int main(int argc, char **argv) >>> t = (double)(engines->ts.cur - engines->ts.prev) / 1e9; >>> >>> disp_clients = scan_clients(clients, true); >>>+ scan_us = elapsed_us(&ts, period_us); >>> >>> if (stop_top) >>> break; >>>@@ -2757,7 +2794,7 @@ int main(int argc, char **argv) >>> >>> lines = print_client(c, engines, t, >>> lines, con_w, >>>- con_h, period_us, >>>+ con_h, scan_us, >>> &class_w); >>> } >>> >>>-- >>>2.37.2 >>>
On 15/03/2023 19:56, Umesh Nerlige Ramappa wrote: > On Wed, Mar 15, 2023 at 09:20:49AM +0000, Tvrtko Ursulin wrote: >> >> Hi Umesh, >> >> On 14/03/2023 18:25, Umesh Nerlige Ramappa wrote: >>> lgtm, >>> >>> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> >> Thanks - I had one second thought though. See below please. >> >>> On Tue, Mar 14, 2023 at 12:17:40PM +0000, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> On a slow machine, or with many processes and/or file descriptors to >>>> parse, the period of the scanning loop can drift significantly from the >>>> assumed value. This results in artificially inflated client busyness >>>> percentages. >>>> >>>> To alleviate the issue take some real timestamps and use actual elapsed >>>> time when calculating relative busyness. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> --- >>>> tools/intel_gpu_top.c | 39 ++++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 38 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c >>>> index e13e35b71f4b..af4b350da8e4 100644 >>>> --- a/tools/intel_gpu_top.c >>>> +++ b/tools/intel_gpu_top.c >>>> @@ -43,6 +43,7 @@ >>>> #include <sys/types.h> >>>> #include <unistd.h> >>>> #include <termios.h> >>>> +#include <time.h> >>>> #include <sys/sysmacros.h> >>>> >>>> #include "igt_perf.h" >>>> @@ -2524,6 +2525,38 @@ static void show_help_screen(void) >>>> "\n"); >>>> } >>>> >>>> +static int gettime(struct timespec *ts) >>>> +{ >>>> + memset(ts, 0, sizeof(*ts)); >>>> + >>>> +#ifdef CLOCK_MONOTONIC_RAW >>>> + if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts)) >>>> + return 0; >>>> +#endif >>>> +#ifdef CLOCK_MONOTONIC_COARSE >>>> + if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts)) >>>> + return 0; >>>> +#endif >> >> So I copied this (with some edits) from igt_core.c but I think I >> should actually remove the CLOCK_MONOTONIC_COARSE option. The usage in >> intel_gpu_top is not performance sensitive and tick granularity >> actually defeats to point of this patch. >> >> Okay to keep the r-b if I remove it? > > Sure, okay to keep the R-b. Thanks, pushed! Regards, Tvrtko
diff --git a/tools/intel_gpu_top.c b/tools/intel_gpu_top.c index e13e35b71f4b..af4b350da8e4 100644 --- a/tools/intel_gpu_top.c +++ b/tools/intel_gpu_top.c @@ -43,6 +43,7 @@ #include <sys/types.h> #include <unistd.h> #include <termios.h> +#include <time.h> #include <sys/sysmacros.h> #include "igt_perf.h" @@ -2524,6 +2525,38 @@ static void show_help_screen(void) "\n"); } +static int gettime(struct timespec *ts) +{ + memset(ts, 0, sizeof(*ts)); + +#ifdef CLOCK_MONOTONIC_RAW + if (!clock_gettime(CLOCK_MONOTONIC_RAW, ts)) + return 0; +#endif +#ifdef CLOCK_MONOTONIC_COARSE + if (!clock_gettime(CLOCK_MONOTONIC_COARSE, ts)) + return 0; +#endif + + return clock_gettime(CLOCK_MONOTONIC, ts); +} + +static unsigned long elapsed_us(struct timespec *prev, unsigned int period_us) +{ + unsigned long elapsed; + struct timespec now; + + if (gettime(&now)) + return period_us; + + elapsed = ((now.tv_nsec - prev->tv_nsec) / 1000 + + (unsigned long)USEC_PER_SEC * (now.tv_sec - prev->tv_sec)); + + *prev = now; + + return elapsed; +} + int main(int argc, char **argv) { unsigned int period_us = DEFAULT_PERIOD_MS * 1000; @@ -2537,6 +2570,7 @@ int main(int argc, char **argv) char *pmu_device, *opt_device = NULL; struct igt_device_card card; char *codename = NULL; + struct timespec ts; /* Parse options */ while ((ch = getopt(argc, argv, "o:s:d:pcJLlh")) != -1) { @@ -2690,6 +2724,7 @@ int main(int argc, char **argv) pmu_sample(engines); scan_clients(clients, false); + gettime(&ts); codename = igt_device_get_pretty_name(&card, false); if (output_mode == JSON) @@ -2698,6 +2733,7 @@ int main(int argc, char **argv) while (!stop_top) { struct clients *disp_clients; bool consumed = false; + unsigned int scan_us; int j, lines = 0; struct winsize ws; struct client *c; @@ -2720,6 +2756,7 @@ int main(int argc, char **argv) t = (double)(engines->ts.cur - engines->ts.prev) / 1e9; disp_clients = scan_clients(clients, true); + scan_us = elapsed_us(&ts, period_us); if (stop_top) break; @@ -2757,7 +2794,7 @@ int main(int argc, char **argv) lines = print_client(c, engines, t, lines, con_w, - con_h, period_us, + con_h, scan_us, &class_w); }