[Qemu-devel] [PATCH] block: do not use get_clock()

[Qemu-devel] [PATCH] block: do not use get_clock()

[Paolo Bonzini]

Use the external qemu-timer API instead.

Cc: kwolf@redhat.com
Cc: stefanha@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/accounting.c | 6 ++++--
 block/raw-posix.c  | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index edbb1cc..18102f0 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -24,6 +24,7 @@
 
 #include "block/accounting.h"
 #include "block/block_int.h"
+#include "qemu/timer.h"
 
 void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
                       int64_t bytes, enum BlockAcctType type)
@@ -31,7 +32,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
     assert(type < BLOCK_MAX_IOTYPE);
 
     cookie->bytes = bytes;
-    cookie->start_time_ns = get_clock();
+    cookie->start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     cookie->type = type;
 }
 
@@ -41,7 +42,8 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
 
     stats->nr_bytes[cookie->type] += cookie->bytes;
     stats->nr_ops[cookie->type]++;
-    stats->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
+    stats->total_time_ns[cookie->type] +=
+        qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - cookie->start_time_ns;
 }
 
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index b1af77e..02e107f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1922,7 +1922,7 @@ static int fd_open(BlockDriverState *bs)
         return 0;
     last_media_present = (s->fd >= 0);
     if (s->fd >= 0 &&
-        (get_clock() - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
+        (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
         qemu_close(s->fd);
         s->fd = -1;
 #ifdef DEBUG_FLOPPY
@@ -1931,7 +1931,7 @@ static int fd_open(BlockDriverState *bs)
     }
     if (s->fd < 0) {
         if (s->fd_got_error &&
-            (get_clock() - s->fd_error_time) < FD_OPEN_TIMEOUT) {
+            (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_error_time) < FD_OPEN_TIMEOUT) {
 #ifdef DEBUG_FLOPPY
             printf("No floppy (open delayed)\n");
 #endif
@@ -1939,7 +1939,7 @@ static int fd_open(BlockDriverState *bs)
         }
         s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
         if (s->fd < 0) {
-            s->fd_error_time = get_clock();
+            s->fd_error_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
             s->fd_got_error = 1;
             if (last_media_present)
                 s->fd_media_changed = 1;
@@ -1954,7 +1954,7 @@ static int fd_open(BlockDriverState *bs)
     }
     if (!last_media_present)
         s->fd_media_changed = 1;
-    s->fd_open_time = get_clock();
+    s->fd_open_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     s->fd_got_error = 0;
     return 0;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Use the external qemu-timer API instead.

Ignorant question: why?

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Paolo Bonzini]

On 27/11/2014 10:19, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Use the external qemu-timer API instead.
> 
> Ignorant question: why?

Because no one else calls it directly, it is an internal function.  I
want to keep it confined to qemu-timer.c (and possibly cpus.c in the
icount implementation, but maybe not even that is necessary).

Paolo

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

On Thu, Nov 27, 2014 at 10:19:45AM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Use the external qemu-timer API instead.
> 
> Ignorant question: why?

Patch seems fine but I concur with Markus.  Let's add the rationale to
the commit description.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Paolo Bonzini]

On 27/11/2014 17:44, Stefan Hajnoczi wrote:
> On Thu, Nov 27, 2014 at 10:19:45AM +0100, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Use the external qemu-timer API instead.
>>
>> Ignorant question: why?
> 
> Patch seems fine but I concur with Markus.  Let's add the rationale to
> the commit description.

Like this?

---------------- 8< ----------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] block: do not use get_clock()

Use the external qemu-timer API instead.

No one else should be calling cpu_get_clock(), get_clock() and 
get_clock_realtime() directly; they are internal functions and they 
should be confined to qemu-timer.c and cpus.c (where the icount 
implementation resides).  All accesses should go through
qemu_clock_get_ns.

Cc: kwolf@redhat.com
Cc: stefanha@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/accounting.c | 6 ++++--
 block/raw-posix.c  | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index edbb1cc..18102f0 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -24,6 +24,7 @@
 
 #include "block/accounting.h"
 #include "block/block_int.h"
+#include "qemu/timer.h"
 
 void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
                       int64_t bytes, enum BlockAcctType type)
@@ -31,7 +32,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
     assert(type < BLOCK_MAX_IOTYPE);
 
     cookie->bytes = bytes;
-    cookie->start_time_ns = get_clock();
+    cookie->start_time_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     cookie->type = type;
 }
 
@@ -41,7 +42,8 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
 
     stats->nr_bytes[cookie->type] += cookie->bytes;
     stats->nr_ops[cookie->type]++;
