[Qemu-devel] [PATCH] blockjob: Don't sleep too short

[Qemu-devel] [PATCH] blockjob: Don't sleep too short

[Fam Zheng]

block_job_sleep_ns is called by block job coroutines to yield the
execution to VCPU threads and monitor etc. It is pointless to sleep for
0 or a few nanoseconds, because that equals to a "yield + enter" with no
intermission in between (the timer fires immediately in the same
iteration of event loop), which means other code still doesn't get a
fair share of main loop / BQL.

Trim the sleep duration with a minimum value.

Reported-by: Alexandre DERUMIER <aderumier@odiso.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockjob.c               | 2 ++
 include/block/blockjob.h | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index ec46fad..b17ed1f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -238,6 +238,8 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
         return;
     }
 
+    ns = MAX(ns, BLOCK_JOB_SLEEP_NS_MIN);
+
     job->busy = false;
     if (block_job_is_paused(job)) {
         qemu_coroutine_yield();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 57d8ef1..3deb731 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -146,11 +146,13 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp);
 
+#define BLOCK_JOB_SLEEP_NS_MIN 10000000L
 /**
  * block_job_sleep_ns:
  * @job: The job that calls the function.
  * @clock: The clock to sleep on.
- * @ns: How many nanoseconds to stop for.
+ * @ns: How many nanoseconds to stop for. It sleeps at least
+ * for BLOCK_JOB_SLEEP_NS_MIN ns, even if a smaller value is specified.
  *
  * Put the job to sleep (assuming that it wasn't canceled) for @ns
  * nanoseconds.  Canceling the job will interrupt the wait immediately.
-- 
2.4.3

Re: [Qemu-devel] [PATCH] blockjob: Don't sleep too short

[Alexandre DERUMIER]

Works fine here,

Thanks !

----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "qemu-devel" <qemu-devel@nongnu.org>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Jeff Cody" <jcody@redhat.com>, qemu-block@nongnu.org, mreitz@redhat.com, jsnow@redhat.com, "aderumier" <aderumier@odiso.com>, "stefanha" <stefanha@redhat.com>, "pbonzini" <pbonzini@redhat.com>
Envoyé: Lundi 6 Juillet 2015 05:28:11
Objet: [PATCH] blockjob: Don't sleep too short

block_job_sleep_ns is called by block job coroutines to yield the 
execution to VCPU threads and monitor etc. It is pointless to sleep for 
0 or a few nanoseconds, because that equals to a "yield + enter" with no 
intermission in between (the timer fires immediately in the same 
iteration of event loop), which means other code still doesn't get a 
fair share of main loop / BQL. 

Trim the sleep duration with a minimum value. 

Reported-by: Alexandre DERUMIER <aderumier@odiso.com> 
Signed-off-by: Fam Zheng <famz@redhat.com> 
--- 
blockjob.c | 2 ++ 
include/block/blockjob.h | 4 +++- 
2 files changed, 5 insertions(+), 1 deletion(-) 

diff --git a/blockjob.c b/blockjob.c 
index ec46fad..b17ed1f 100644 
--- a/blockjob.c 
+++ b/blockjob.c 
@@ -238,6 +238,8 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns) 
return; 
} 

+ ns = MAX(ns, BLOCK_JOB_SLEEP_NS_MIN); 
+ 
job->busy = false; 
if (block_job_is_paused(job)) { 
qemu_coroutine_yield(); 
diff --git a/include/block/blockjob.h b/include/block/blockjob.h 
index 57d8ef1..3deb731 100644 
--- a/include/block/blockjob.h 
+++ b/include/block/blockjob.h 
@@ -146,11 +146,13 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, 
int64_t speed, BlockCompletionFunc *cb, 
void *opaque, Error **errp); 

+#define BLOCK_JOB_SLEEP_NS_MIN 10000000L 
/** 
* block_job_sleep_ns: 
* @job: The job that calls the function. 
* @clock: The clock to sleep on. 
- * @ns: How many nanoseconds to stop for. 
+ * @ns: How many nanoseconds to stop for. It sleeps at least 
+ * for BLOCK_JOB_SLEEP_NS_MIN ns, even if a smaller value is specified. 
* 
* Put the job to sleep (assuming that it wasn't canceled) for @ns 
* nanoseconds. Canceling the job will interrupt the wait immediately. 
-- 
2.4.3 

Re: [Qemu-devel] [Qemu-block] [PATCH] blockjob: Don't sleep too short

[Stefan Hajnoczi]

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

On Mon, Jul 06, 2015 at 11:28:11AM +0800, Fam Zheng wrote:
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 57d8ef1..3deb731 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -146,11 +146,13 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>                         int64_t speed, BlockCompletionFunc *cb,
>                         void *opaque, Error **errp);
>  
> +#define BLOCK_JOB_SLEEP_NS_MIN 10000000L

Please introduce a block_job_relax_cpu() or similar function instead of
changing block_job_sleep_ns() to 10 millisecond minimum.  This change
would make legitimate <10 ms users imprecise!

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