Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
δΊ 2013-8-10 19:05, Alex Bligh ει: Paolo, On 9 Aug 2013, at 15:59, Paolo Bonzini wrote: It's not papering over anything. Timers right now are provided by the event loop. If you make AioContexts have timers, you can have a new AioContext for the timers that the event loop handles before your patches. It's not related to having two nested event loops. The nested event loops have the problem that timers do not run in them, but it's also a feature---because you know exactly what code runs in the nested event loop and what doesn't. Using an entirely distinct event loop preserves the feature. I've submitted a first cut as a separate patch just to see whether it works: http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg01412.html It passes 'make check' anyway. I didn't wrap TimerListGroup into a separate struct for reasons I'll set out below. I think there are two ways to go here: 1. Wrap TimerListGroup into a separate struct, leave all the TimerListGroup functions in. This probably makes it easier if (for instance) we decided to get rid of AioContexts entirely and make them g_source subclasses (per Wenchao Xia). I have a quick view of this series, but not very carefully since it is quite long.:) I have replied with Paolo's comments on my RFC thread. Personally I like g_source sub class, which make code clear, but let's discuss first if the direction is right in that mail. 2. Remove the TimerListGroup thing entirely and just have an array of TimerLists of size QEMU_CLOCK_MAX. I can leave the things that iterate that array inside qemu_timer.c (which I'd rather do for encapsulation). Part of the problem with the old timer code was that there were (in my opinion) far too many files that were working out what to iterate, and I really don't want to reintroduce that. Despite the fact we both dislike the name TimerListGroup, I think the way to go here is (1). (2) does not really save lines of code (certainly not compiled instructions) - it's main saving is removing a pile of commenting from include/qemu/timer.h, which makes things more opaque. I also think there may well be a use for something that wants to use timers but not AioContext (I am thinking for instance of a thread that does not do block IO). This permits that, but does not require it. WDYT? -- Best Regards Wenchao Xia
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
--On 12 August 2013 14:53:06 +0800 Wenchao Xia xiaw...@linux.vnet.ibm.com wrote: 1. Wrap TimerListGroup into a separate struct, leave all the TimerListGroup functions in. This probably makes it easier if (for instance) we decided to get rid of AioContexts entirely and make them g_source subclasses (per Wenchao Xia). I have a quick view of this series, but not very carefully since it is quite long.:) I have replied with Paolo's comments on my RFC thread. Personally I like g_source sub class, which make code clear, but let's discuss first if the direction is right in that mail. Looks like Paolo is happy going with TimerListGroup for now. So I wrapped the array in a struct for v10. -- Alex Bligh
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 11/08/2013 10:29, Alex Bligh ha scritto: Paolo, --On 11 August 2013 09:53:38 +0200 Paolo Bonzini pbonz...@redhat.com wrote: There is actually a disadvantage of moving TimerListGroup to AioContext. The disadvantage is that GSources can only work with millisecond resolution. Thus you would need anyway some special casing of the timer AioContext to get the deadline in nanoseconds. We also need to special case the notifier as it needs to qemu_notify() rather than aio_notify(). No, not really, because qemu_notify_event() is exactly the same aio_notify(main_loop_context). You do not need to special case the notifier even if TimerListGroup remains separate. So let's keep the TimerListGroup for now. OK - do you want me to wrap it in a struct? Other than that I think I've done all the comments in v8. Happy to do that with v10 if there are other comments on v9. No, it's okay. Unless you want to remove the callback and use aio_notify everywhere. I note no one has yet commented on the changes to the icount stuff where a timeout is apparently arbitrarily capped at 2^31 ns (about 2.1 seconds) in PATCHv9 19/31 - attached below. That's the area I'm worried about as I'm not sure I understood the code. Yes, that's ok. I'm more worried about the thread safety, but it's not a problem yet. Paolo
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
On 12 Aug 2013, at 18:27, Paolo Bonzini wrote: So let's keep the TimerListGroup for now. OK - do you want me to wrap it in a struct? Other than that I think I've done all the comments in v8. Happy to do that with v10 if there are other comments on v9. No, it's okay. Unless you want to remove the callback and use aio_notify everywhere. I wrapped it anyway in v10. No harm done. -- Alex Bligh
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 10/08/2013 13:05, Alex Bligh ha scritto: Despite the fact we both dislike the name TimerListGroup, I think the way to go here is (1). (2) does not really save lines of code (certainly not compiled instructions) - it's main saving is removing a pile of commenting from include/qemu/timer.h, which makes things more opaque. I also think there may well be a use for something that wants to use timers but not AioContext (I am thinking for instance of a thread that does not do block IO). This permits that, but does not require it. There is actually a disadvantage of moving TimerListGroup to AioContext. The disadvantage is that GSources can only work with millisecond resolution. Thus you would need anyway some special casing of the timer AioContext to get the deadline in nanoseconds. So let's keep the TimerListGroup for now. Paolo WDYT?
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Paolo,
--On 11 August 2013 09:53:38 +0200 Paolo Bonzini pbonz...@redhat.com
wrote:
There is actually a disadvantage of moving TimerListGroup to AioContext.
The disadvantage is that GSources can only work with millisecond
resolution. Thus you would need anyway some special casing of the
timer AioContext to get the deadline in nanoseconds.
We also need to special case the notifier as it needs to qemu_notify()
rather than aio_notify().
So let's keep the TimerListGroup for now.
OK - do you want me to wrap it in a struct? Other than that I think I've
done all the comments in v8. Happy to do that with v10 if there are
other comments on v9.
I note no one has yet commented on the changes to the icount stuff
where a timeout is apparently arbitrarily capped at 2^31 ns (about
2.1 seconds) in PATCHv9 19/31 - attached below. That's the area
I'm worried about as I'm not sure I understood the code.
--
Alex Bligh
-- Forwarded Message --
Subject: [RFC] [PATCHv9 19/31] aio / timers: Use all timerlists in icount
warp calculations
...
For compatibility, maintain an apparent bug where when using
icount, if no vm_clock timer was set, qemu_clock_deadline
would return INT32_MAX and always set an icount clock expiry
about 2 seconds ahead.
...
diff --git a/cpus.c b/cpus.c
index 0f65e76..673d506 100644
--- a/cpus.c
+++ b/cpus.c
...
@@ -314,7 +314,18 @@ void qemu_clock_warp(QEMUClock *clock)
}
vm_clock_warp_start = qemu_get_clock_ns(rt_clock);
-deadline = qemu_clock_deadline(vm_clock);
+/* We want to use the earliest deadline from ALL vm_clocks */
+deadline = qemu_clock_deadline_ns_all(vm_clock);
+
+/* Maintain prior (possibly buggy) behaviour where if no deadline
+ * was set (as there is no vm_clock timer) or it is more than
+ * INT32_MAX nanoseconds ahead, we still use INT32_MAX
+ * nanoseconds.
+ */
+if ((deadline 0) || (deadline INT32_MAX)) {
+deadline = INT32_MAX; / THIS
+}
+
if (deadline 0) {
/*
* Ensure the vm_clock proceeds even when the virtual CPU goes to
@@ -333,8 +344,8 @@ void qemu_clock_warp(QEMUClock *clock)
* packets continuously instead of every 100ms.
*/
qemu_mod_timer(icount_warp_timer, vm_clock_warp_start + deadline);
-} else {
-qemu_notify_event();
+} else if (deadline == 0) { -- AND THIS
+qemu_clock_notify(vm_clock);
}
}
...
@@ -1145,11 +1161,23 @@ static int tcg_cpu_exec(CPUArchState *env)
#endif
if (use_icount) {
int64_t count;
+int64_t deadline;
int decr;
qemu_icount -= (env-icount_decr.u16.low + env-icount_extra);
env-icount_decr.u16.low = 0;
env-icount_extra = 0;
-count = qemu_icount_round(qemu_clock_deadline(vm_clock));
+deadline = qemu_clock_deadline_ns_all(vm_clock);
+
+/* Maintain prior (possibly buggy) behaviour where if no deadline
+ * was set (as there is no vm_clock timer) or it is more than
+ * INT32_MAX nanoseconds ahead, we still use INT32_MAX
+ * nanoseconds.
+ */
+if ((deadline 0) || (deadline INT32_MAX)) {
+deadline = INT32_MAX; / --- AND THIS
+}
+
+count = qemu_icount_round(deadline);
qemu_icount += count;
decr = (count 0x) ? 0x : count;
count -= decr;
--
Alex Bligh
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 10/08/2013 05:27, liu ping fan ha scritto: ... except if we get rid of TimerListGroup completely and put it in AioContext. To do so, we can have _two_ AioContexts in main-loop.c. One is for the block layer, the other is only used for timers. Later we Why can the AioContexts of block layer NOT hold timers? For the reason mainloop's iohandler runs after it? Or any reason raised by blocklayer itself? The point of this series is to make the AioContexts of the block layer able to hold timers. At the same time, however, the main loop's timers cannot run during a qemu_aio_wait(), as that would be backwards-incompatible. So I'm proposing to have two AioContext, and in the future separate timers-for-the-block-layer from timers-for-everything-else. Paolo
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Paolo, On 9 Aug 2013, at 15:59, Paolo Bonzini wrote: It's not papering over anything. Timers right now are provided by the event loop. If you make AioContexts have timers, you can have a new AioContext for the timers that the event loop handles before your patches. It's not related to having two nested event loops. The nested event loops have the problem that timers do not run in them, but it's also a feature---because you know exactly what code runs in the nested event loop and what doesn't. Using an entirely distinct event loop preserves the feature. I've submitted a first cut as a separate patch just to see whether it works: http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg01412.html It passes 'make check' anyway. I didn't wrap TimerListGroup into a separate struct for reasons I'll set out below. I think there are two ways to go here: 1. Wrap TimerListGroup into a separate struct, leave all the TimerListGroup functions in. This probably makes it easier if (for instance) we decided to get rid of AioContexts entirely and make them g_source subclasses (per Wenchao Xia). 2. Remove the TimerListGroup thing entirely and just have an array of TimerLists of size QEMU_CLOCK_MAX. I can leave the things that iterate that array inside qemu_timer.c (which I'd rather do for encapsulation). Part of the problem with the old timer code was that there were (in my opinion) far too many files that were working out what to iterate, and I really don't want to reintroduce that. Despite the fact we both dislike the name TimerListGroup, I think the way to go here is (1). (2) does not really save lines of code (certainly not compiled instructions) - it's main saving is removing a pile of commenting from include/qemu/timer.h, which makes things more opaque. I also think there may well be a use for something that wants to use timers but not AioContext (I am thinking for instance of a thread that does not do block IO). This permits that, but does not require it. WDYT? -- Alex Bligh
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 08/08/2013 23:42, Alex Bligh ha scritto:
Add QEMUTimerListGroup and helper functions, to represent
a QEMUTimerList associated with each clock. Add a default
QEMUTimerListGroup representing the default timer lists
which are not associated with any other object (e.g.
an AioContext as added by future patches).
Signed-off-by: Alex Bligh a...@alex.org.uk
---
include/qemu/timer.h | 45 +
qemu-timer.c | 41 +
2 files changed, 86 insertions(+)
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 1baa0e2..3b46f60 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,8 +52,10 @@ typedef enum {
typedef struct QEMUClock QEMUClock;
typedef struct QEMUTimerList QEMUTimerList;
+typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
Please wrap this in a struct for easier future extensibility.
I'm not a big fan of the TimerListGroup name, but I cannot think of
anything better...
... except if we get rid of TimerListGroup completely and put it in
AioContext. To do so, we can have _two_ AioContexts in main-loop.c.
One is for the block layer, the other is only used for timers. Later we
could move bottom halves from the first to the second, too.
Sorry for not thinking of this before.
Paolo
typedef void QEMUTimerCB(void *opaque);
+extern QEMUTimerListGroup main_loop_tlg;
extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/**
@@ -211,6 +213,49 @@ QEMUClock *timerlist_get_clock(QEMUTimerList
*timer_list);
bool timerlist_run_timers(QEMUTimerList *timer_list);
/**
+ * timerlistgroup_init:
+ * @tlg: the timer list group
+ *
+ * Initialise a timer list group. This must already be
+ * allocated in memory and zeroed.
+ */
+void timerlistgroup_init(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deinit:
+ * @tlg: the timer list group
+ *
+ * Deinitialise a timer list group. This must already be
+ * initialised. Note the memory is not freed.
+ */
+void timerlistgroup_deinit(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_run_timers:
+ * @tlg: the timer list group
+ *
+ * Run the timers associated with a timer list group.
+ * This will run timers on multiple clocks.
+ *
+ * Returns: true if any timer callback ran
+ */
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deadline_ns
+ * @tlg: the timer list group
+ *
+ * Determine the deadline of the soonest timer to
+ * expire associated with any timer list linked to
+ * the timer list group. Only clocks suitable for
+ * deadline calculation are included.
+ *
+ * Returns: the deadline in nanoseconds or -1 if no
+ * timers are to expire.
+ */
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
+
+/**
* qemu_timeout_ns_to_ms:
* @ns: nanosecond timeout value
*
diff --git a/qemu-timer.c b/qemu-timer.c
index 1a0a4b1..e151d24 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -59,6 +59,7 @@ struct QEMUClock {
bool enabled;
};
+QEMUTimerListGroup main_loop_tlg;
QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/* A QEMUTimerList is a list of timers attached to a clock. More
@@ -575,6 +576,45 @@ bool qemu_run_timers(QEMUClock *clock)
return timerlist_run_timers(clock-main_loop_timerlist);
}
+void timerlistgroup_init(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+tlg[type] = timerlist_new(type);
+}
+}
+
+void timerlistgroup_deinit(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+timerlist_free(tlg[type]);
+}
+}
+
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+bool progress = false;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+progress |= timerlist_run_timers(tlg[type]);
+}
+return progress;
+}
+
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg)
+{
+int64_t deadline = -1;
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+if (qemu_clock_use_for_deadline(tlg[type]-clock)) {
+deadline = qemu_soonest_timeout(deadline,
+
timerlist_deadline_ns(tlg[type]));
+}
+}
+return deadline;
+}
+
int64_t qemu_get_clock_ns(QEMUClock *clock)
{
int64_t now, last;
@@ -616,6 +656,7 @@ void init_clocks(void)
for (type = 0; type QEMU_CLOCK_MAX; type++) {
if (!qemu_clocks[type]) {
qemu_clocks[type] = qemu_clock_new(type);
+main_loop_tlg[type] = qemu_clocks[type]-main_loop_timerlist;
}
}
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Paolo,
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 1baa0e2..3b46f60 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,8 +52,10 @@ typedef enum {
typedef struct QEMUClock QEMUClock;
typedef struct QEMUTimerList QEMUTimerList;
+typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
Please wrap this in a struct for easier future extensibility.
OK
I'm not a big fan of the TimerListGroup name, but I cannot think of
anything better...
Ditto
... except if we get rid of TimerListGroup completely and put it in
AioContext. To do so, we can have _two_ AioContexts in main-loop.c.
One is for the block layer, the other is only used for timers. Later we
could move bottom halves from the first to the second, too.
I don't really want to do this, or at least I don't want to do it yet.
Partly my concern is about initialising a dummy AioContext and having
it hang around. Partly the point is that the timer stuff is meant to
work without any AioContexts. But mostly I think Stefan is already
looking at reworking the thing into a single event loop so this
problem will go away of its own accord.
Apart from the fact I will have just wrapped it in a struct ( :-) ),
the entire change required then will be:
1. delete main_loop_ltg
2. rewrite the wrappers to use a default AioContext
3. delete the typedef and put the raw array into the AioContext
struct (having deleted main_loop_tlg as unused.
This is all pretty trivial and I think best done after Stefan's
work than before.
Alex
Sorry for not thinking of this before.
Paolo
typedef void QEMUTimerCB(void *opaque);
+extern QEMUTimerListGroup main_loop_tlg;
extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/**
@@ -211,6 +213,49 @@ QEMUClock *timerlist_get_clock(QEMUTimerList
*timer_list);
bool timerlist_run_timers(QEMUTimerList *timer_list);
/**
+ * timerlistgroup_init:
+ * @tlg: the timer list group
+ *
+ * Initialise a timer list group. This must already be
+ * allocated in memory and zeroed.
+ */
+void timerlistgroup_init(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deinit:
+ * @tlg: the timer list group
+ *
+ * Deinitialise a timer list group. This must already be
+ * initialised. Note the memory is not freed.
+ */
+void timerlistgroup_deinit(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_run_timers:
+ * @tlg: the timer list group
+ *
+ * Run the timers associated with a timer list group.
+ * This will run timers on multiple clocks.
+ *
+ * Returns: true if any timer callback ran
+ */
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deadline_ns
+ * @tlg: the timer list group
+ *
+ * Determine the deadline of the soonest timer to
+ * expire associated with any timer list linked to
+ * the timer list group. Only clocks suitable for
+ * deadline calculation are included.
+ *
+ * Returns: the deadline in nanoseconds or -1 if no
+ * timers are to expire.
+ */
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
+
+/**
* qemu_timeout_ns_to_ms:
* @ns: nanosecond timeout value
*
diff --git a/qemu-timer.c b/qemu-timer.c
index 1a0a4b1..e151d24 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -59,6 +59,7 @@ struct QEMUClock {
bool enabled;
};
+QEMUTimerListGroup main_loop_tlg;
QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/* A QEMUTimerList is a list of timers attached to a clock. More
@@ -575,6 +576,45 @@ bool qemu_run_timers(QEMUClock *clock)
return timerlist_run_timers(clock-main_loop_timerlist);
}
+void timerlistgroup_init(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+tlg[type] = timerlist_new(type);
+}
+}
+
+void timerlistgroup_deinit(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+timerlist_free(tlg[type]);
+}
+}
+
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+bool progress = false;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+progress |= timerlist_run_timers(tlg[type]);
+}
+return progress;
+}
+
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg)
+{
+int64_t deadline = -1;
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+if (qemu_clock_use_for_deadline(tlg[type]-clock)) {
+deadline = qemu_soonest_timeout(deadline,
+
timerlist_deadline_ns(tlg[type]));
+}
+}
+return deadline;
+}
+
int64_t qemu_get_clock_ns(QEMUClock *clock)
{
int64_t now, last;
@@ -616,6 +656,7 @@ void init_clocks(void)
for (type = 0; type QEMU_CLOCK_MAX; type++) {
if (!qemu_clocks[type]) {
qemu_clocks[type] = qemu_clock_new(type);
+main_loop_tlg[type] = qemu_clocks[type]-main_loop_timerlist;
}
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 09/08/2013 16:27, Alex Bligh ha scritto:
Paolo,
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 1baa0e2..3b46f60 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,8 +52,10 @@ typedef enum {
typedef struct QEMUClock QEMUClock;
typedef struct QEMUTimerList QEMUTimerList;
+typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
Please wrap this in a struct for easier future extensibility.
OK
I'm not a big fan of the TimerListGroup name, but I cannot think of
anything better...
Ditto
... except if we get rid of TimerListGroup completely and put it in
AioContext. To do so, we can have _two_ AioContexts in main-loop.c.
One is for the block layer, the other is only used for timers. Later we
could move bottom halves from the first to the second, too.
I don't really want to do this, or at least I don't want to do it yet.
Partly my concern is about initialising a dummy AioContext and having
it hang around. Partly the point is that the timer stuff is meant to
work without any AioContexts.
Perhaps, but do we really need it to work outside AioContexts? Using a
second AioContext for bottom halves has been mentioned for a while. It
would fix issues where a bottom half runs spuriously before QEMU starts,
just because something uses qemu_aio_wait(). Extending the same
approach to timers is natural.
At some point Stefan was planning to add a per-BlockDriverState
AioContext. Once that is done, using two different AioContext for
default block layer context and default main loop context would be
very easily done.
But mostly I think Stefan is already
looking at reworking the thing into a single event loop so this
problem will go away of its own accord.
Apart from the fact I will have just wrapped it in a struct ( :-) ),
the entire change required then will be:
1. delete main_loop_ltg
2. rewrite the wrappers to use a default AioContext
3. delete the typedef and put the raw array into the AioContext
struct (having deleted main_loop_tlg as unused.
This is all pretty trivial and I think best done after Stefan's
work than before.
There's also some more cleanup to do, for example you would also get
rid of the notify callback.
The point is that you can get rid altogether of TimerListGroup if you
just stick the array in the AioContext. There's no use in adding a
concept with an easy plan to delete it, only waiting for someone willing
to do the work. It is not related to anything that Stefan is
doing---the concept of TimerListGroup is introduced by this series.
Paolo
Alex
Sorry for not thinking of this before.
Paolo
typedef void QEMUTimerCB(void *opaque);
+extern QEMUTimerListGroup main_loop_tlg;
extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/**
@@ -211,6 +213,49 @@ QEMUClock *timerlist_get_clock(QEMUTimerList
*timer_list);
bool timerlist_run_timers(QEMUTimerList *timer_list);
/**
+ * timerlistgroup_init:
+ * @tlg: the timer list group
+ *
+ * Initialise a timer list group. This must already be
+ * allocated in memory and zeroed.
+ */
+void timerlistgroup_init(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deinit:
+ * @tlg: the timer list group
+ *
+ * Deinitialise a timer list group. This must already be
+ * initialised. Note the memory is not freed.
+ */
+void timerlistgroup_deinit(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_run_timers:
+ * @tlg: the timer list group
+ *
+ * Run the timers associated with a timer list group.
+ * This will run timers on multiple clocks.
+ *
+ * Returns: true if any timer callback ran
+ */
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deadline_ns
+ * @tlg: the timer list group
+ *
+ * Determine the deadline of the soonest timer to
+ * expire associated with any timer list linked to
+ * the timer list group. Only clocks suitable for
+ * deadline calculation are included.
+ *
+ * Returns: the deadline in nanoseconds or -1 if no
+ * timers are to expire.
+ */
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
+
+/**
* qemu_timeout_ns_to_ms:
* @ns: nanosecond timeout value
*
diff --git a/qemu-timer.c b/qemu-timer.c
index 1a0a4b1..e151d24 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -59,6 +59,7 @@ struct QEMUClock {
bool enabled;
};
+QEMUTimerListGroup main_loop_tlg;
QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/* A QEMUTimerList is a list of timers attached to a clock. More
@@ -575,6 +576,45 @@ bool qemu_run_timers(QEMUClock *clock)
return timerlist_run_timers(clock-main_loop_timerlist);
}
+void timerlistgroup_init(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+tlg[type] = timerlist_new(type);
+}
+}
+
+void timerlistgroup_deinit(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+timerlist_free(tlg[type]);
+
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
On 9 Aug 2013, at 15:35, Paolo Bonzini wrote: The point is that you can get rid altogether of TimerListGroup if you just stick the array in the AioContext. There's no use in adding a concept with an easy plan to delete it, only waiting for someone willing to do the work. It is not related to anything that Stefan is doing---the concept of TimerListGroup is introduced by this series. I don't think that's correct. Timers currently operate only outside of AioContexts and that's what we're maintaining. You're saying make them work only inside AioContexts and add another AioContext to paper over the hole that's there because we have two nested event loops. What I'm saying is if don't have 2 nested event loops (i.e. once Stefan has done his stuff), we won't need the main_loop_tlg then, and removing it will be easy. What I don't want to do is have all the worry about breaking things I don't fully understand by requiring that everything has an AioContext that uses timers now. -- Alex Bligh
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
On Fri, Aug 09, 2013 at 04:35:31PM +0200, Paolo Bonzini wrote:
Il 09/08/2013 16:27, Alex Bligh ha scritto:
Paolo,
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 1baa0e2..3b46f60 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,8 +52,10 @@ typedef enum {
typedef struct QEMUClock QEMUClock;
typedef struct QEMUTimerList QEMUTimerList;
+typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
Please wrap this in a struct for easier future extensibility.
OK
I'm not a big fan of the TimerListGroup name, but I cannot think of
anything better...
Ditto
... except if we get rid of TimerListGroup completely and put it in
AioContext. To do so, we can have _two_ AioContexts in main-loop.c.
One is for the block layer, the other is only used for timers. Later we
could move bottom halves from the first to the second, too.
I don't really want to do this, or at least I don't want to do it yet.
Partly my concern is about initialising a dummy AioContext and having
it hang around. Partly the point is that the timer stuff is meant to
work without any AioContexts.
Perhaps, but do we really need it to work outside AioContexts? Using a
second AioContext for bottom halves has been mentioned for a while. It
would fix issues where a bottom half runs spuriously before QEMU starts,
just because something uses qemu_aio_wait(). Extending the same
approach to timers is natural.
At some point Stefan was planning to add a per-BlockDriverState
AioContext. Once that is done, using two different AioContext for
default block layer context and default main loop context would be
very easily done.
The model I'm going for is that the user can configure how many event
loop threads to use. Then they can bind a -drive to a specific event
loop thread and it will operate in that AioContext.
So it isn't quite a per-BDS AioContext model but perhaps I'll hit
something which does require per-BDS AioContexts. History has shown the
Paolo's ideas here tend to be a good solution, even when I initially
tried another approach.
Stefan
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 09/08/2013 17:09, Stefan Hajnoczi ha scritto: Perhaps, but do we really need it to work outside AioContexts? Using a second AioContext for bottom halves has been mentioned for a while. It would fix issues where a bottom half runs spuriously before QEMU starts, just because something uses qemu_aio_wait(). Extending the same approach to timers is natural. At some point Stefan was planning to add a per-BlockDriverState AioContext. Once that is done, using two different AioContext for default block layer context and default main loop context would be very easily done. The model I'm going for is that the user can configure how many event loop threads to use. Then they can bind a -drive to a specific event loop thread and it will operate in that AioContext. Yes, this is really what I meant. I wasn't sure exactly how to describe it. In any case, it is not really related to nested event loops. It even seems like an extension of what I'm asking Alex to do. In principle you could have many AioContexts (plus the one for qemu_new_timer aka qemu_timer_new) but put all of them in the main iothread. Paolo So it isn't quite a per-BDS AioContext model but perhaps I'll hit something which does require per-BDS AioContexts.
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Il 09/08/2013 16:47, Alex Bligh ha scritto: On 9 Aug 2013, at 15:35, Paolo Bonzini wrote: The point is that you can get rid altogether of TimerListGroup if you just stick the array in the AioContext. There's no use in adding a concept with an easy plan to delete it, only waiting for someone willing to do the work. It is not related to anything that Stefan is doing---the concept of TimerListGroup is introduced by this series. I don't think that's correct. Timers currently operate only outside of AioContexts and that's what we're maintaining. You're saying make them work only inside AioContexts and add another AioContext to paper over the hole that's there because we have two nested event loops. It's not papering over anything. Timers right now are provided by the event loop. If you make AioContexts have timers, you can have a new AioContext for the timers that the event loop handles before your patches. It's not related to having two nested event loops. The nested event loops have the problem that timers do not run in them, but it's also a feature---because you know exactly what code runs in the nested event loop and what doesn't. Using an entirely distinct event loop preserves the feature. What I'm saying is if don't have 2 nested event loops (i.e. once Stefan has done his stuff), I'm not sure at all that Stefan's work would remove the nested event loops. Perhaps that's the source of the misunderstanding. But anyway I don't think this is related to the nested event loops. It is all about not introducing an unnecessary indirection between AioContext and TimerList. we won't need the main_loop_tlg then, and removing it will be easy. What I don't want to do is have all the worry about breaking things I don't fully understand by requiring that everything has an AioContext that uses timers now. There's nothing really to be worried about. It is totally hidden to the users that timers will go through an AioContext---just like it is totally hidden that timers do not go anymore through an alarm timer. Adding a new AioContext to the main loop is trivial, since an AioContext is simply a GSource. Your current series has much more scary surgery than this! Paolo
Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
[...]
typedef struct QEMUClock QEMUClock;
typedef struct QEMUTimerList QEMUTimerList;
+typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
Please wrap this in a struct for easier future extensibility.
I'm not a big fan of the TimerListGroup name, but I cannot think of
anything better...
... except if we get rid of TimerListGroup completely and put it in
AioContext. To do so, we can have _two_ AioContexts in main-loop.c.
One is for the block layer, the other is only used for timers. Later we
Why can the AioContexts of block layer NOT hold timers? For the
reason mainloop's iohandler runs after it? Or any reason raised by
blocklayer itself?
Thx
Pingfan
could move bottom halves from the first to the second, too.
Sorry for not thinking of this before.
Paolo
typedef void QEMUTimerCB(void *opaque);
+extern QEMUTimerListGroup main_loop_tlg;
extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/**
@@ -211,6 +213,49 @@ QEMUClock *timerlist_get_clock(QEMUTimerList
*timer_list);
bool timerlist_run_timers(QEMUTimerList *timer_list);
/**
+ * timerlistgroup_init:
+ * @tlg: the timer list group
+ *
+ * Initialise a timer list group. This must already be
+ * allocated in memory and zeroed.
+ */
+void timerlistgroup_init(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deinit:
+ * @tlg: the timer list group
+ *
+ * Deinitialise a timer list group. This must already be
+ * initialised. Note the memory is not freed.
+ */
+void timerlistgroup_deinit(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_run_timers:
+ * @tlg: the timer list group
+ *
+ * Run the timers associated with a timer list group.
+ * This will run timers on multiple clocks.
+ *
+ * Returns: true if any timer callback ran
+ */
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deadline_ns
+ * @tlg: the timer list group
+ *
+ * Determine the deadline of the soonest timer to
+ * expire associated with any timer list linked to
+ * the timer list group. Only clocks suitable for
+ * deadline calculation are included.
+ *
+ * Returns: the deadline in nanoseconds or -1 if no
+ * timers are to expire.
+ */
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
+
+/**
* qemu_timeout_ns_to_ms:
* @ns: nanosecond timeout value
*
diff --git a/qemu-timer.c b/qemu-timer.c
index 1a0a4b1..e151d24 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -59,6 +59,7 @@ struct QEMUClock {
bool enabled;
};
+QEMUTimerListGroup main_loop_tlg;
QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/* A QEMUTimerList is a list of timers attached to a clock. More
@@ -575,6 +576,45 @@ bool qemu_run_timers(QEMUClock *clock)
return timerlist_run_timers(clock-main_loop_timerlist);
}
+void timerlistgroup_init(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+tlg[type] = timerlist_new(type);
+}
+}
+
+void timerlistgroup_deinit(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+timerlist_free(tlg[type]);
+}
+}
+
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+bool progress = false;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+progress |= timerlist_run_timers(tlg[type]);
+}
+return progress;
+}
+
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg)
+{
+int64_t deadline = -1;
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+if (qemu_clock_use_for_deadline(tlg[type]-clock)) {
+deadline = qemu_soonest_timeout(deadline,
+
timerlist_deadline_ns(tlg[type]));
+}
+}
+return deadline;
+}
+
int64_t qemu_get_clock_ns(QEMUClock *clock)
{
int64_t now, last;
@@ -616,6 +656,7 @@ void init_clocks(void)
for (type = 0; type QEMU_CLOCK_MAX; type++) {
if (!qemu_clocks[type]) {
qemu_clocks[type] = qemu_clock_new(type);
+main_loop_tlg[type] = qemu_clocks[type]-main_loop_timerlist;
}
}
[Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions
Add QEMUTimerListGroup and helper functions, to represent
a QEMUTimerList associated with each clock. Add a default
QEMUTimerListGroup representing the default timer lists
which are not associated with any other object (e.g.
an AioContext as added by future patches).
Signed-off-by: Alex Bligh a...@alex.org.uk
---
include/qemu/timer.h | 45 +
qemu-timer.c | 41 +
2 files changed, 86 insertions(+)
diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 1baa0e2..3b46f60 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -52,8 +52,10 @@ typedef enum {
typedef struct QEMUClock QEMUClock;
typedef struct QEMUTimerList QEMUTimerList;
+typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
typedef void QEMUTimerCB(void *opaque);
+extern QEMUTimerListGroup main_loop_tlg;
extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/**
@@ -211,6 +213,49 @@ QEMUClock *timerlist_get_clock(QEMUTimerList *timer_list);
bool timerlist_run_timers(QEMUTimerList *timer_list);
/**
+ * timerlistgroup_init:
+ * @tlg: the timer list group
+ *
+ * Initialise a timer list group. This must already be
+ * allocated in memory and zeroed.
+ */
+void timerlistgroup_init(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deinit:
+ * @tlg: the timer list group
+ *
+ * Deinitialise a timer list group. This must already be
+ * initialised. Note the memory is not freed.
+ */
+void timerlistgroup_deinit(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_run_timers:
+ * @tlg: the timer list group
+ *
+ * Run the timers associated with a timer list group.
+ * This will run timers on multiple clocks.
+ *
+ * Returns: true if any timer callback ran
+ */
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg);
+
+/**
+ * timerlistgroup_deadline_ns
+ * @tlg: the timer list group
+ *
+ * Determine the deadline of the soonest timer to
+ * expire associated with any timer list linked to
+ * the timer list group. Only clocks suitable for
+ * deadline calculation are included.
+ *
+ * Returns: the deadline in nanoseconds or -1 if no
+ * timers are to expire.
+ */
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
+
+/**
* qemu_timeout_ns_to_ms:
* @ns: nanosecond timeout value
*
diff --git a/qemu-timer.c b/qemu-timer.c
index 1a0a4b1..e151d24 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -59,6 +59,7 @@ struct QEMUClock {
bool enabled;
};
+QEMUTimerListGroup main_loop_tlg;
QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
/* A QEMUTimerList is a list of timers attached to a clock. More
@@ -575,6 +576,45 @@ bool qemu_run_timers(QEMUClock *clock)
return timerlist_run_timers(clock-main_loop_timerlist);
}
+void timerlistgroup_init(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+tlg[type] = timerlist_new(type);
+}
+}
+
+void timerlistgroup_deinit(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+timerlist_free(tlg[type]);
+}
+}
+
+bool timerlistgroup_run_timers(QEMUTimerListGroup tlg)
+{
+QEMUClockType type;
+bool progress = false;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+progress |= timerlist_run_timers(tlg[type]);
+}
+return progress;
+}
+
+int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg)
+{
+int64_t deadline = -1;
+QEMUClockType type;
+for (type = 0; type QEMU_CLOCK_MAX; type++) {
+if (qemu_clock_use_for_deadline(tlg[type]-clock)) {
+deadline = qemu_soonest_timeout(deadline,
+timerlist_deadline_ns(tlg[type]));
+}
+}
+return deadline;
+}
+
int64_t qemu_get_clock_ns(QEMUClock *clock)
{
int64_t now, last;
@@ -616,6 +656,7 @@ void init_clocks(void)
for (type = 0; type QEMU_CLOCK_MAX; type++) {
if (!qemu_clocks[type]) {
qemu_clocks[type] = qemu_clock_new(type);
+main_loop_tlg[type] = qemu_clocks[type]-main_loop_timerlist;
}
}
--
1.7.9.5
