Re: [PATCH 1/3] util & iothread: Introduce event-loop abstract class
On Mon, Feb 28, 2022 at 08:05:52PM +0100, Nicolas Saenz Julienne wrote: > On Thu, 2022-02-24 at 09:48 +, Stefan Hajnoczi wrote: > > On Mon, Feb 21, 2022 at 06:08:43PM +0100, Nicolas Saenz Julienne wrote: > > > diff --git a/qom/meson.build b/qom/meson.build > > > index 062a3789d8..c20e5dd1cb 100644 > > > --- a/qom/meson.build > > > +++ b/qom/meson.build > > > @@ -4,6 +4,7 @@ qom_ss.add(files( > > >'object.c', > > >'object_interfaces.c', > > >'qom-qobject.c', > > > + '../util/event-loop.c', > > > > This looks strange. I expected util/event-loop.c to be in > > util/meson.build and added to the util_ss SourceSet instead of qom_ss. > > > > What is the reason for this? > > Sorry I meant to move it into the qom directory while cleaning up the series > but forgot about it. > > That said, I can see how moving 'event-loop-backend' in qom_ss isn't the > cleanest. Yes, qom/ is meant for the QEMU Object Model infrastructure itself, not for all the QOM classes that rely on it. > So I tried moving it into util_ss, but for some reason nobody is calling > 'type_init(even_loop_register_type)'. My guess is there's some compilation > quirk I'm missing. Maybe the issue is that libqemuutil.a (util_ss) object files are linked on demand. If there are no symbol dependencies in the main QEMU code to event-loop.o then it won't be linked into the executable. That may be why event_loop_register_type() isn't being called (it's set up by an __attribute__((constructor)) function in event-loop.o so it doesn't help create a symbol dependency). > Any suggestions? I wonder if util_ss is the right spot for > 'event-loop-backend' > anyway, but I don't have a better idea. What Paolo suggested sounds good: move event-loop.c next to iothread.c in the top-level source directory. Stefan signature.asc Description: PGP signature
Re: [PATCH 1/3] util & iothread: Introduce event-loop abstract class
Hi Stefan, thanks for the review.
On Thu, 2022-02-24 at 09:48 +, Stefan Hajnoczi wrote:
> On Mon, Feb 21, 2022 at 06:08:43PM +0100, Nicolas Saenz Julienne wrote:
> > diff --git a/qom/meson.build b/qom/meson.build
> > index 062a3789d8..c20e5dd1cb 100644
> > --- a/qom/meson.build
> > +++ b/qom/meson.build
> > @@ -4,6 +4,7 @@ qom_ss.add(files(
> >'object.c',
> >'object_interfaces.c',
> >'qom-qobject.c',
> > + '../util/event-loop.c',
>
> This looks strange. I expected util/event-loop.c to be in
> util/meson.build and added to the util_ss SourceSet instead of qom_ss.
>
> What is the reason for this?
Sorry I meant to move it into the qom directory while cleaning up the series
but forgot about it.
That said, I can see how moving 'event-loop-backend' in qom_ss isn't the
cleanest.
So I tried moving it into util_ss, but for some reason nobody is calling
'type_init(even_loop_register_type)'. My guess is there's some compilation
quirk I'm missing.
Any suggestions? I wonder if util_ss is the right spot for 'event-loop-backend'
anyway, but I don't have a better idea.
> > ))
> >
> > qmp_ss.add(files('qom-qmp-cmds.c'))
> > diff --git a/util/event-loop.c b/util/event-loop.c
> > new file mode 100644
> > index 00..f3e50909a0
> > --- /dev/null
> > +++ b/util/event-loop.c
>
> The naming is a little inconsistent. The filename "event-loop.c" does
> match the QOM type or typedef name event-loop-backend/EventLoopBackend.
>
> I suggest calling the source file event-loop-base.c and the QOM type
> "event-loop-base".
Agree.
> > @@ -0,0 +1,142 @@
> > +/*
> > + * QEMU event-loop backend
> > + *
> > + * Copyright (C) 2022 Red Hat Inc
> > + *
> > + * Authors:
> > + * Nicolas Saenz Julienne
>
> Most of the code is cut and pasted. It would be nice to carry over the
> authorship information too.
Yes, of course.
> > +struct EventLoopBackend {
> > +Object parent;
> > +
> > +/* AioContext poll parameters */
> > +int64_t poll_max_ns;
> > +int64_t poll_grow;
> > +int64_t poll_shrink;
>
> These parameters do not affect the main loop because it cannot poll. If
> you decide to keep them in the base class, please document that they
> have no effect on the main loop so users aren't confused. I would keep
> them unique to IOThread for now.
OK, I'll keep them unique then.
Thanks!
--
Nicolás Sáenz
Re: [PATCH 1/3] util & iothread: Introduce event-loop abstract class
On 2/24/22 10:48, Stefan Hajnoczi wrote:
On Mon, Feb 21, 2022 at 06:08:43PM +0100, Nicolas Saenz Julienne wrote:
diff --git a/qom/meson.build b/qom/meson.build
index 062a3789d8..c20e5dd1cb 100644
--- a/qom/meson.build
+++ b/qom/meson.build
@@ -4,6 +4,7 @@ qom_ss.add(files(
'object.c',
'object_interfaces.c',
'qom-qobject.c',
+ '../util/event-loop.c',
This looks strange. I expected util/event-loop.c to be in
util/meson.build and added to the util_ss SourceSet instead of qom_ss.
Or alternatively, to be in the root just like iothread.c.
Paolo
What is the reason for this?
))
qmp_ss.add(files('qom-qmp-cmds.c'))
diff --git a/util/event-loop.c b/util/event-loop.c
new file mode 100644
index 00..f3e50909a0
--- /dev/null
+++ b/util/event-loop.c
The naming is a little inconsistent. The filename "event-loop.c" does
match the QOM type or typedef name event-loop-backend/EventLoopBackend.
I suggest calling the source file event-loop-base.c and the QOM type
"event-loop-base".
@@ -0,0 +1,142 @@
+/*
+ * QEMU event-loop backend
+ *
+ * Copyright (C) 2022 Red Hat Inc
+ *
+ * Authors:
+ * Nicolas Saenz Julienne
Most of the code is cut and pasted. It would be nice to carry over the
authorship information too.
+struct EventLoopBackend {
+Object parent;
+
+/* AioContext poll parameters */
+int64_t poll_max_ns;
+int64_t poll_grow;
+int64_t poll_shrink;
These parameters do not affect the main loop because it cannot poll. If
you decide to keep them in the base class, please document that they
have no effect on the main loop so users aren't confused. I would keep
them unique to IOThread for now.
Re: [PATCH 1/3] util & iothread: Introduce event-loop abstract class
On Mon, Feb 21, 2022 at 06:08:43PM +0100, Nicolas Saenz Julienne wrote:
> diff --git a/qom/meson.build b/qom/meson.build
> index 062a3789d8..c20e5dd1cb 100644
> --- a/qom/meson.build
> +++ b/qom/meson.build
> @@ -4,6 +4,7 @@ qom_ss.add(files(
>'object.c',
>'object_interfaces.c',
>'qom-qobject.c',
> + '../util/event-loop.c',
This looks strange. I expected util/event-loop.c to be in
util/meson.build and added to the util_ss SourceSet instead of qom_ss.
What is the reason for this?
> ))
>
> qmp_ss.add(files('qom-qmp-cmds.c'))
> diff --git a/util/event-loop.c b/util/event-loop.c
> new file mode 100644
> index 00..f3e50909a0
> --- /dev/null
> +++ b/util/event-loop.c
The naming is a little inconsistent. The filename "event-loop.c" does
match the QOM type or typedef name event-loop-backend/EventLoopBackend.
I suggest calling the source file event-loop-base.c and the QOM type
"event-loop-base".
> @@ -0,0 +1,142 @@
> +/*
> + * QEMU event-loop backend
> + *
> + * Copyright (C) 2022 Red Hat Inc
> + *
> + * Authors:
> + * Nicolas Saenz Julienne
Most of the code is cut and pasted. It would be nice to carry over the
authorship information too.
> +struct EventLoopBackend {
> +Object parent;
> +
> +/* AioContext poll parameters */
> +int64_t poll_max_ns;
> +int64_t poll_grow;
> +int64_t poll_shrink;
These parameters do not affect the main loop because it cannot poll. If
you decide to keep them in the base class, please document that they
have no effect on the main loop so users aren't confused. I would keep
them unique to IOThread for now.
signature.asc
Description: PGP signature
[PATCH 1/3] util & iothread: Introduce event-loop abstract class
Introduce the 'event-loop-backend' abstract class, it'll hold the
properties common to all event loops and provide the necessary hooks for
their creation. Then have 'IOThread' inherit from it. The class is
defined as user creatable and provides a hook for its children to attach
themselves to the user creatable class 'complete' function.
The new 'event-loop-backend' class will live in the util directory, and
will be packed into the qom static library.
No functional changes intended.
Signed-off-by: Nicolas Saenz Julienne
---
MAINTAINERS | 1 +
include/sysemu/iothread.h | 11 +--
iothread.c| 171 --
qom/meson.build | 1 +
util/event-loop.c | 142 +++
util/event-loop.h | 40 +
6 files changed, 204 insertions(+), 162 deletions(-)
create mode 100644 util/event-loop.c
create mode 100644 util/event-loop.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 4b3ae2ab08..e5ffbea449 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2670,6 +2670,7 @@ F: include/sysemu/runstate.h
F: include/sysemu/runstate-action.h
F: util/main-loop.c
F: util/qemu-timer.c
+F: util/event-loop*
F: softmmu/vl.c
F: softmmu/main.c
F: softmmu/cpus.c
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index 7f714bd136..aafef546b5 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -17,11 +17,12 @@
#include "block/aio.h"
#include "qemu/thread.h"
#include "qom/object.h"
+#include "util/event-loop.h"
#define TYPE_IOTHREAD "iothread"
struct IOThread {
-Object parent_obj;
+EventLoopBackend parent_obj;
QemuThread thread;
AioContext *ctx;
@@ -32,14 +33,6 @@ struct IOThread {
bool stopping; /* has iothread_stop() been called? */
bool running; /* should iothread_run() continue? */
int thread_id;
-
-/* AioContext poll parameters */
-int64_t poll_max_ns;
-int64_t poll_grow;
-int64_t poll_shrink;
-
-/* AioContext AIO engine parameters */
-int64_t aio_max_batch;
};
typedef struct IOThread IOThread;
diff --git a/iothread.c b/iothread.c
index 0f98af0f2a..fca5ee8f4c 100644
--- a/iothread.c
+++ b/iothread.c
@@ -23,22 +23,13 @@
#include "qemu/error-report.h"
#include "qemu/rcu.h"
#include "qemu/main-loop.h"
+#include "util/event-loop.h"
typedef ObjectClass IOThreadClass;
DECLARE_CLASS_CHECKERS(IOThreadClass, IOTHREAD,
TYPE_IOTHREAD)
-#ifdef CONFIG_POSIX
-/* Benchmark results from 2016 on NVMe SSD drives show max polling times around
- * 16-32 microseconds yield IOPS improvements for both iodepth=1 and iodepth=32
- * workloads.
- */
-#define IOTHREAD_POLL_MAX_NS_DEFAULT 32768ULL
-#else
-#define IOTHREAD_POLL_MAX_NS_DEFAULT 0ULL
-#endif
-
static void *iothread_run(void *opaque)
{
IOThread *iothread = opaque;
@@ -105,7 +96,6 @@ static void iothread_instance_init(Object *obj)
{
IOThread *iothread = IOTHREAD(obj);
-iothread->poll_max_ns = IOTHREAD_POLL_MAX_NS_DEFAULT;
iothread->thread_id = -1;
qemu_sem_init(>init_done_sem, 0);
/* By default, we don't run gcontext */
@@ -152,28 +142,24 @@ static void iothread_init_gcontext(IOThread *iothread)
iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
}
-static void iothread_set_aio_context_params(IOThread *iothread, Error **errp)
+static void iothread_set_aio_context_params(EventLoopBackend *bc, Error **errp)
{
+IOThread *iothread = IOTHREAD(bc);
ERRP_GUARD();
-aio_context_set_poll_params(iothread->ctx,
-iothread->poll_max_ns,
-iothread->poll_grow,
-iothread->poll_shrink,
-errp);
+aio_context_set_poll_params(iothread->ctx, bc->poll_max_ns, bc->poll_grow,
+bc->poll_shrink, errp);
if (*errp) {
return;
}
-aio_context_set_aio_params(iothread->ctx,
- iothread->aio_max_batch,
- errp);
+aio_context_set_aio_params(iothread->ctx, bc->aio_max_batch, errp);
}
-static void iothread_complete(UserCreatable *obj, Error **errp)
+static void iothread_init(EventLoopBackend *bc, Error **errp)
{
Error *local_error = NULL;
-IOThread *iothread = IOTHREAD(obj);
+IOThread *iothread = IOTHREAD(bc);
char *thread_name;
iothread->stopping = false;
@@ -189,7 +175,7 @@ static void iothread_complete(UserCreatable *obj, Error
**errp)
*/
iothread_init_gcontext(iothread);
-iothread_set_aio_context_params(iothread, _error);
+iothread_set_aio_context_params(bc, _error);
if (local_error) {
error_propagate(errp, local_error);
aio_context_unref(iothread->ctx);
@@ -201,7 +187,7 @@ static void iothread_complete(UserCreatable *obj, Error
**errp)
* to