-    stats->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
+    stats->total_time_ns[cookie->type] +=
+        qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - cookie->start_time_ns;
 }
 
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index b1af77e..02e107f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1922,7 +1922,7 @@ static int fd_open(BlockDriverState *bs)
         return 0;
     last_media_present = (s->fd >= 0);
     if (s->fd >= 0 &&
-        (get_clock() - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
+        (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_open_time) >= FD_OPEN_TIMEOUT) {
         qemu_close(s->fd);
         s->fd = -1;
 #ifdef DEBUG_FLOPPY
@@ -1931,7 +1931,7 @@ static int fd_open(BlockDriverState *bs)
     }
     if (s->fd < 0) {
         if (s->fd_got_error &&
-            (get_clock() - s->fd_error_time) < FD_OPEN_TIMEOUT) {
+            (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->fd_error_time) < FD_OPEN_TIMEOUT) {
 #ifdef DEBUG_FLOPPY
             printf("No floppy (open delayed)\n");
 #endif
@@ -1939,7 +1939,7 @@ static int fd_open(BlockDriverState *bs)
         }
         s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
         if (s->fd < 0) {
-            s->fd_error_time = get_clock();
+            s->fd_error_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
             s->fd_got_error = 1;
             if (last_media_present)
                 s->fd_media_changed = 1;
@@ -1954,7 +1954,7 @@ static int fd_open(BlockDriverState *bs)
     }
     if (!last_media_present)
         s->fd_media_changed = 1;
-    s->fd_open_time = get_clock();
+    s->fd_open_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     s->fd_got_error = 0;
     return 0;
 }
-- 1.8.3.1 

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/11/2014 10:19, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Use the external qemu-timer API instead.
>> 
>> Ignorant question: why?
>
> Because no one else calls it directly, it is an internal function.  I
> want to keep it confined to qemu-timer.c (and possibly cpus.c in the
> icount implementation, but maybe not even that is necessary).

That's a perfectly sensible reason.  Please add it to the commit
message, so we have a more permanent record.

Of course, the best permanent record would be a static keyword :)

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Paolo Bonzini]

On 27/11/2014 18:45, Markus Armbruster wrote:
>> >
>> > Because no one else calls it directly, it is an internal function.  I
>> > want to keep it confined to qemu-timer.c (and possibly cpus.c in the
>> > icount implementation, but maybe not even that is necessary).
> That's a perfectly sensible reason.  Please add it to the commit
> message, so we have a more permanent record.
> 
> Of course, the best permanent record would be a static keyword :)

That's on the todo list. :)  I'll send v2 as a toplevel patch if I don't
hear from Kevin or Stefan in a few days.  This is 2.3 work so there's time.

Paolo

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Wed, Nov 26, 2014 at 03:01:02PM +0100, Paolo Bonzini wrote:
> Use the external qemu-timer API instead.
> 
> Cc: kwolf@redhat.com
> Cc: stefanha@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/accounting.c | 6 ++++--
>  block/raw-posix.c  | 8 ++++----
>  2 files changed, 8 insertions(+), 6 deletions(-)

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Nov 26, 2014 at 03:01:02PM +0100, Paolo Bonzini wrote:
>> Use the external qemu-timer API instead.
>> 
>> Cc: kwolf@redhat.com
>> Cc: stefanha@redhat.com
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  block/accounting.c | 6 ++++--
>>  block/raw-posix.c  | 8 ++++----
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> Thanks, applied to my block-next tree:
> https://github.com/stefanha/qemu/commits/block-next

Please wait for Paolo's v2 with rationale in the commit message.

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

On Fri, Nov 28, 2014 at 02:41:54PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Wed, Nov 26, 2014 at 03:01:02PM +0100, Paolo Bonzini wrote:
> >> Use the external qemu-timer API instead.
> >> 
> >> Cc: kwolf@redhat.com
> >> Cc: stefanha@redhat.com
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  block/accounting.c | 6 ++++--
> >>  block/raw-posix.c  | 8 ++++----
> >>  2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > Thanks, applied to my block-next tree:
> > https://github.com/stefanha/qemu/commits/block-next
> 
> Please wait for Paolo's v2 with rationale in the commit message.

Got it.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block: do not use get_clock()

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Nov 28, 2014 at 02:41:54PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>> 
>> > On Wed, Nov 26, 2014 at 03:01:02PM +0100, Paolo Bonzini wrote:
>> >> Use the external qemu-timer API instead.
>> >> 
>> >> Cc: kwolf@redhat.com
>> >> Cc: stefanha@redhat.com
>> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> >> ---
>> >>  block/accounting.c | 6 ++++--
>> >>  block/raw-posix.c  | 8 ++++----
>> >>  2 files changed, 8 insertions(+), 6 deletions(-)
>> >
>> > Thanks, applied to my block-next tree:
>> > https://github.com/stefanha/qemu/commits/block-next
>> 
>> Please wait for Paolo's v2 with rationale in the commit message.
>
> Got it.

https://github.com/stefanha/qemu/commit/1800094faabbadb017acb15bc0b1bd6dde283f45
looks good, thanks!