diff mbox series

[v2,6/8] Revert use of clock_gettime for benchmarking

Message ID AM9PR09MB485113D76C5AE02516C08E8E84402@AM9PR09MB4851.eurprd09.prod.outlook.com (mailing list archive)
State New
Headers show
Series [v2,1/8] Add CP0 MemoryMapID register implementation | expand

Commit Message

Aleksandar Rakic Oct. 18, 2024, 1:20 p.m. UTC
This patch reverts the commit (with SHA
50290c002c045280f8defad911901e16bfb52884 from
https://github.com/MIPS/gnutools-qemu) that breaks for mingw builds,
where clock_gettime and CLOCK_MONOTONIC are not available.

Cherry-picked d57c735e1af1ca719dbd0c3a904ad70c9c31cbb7
from https://github.com/MIPS/gnutools-qemu

Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
---
 qemu-io-cmds.c | 77 +++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 19, 2024, 7:35 p.m. UTC | #1
Hi,

On 18/10/24 10:20, Aleksandar Rakic wrote:
> This patch reverts the commit (with SHA
> 50290c002c045280f8defad911901e16bfb52884 from
> https://github.com/MIPS/gnutools-qemu) that breaks for mingw builds,
> where clock_gettime and CLOCK_MONOTONIC are not available.

Isn't get_clock() what we want here?

> Cherry-picked d57c735e1af1ca719dbd0c3a904ad70c9c31cbb7
> from https://github.com/MIPS/gnutools-qemu
> 
> Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
> Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
> ---
>   qemu-io-cmds.c | 77 +++++++++++++++++++++++++-------------------------
>   1 file changed, 39 insertions(+), 38 deletions(-)

Please Cc maintainers (done now):

$ ./scripts/get_maintainer.pl -f qemu-io-cmds.c
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core)

> @@ -904,7 +905,7 @@ static const cmdinfo_t readv_cmd = {
>   
>   static int readv_f(BlockBackend *blk, int argc, char **argv)
>   {
> -    struct timespec t1, t2;
> +    struct timeval t1, t2;
>       bool Cflag = false, qflag = false, vflag = false;
>       int c, cnt, ret;
>       char *buf;
> @@ -964,9 +965,9 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
>           return -EINVAL;
>       }
>   
> -    clock_gettime(CLOCK_MONOTONIC, &t1);
> +    gettimeofday(&t1, NULL);
>       ret = do_aio_readv(blk, &qiov, offset, flags, &total);
> -    clock_gettime(CLOCK_MONOTONIC, &t2);
> +    gettimeofday(&t2, NULL);
>   
>       if (ret < 0) {
>           printf("readv failed: %s\n", strerror(-ret));
Paolo Bonzini Oct. 20, 2024, 2:04 p.m. UTC | #2
On 10/18/24 15:20, Aleksandar Rakic wrote:
> This patch reverts the commit (with SHA
> 50290c002c045280f8defad911901e16bfb52884 from
> https://github.com/MIPS/gnutools-qemu) that breaks for mingw builds,
> where clock_gettime and CLOCK_MONOTONIC are not available.
> 
> Cherry-picked d57c735e1af1ca719dbd0c3a904ad70c9c31cbb7
> from https://github.com/MIPS/gnutools-qemu
> 
> Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
> Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>

Can you change it to use qemu_clock_get_ns(QEMU_CLOCK_REALTIME) instead? 
  It's the same as clock_gettime() on POSIX systems and works on Windows 
as well.

Plus, because the result is just use uint64_t, tsub and tdiv become 
simple subtraction and (double)a / b respectively; and timestr is also 
just as simple with:

-    double frac_sec = tv->tv_nsec / 1e9;
+    uint64_t sec = nsec / NANOSECONDS_PER_SECOND;
+    double frac_sec = (nsec % NANOSECONDS_PER_SECOND) / 1e9;

Paolo
Kevin Wolf Oct. 21, 2024, 11:42 a.m. UTC | #3
Am 19.10.2024 um 21:35 hat Philippe Mathieu-Daudé geschrieben:
> Hi,
> 
> On 18/10/24 10:20, Aleksandar Rakic wrote:
> > This patch reverts the commit (with SHA
> > 50290c002c045280f8defad911901e16bfb52884 from
> > https://github.com/MIPS/gnutools-qemu) that breaks for mingw builds,
> > where clock_gettime and CLOCK_MONOTONIC are not available.

What does "not available" mean? I'm sure that we have kept building QEMU
with mingw in the past five years (the commit you want to revert is from
2019), so they must exist in some form?

> Isn't get_clock() what we want here?
> 
> > Cherry-picked d57c735e1af1ca719dbd0c3a904ad70c9c31cbb7
> > from https://github.com/MIPS/gnutools-qemu
> > 
> > Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
> > Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
> > ---
> >   qemu-io-cmds.c | 77 +++++++++++++++++++++++++-------------------------
> >   1 file changed, 39 insertions(+), 38 deletions(-)
> 
> Please Cc maintainers (done now):
> 
> $ ./scripts/get_maintainer.pl -f qemu-io-cmds.c
> Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
> Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core)

Also Alex (CCed) who is the author of the patch you want to revert.

Kevin

> > @@ -904,7 +905,7 @@ static const cmdinfo_t readv_cmd = {
> >   static int readv_f(BlockBackend *blk, int argc, char **argv)
> >   {
> > -    struct timespec t1, t2;
> > +    struct timeval t1, t2;
> >       bool Cflag = false, qflag = false, vflag = false;
> >       int c, cnt, ret;
> >       char *buf;
> > @@ -964,9 +965,9 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
> >           return -EINVAL;
> >       }
> > -    clock_gettime(CLOCK_MONOTONIC, &t1);
> > +    gettimeofday(&t1, NULL);
> >       ret = do_aio_readv(blk, &qiov, offset, flags, &total);
> > -    clock_gettime(CLOCK_MONOTONIC, &t2);
> > +    gettimeofday(&t2, NULL);
> >       if (ret < 0) {
> >           printf("readv failed: %s\n", strerror(-ret));
>
Daniel P. Berrangé Oct. 21, 2024, 11:48 a.m. UTC | #4
On Mon, Oct 21, 2024 at 01:42:29PM +0200, Kevin Wolf wrote:
> Am 19.10.2024 um 21:35 hat Philippe Mathieu-Daudé geschrieben:
> > Hi,
> > 
> > On 18/10/24 10:20, Aleksandar Rakic wrote:
> > > This patch reverts the commit (with SHA
> > > 50290c002c045280f8defad911901e16bfb52884 from
> > > https://github.com/MIPS/gnutools-qemu) that breaks for mingw builds,
> > > where clock_gettime and CLOCK_MONOTONIC are not available.
> 
> What does "not available" mean? I'm sure that we have kept building QEMU
> with mingw in the past five years (the commit you want to revert is from
> 2019), so they must exist in some form?

They exist in the mingw headers

$ grep clock_gettime /usr/i686-w64-mingw32/sys-root/mingw/include/pthread_time.h
int __cdecl WINPTHREAD_API clock_gettime(clockid_t clock_id, struct timespec *tp);

$ grep CLOCK_MONOTONIC /usr/i686-w64-mingw32/sys-root/mingw/include/pthread_time.h
#ifndef CLOCK_MONOTONIC
#define CLOCK_MONOTONIC             1

So I think we need more clarity about what's broken before we can
consider merging this.

> 
> > Isn't get_clock() what we want here?
> > 
> > > Cherry-picked d57c735e1af1ca719dbd0c3a904ad70c9c31cbb7
> > > from https://github.com/MIPS/gnutools-qemu
> > > 
> > > Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
> > > Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
> > > ---
> > >   qemu-io-cmds.c | 77 +++++++++++++++++++++++++-------------------------
> > >   1 file changed, 39 insertions(+), 38 deletions(-)
> > 
> > Please Cc maintainers (done now):
> > 
> > $ ./scripts/get_maintainer.pl -f qemu-io-cmds.c
> > Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
> > Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core)
> 
> Also Alex (CCed) who is the author of the patch you want to revert.
> 
> Kevin
> 
> > > @@ -904,7 +905,7 @@ static const cmdinfo_t readv_cmd = {
> > >   static int readv_f(BlockBackend *blk, int argc, char **argv)
> > >   {
> > > -    struct timespec t1, t2;
> > > +    struct timeval t1, t2;
> > >       bool Cflag = false, qflag = false, vflag = false;
> > >       int c, cnt, ret;
> > >       char *buf;
> > > @@ -964,9 +965,9 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
> > >           return -EINVAL;
> > >       }
> > > -    clock_gettime(CLOCK_MONOTONIC, &t1);
> > > +    gettimeofday(&t1, NULL);
> > >       ret = do_aio_readv(blk, &qiov, offset, flags, &total);
> > > -    clock_gettime(CLOCK_MONOTONIC, &t2);
> > > +    gettimeofday(&t2, NULL);
> > >       if (ret < 0) {
> > >           printf("readv failed: %s\n", strerror(-ret));
> > 
> 
> 

With regards,
Daniel
diff mbox series

Patch

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index e2fab57183..a846746553 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -259,21 +259,20 @@  static void cvtstr(double value, char *str, size_t size)
 
 
 
-static struct timespec tsub(struct timespec t1, struct timespec t2)
+static struct timeval tsub(struct timeval t1, struct timeval t2)
 {
-    t1.tv_nsec -= t2.tv_nsec;
-    if (t1.tv_nsec < 0) {
-        t1.tv_nsec += NANOSECONDS_PER_SECOND;
+    t1.tv_usec -= t2.tv_usec;
+    if (t1.tv_usec < 0) {
+        t1.tv_usec += 1000000;
         t1.tv_sec--;
     }
     t1.tv_sec -= t2.tv_sec;
     return t1;
 }
 
-static double tdiv(double value, struct timespec tv)
+static double tdiv(double value, struct timeval tv)
 {
-    double seconds = tv.tv_sec + (tv.tv_nsec / 1e9);
-    return value / seconds;
+    return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
 }
 
 #define HOURS(sec)      ((sec) / (60 * 60))
@@ -286,27 +285,29 @@  enum {
     VERBOSE_FIXED_TIME  = 0x2,
 };
 
-static void timestr(struct timespec *tv, char *ts, size_t size, int format)
+static void timestr(struct timeval *tv, char *ts, size_t size, int format)
 {
-    double frac_sec = tv->tv_nsec / 1e9;
+    double usec = (double)tv->tv_usec / 1000000.0;
 
     if (format & TERSE_FIXED_TIME) {
         if (!HOURS(tv->tv_sec)) {
-            snprintf(ts, size, "%u:%05.2f",
-                     (unsigned int) MINUTES(tv->tv_sec),
-                     SECONDS(tv->tv_sec) + frac_sec);
+            snprintf(ts, size, "%u:%02u.%02u",
+                    (unsigned int) MINUTES(tv->tv_sec),
+                    (unsigned int) SECONDS(tv->tv_sec),
+                    (unsigned int) (usec * 100));
             return;
         }
         format |= VERBOSE_FIXED_TIME; /* fallback if hours needed */
     }
 
     if ((format & VERBOSE_FIXED_TIME) || tv->tv_sec) {
-        snprintf(ts, size, "%u:%02u:%05.2f",
+        snprintf(ts, size, "%u:%02u:%02u.%02u",
                 (unsigned int) HOURS(tv->tv_sec),
                 (unsigned int) MINUTES(tv->tv_sec),
-                 SECONDS(tv->tv_sec) + frac_sec);
+                (unsigned int) SECONDS(tv->tv_sec),
+                (unsigned int) (usec * 100));
     } else {
-        snprintf(ts, size, "%05.2f sec", frac_sec);
+        snprintf(ts, size, "0.%04u sec", (unsigned int) (usec * 10000));
     }
 }
 
@@ -467,7 +468,7 @@  static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
     }
 }
 
-static void print_report(const char *op, struct timespec *t, int64_t offset,
+static void print_report(const char *op, struct timeval *t, int64_t offset,
                          int64_t count, int64_t total, int cnt, bool Cflag)
 {
     char s1[64], s2[64], ts[64];
@@ -707,7 +708,7 @@  static const cmdinfo_t read_cmd = {
 
 static int read_f(BlockBackend *blk, int argc, char **argv)
 {
-    struct timespec t1, t2;
+    struct timeval t1, t2;
     bool Cflag = false, qflag = false, vflag = false;
     bool Pflag = false, sflag = false, lflag = false, bflag = false;
     int c, cnt, ret;
@@ -825,13 +826,13 @@  static int read_f(BlockBackend *blk, int argc, char **argv)
 
     buf = qemu_io_alloc(blk, count, 0xab, flags & BDRV_REQ_REGISTERED_BUF);
 
-    clock_gettime(CLOCK_MONOTONIC, &t1);
+    gettimeofday(&t1, NULL);
     if (bflag) {
         ret = do_load_vmstate(blk, buf, offset, count, &total);
     } else {
         ret = do_pread(blk, buf, offset, count, flags, &total);
     }
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
     if (ret < 0) {
         printf("read failed: %s\n", strerror(-ret));
@@ -904,7 +905,7 @@  static const cmdinfo_t readv_cmd = {
 
 static int readv_f(BlockBackend *blk, int argc, char **argv)
 {
-    struct timespec t1, t2;
+    struct timeval t1, t2;
     bool Cflag = false, qflag = false, vflag = false;
     int c, cnt, ret;
     char *buf;
@@ -964,9 +965,9 @@  static int readv_f(BlockBackend *blk, int argc, char **argv)
         return -EINVAL;
     }
 
-    clock_gettime(CLOCK_MONOTONIC, &t1);
+    gettimeofday(&t1, NULL);
     ret = do_aio_readv(blk, &qiov, offset, flags, &total);
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
     if (ret < 0) {
         printf("readv failed: %s\n", strerror(-ret));
@@ -1047,7 +1048,7 @@  static const cmdinfo_t write_cmd = {
 
 static int write_f(BlockBackend *blk, int argc, char **argv)
 {
-    struct timespec t1, t2;
+    struct timeval t1, t2;
     bool Cflag = false, qflag = false, bflag = false;
     bool Pflag = false, zflag = false, cflag = false, sflag = false;
     BdrvRequestFlags flags = 0;
@@ -1190,7 +1191,7 @@  static int write_f(BlockBackend *blk, int argc, char **argv)
         }
     }
 
-    clock_gettime(CLOCK_MONOTONIC, &t1);
+    gettimeofday(&t1, NULL);
     if (bflag) {
         ret = do_save_vmstate(blk, buf, offset, count, &total);
     } else if (zflag) {
@@ -1200,7 +1201,7 @@  static int write_f(BlockBackend *blk, int argc, char **argv)
     } else {
         ret = do_pwrite(blk, buf, offset, count, flags, &total);
     }
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
     if (ret < 0) {
         printf("write failed: %s\n", strerror(-ret));
@@ -1260,7 +1261,7 @@  static const cmdinfo_t writev_cmd = {
 
 static int writev_f(BlockBackend *blk, int argc, char **argv)
 {
-    struct timespec t1, t2;
+    struct timeval t1, t2;
     bool Cflag = false, qflag = false;
     BdrvRequestFlags flags = 0;
     int c, cnt, ret;
@@ -1317,9 +1318,9 @@  static int writev_f(BlockBackend *blk, int argc, char **argv)
         return -EINVAL;
     }
 
-    clock_gettime(CLOCK_MONOTONIC, &t1);
+    gettimeofday(&t1, NULL);
     ret = do_aio_writev(blk, &qiov, offset, flags, &total);
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
     if (ret < 0) {
         printf("writev failed: %s\n", strerror(-ret));
@@ -1355,15 +1356,15 @@  struct aio_ctx {
     BlockAcctCookie acct;
     int pattern;
     BdrvRequestFlags flags;
-    struct timespec t1;
+    struct timeval t1;
 };
 
 static void aio_write_done(void *opaque, int ret)
 {
     struct aio_ctx *ctx = opaque;
-    struct timespec t2;
+    struct timeval t2;
 
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
 
     if (ret < 0) {
@@ -1394,9 +1395,9 @@  out:
 static void aio_read_done(void *opaque, int ret)
 {
     struct aio_ctx *ctx = opaque;
-    struct timespec t2;
+    struct timeval t2;
 
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
     if (ret < 0) {
         printf("readv failed: %s\n", strerror(-ret));
@@ -1537,7 +1538,7 @@  static int aio_read_f(BlockBackend *blk, int argc, char **argv)
         return -EINVAL;
     }
 
-    clock_gettime(CLOCK_MONOTONIC, &ctx->t1);
+    gettimeofday(&ctx->t1, NULL);
     block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size,
                      BLOCK_ACCT_READ);
     blk_aio_preadv(blk, ctx->offset, &ctx->qiov, ctx->flags, aio_read_done,
@@ -1692,7 +1693,7 @@  static int aio_write_f(BlockBackend *blk, int argc, char **argv)
             return -EINVAL;
         }
 
-        clock_gettime(CLOCK_MONOTONIC, &ctx->t1);
+        gettimeofday(&ctx->t1, NULL);
         block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size,
                          BLOCK_ACCT_WRITE);
 
@@ -2159,7 +2160,7 @@  static const cmdinfo_t discard_cmd = {
 
 static int discard_f(BlockBackend *blk, int argc, char **argv)
 {
-    struct timespec t1, t2;
+    struct timeval t1, t2;
     bool Cflag = false, qflag = false;
     int c, ret;
     int64_t offset, bytes;
@@ -2200,9 +2201,9 @@  static int discard_f(BlockBackend *blk, int argc, char **argv)
         return -EINVAL;
     }
 
-    clock_gettime(CLOCK_MONOTONIC, &t1);
+    gettimeofday(&t1, NULL);
     ret = blk_pdiscard(blk, offset, bytes);
-    clock_gettime(CLOCK_MONOTONIC, &t2);
+    gettimeofday(&t2, NULL);
 
     if (ret < 0) {
         printf("discard failed: %s\n", strerror(-ret));