Re: [PATCH v2 2/4] Introduce event-loop-base abstract class
On Mon, 2022-03-14 at 13:33 +, Stefan Hajnoczi wrote:
> On Fri, Mar 11, 2022 at 11:17:22AM +0100, Nicolas Saenz Julienne wrote:
> > On Thu, 2022-03-10 at 10:25 +, Stefan Hajnoczi wrote:
> > > On Thu, Mar 03, 2022 at 03:58:20PM +0100, Nicolas Saenz Julienne wrote:
> > > > @@ -2935,13 +2947,6 @@ qemu_syms = custom_target('qemu.syms', output:
> > > > 'qemu.syms',
> > > > capture: true,
> > > > command: [undefsym, nm, '@INPUT@'])
> > > >
> > > > -qom_ss = qom_ss.apply(config_host, strict: false)
> > > > -libqom = static_library('qom', qom_ss.sources() + genh,
> > > > -dependencies: [qom_ss.dependencies()],
> > > > -name_suffix: 'fa')
> > > > -
> > > > -qom = declare_dependency(link_whole: libqom)
> > > > -
> > >
> > > Why was it necessary to move qom_ss and subdir('hw') up? Can
> > > event_loop_base be defined down here instead?
> >
> > The way I setup it up, qemuutil now depdens on event_loop_base which in turn
> > depends on qom. IIUC I can't declare dependencies without declaring first
> > the
> > libraries and source sets. All has to happen sequencially. With this in
> > mind,
> > almost all libraries depend on libqemuutil so moving it down isn't possible.
>
> I see now. The qemuutil dependency on event_loop_base is introduced in
> the next patch so the reason wasn't clear at this point in the patch
> series.
I'll mention it in the commit message.
--
Nicolás Sáenz
Re: [PATCH v2 2/4] Introduce event-loop-base abstract class
On Fri, Mar 11, 2022 at 11:17:22AM +0100, Nicolas Saenz Julienne wrote:
> On Thu, 2022-03-10 at 10:25 +, Stefan Hajnoczi wrote:
> > On Thu, Mar 03, 2022 at 03:58:20PM +0100, Nicolas Saenz Julienne wrote:
> > > @@ -2935,13 +2947,6 @@ qemu_syms = custom_target('qemu.syms', output:
> > > 'qemu.syms',
> > > capture: true,
> > > command: [undefsym, nm, '@INPUT@'])
> > >
> > > -qom_ss = qom_ss.apply(config_host, strict: false)
> > > -libqom = static_library('qom', qom_ss.sources() + genh,
> > > -dependencies: [qom_ss.dependencies()],
> > > -name_suffix: 'fa')
> > > -
> > > -qom = declare_dependency(link_whole: libqom)
> > > -
> >
> > Why was it necessary to move qom_ss and subdir('hw') up? Can
> > event_loop_base be defined down here instead?
>
> The way I setup it up, qemuutil now depdens on event_loop_base which in turn
> depends on qom. IIUC I can't declare dependencies without declaring first the
> libraries and source sets. All has to happen sequencially. With this in mind,
> almost all libraries depend on libqemuutil so moving it down isn't possible.
I see now. The qemuutil dependency on event_loop_base is introduced in
the next patch so the reason wasn't clear at this point in the patch
series.
Stefan
signature.asc
Description: PGP signature
Re: [PATCH v2 2/4] Introduce event-loop-base abstract class
On Thu, 2022-03-10 at 10:25 +, Stefan Hajnoczi wrote:
> On Thu, Mar 03, 2022 at 03:58:20PM +0100, Nicolas Saenz Julienne wrote:
> > @@ -2935,13 +2947,6 @@ qemu_syms = custom_target('qemu.syms', output:
> > 'qemu.syms',
> > capture: true,
> > command: [undefsym, nm, '@INPUT@'])
> >
> > -qom_ss = qom_ss.apply(config_host, strict: false)
> > -libqom = static_library('qom', qom_ss.sources() + genh,
> > -dependencies: [qom_ss.dependencies()],
> > -name_suffix: 'fa')
> > -
> > -qom = declare_dependency(link_whole: libqom)
> > -
>
> Why was it necessary to move qom_ss and subdir('hw') up? Can
> event_loop_base be defined down here instead?
The way I setup it up, qemuutil now depdens on event_loop_base which in turn
depends on qom. IIUC I can't declare dependencies without declaring first the
libraries and source sets. All has to happen sequencially. With this in mind,
almost all libraries depend on libqemuutil so moving it down isn't possible.
subdir('hw') was also moved up, as 'hw/nvram/meson.build' is introducing files
into qom_ss. This operation has to be performed before declaring libqom.
> (The benefit of less code churn is it reduces the risk of patch conflicts.)
Agree, I know how painful it can be for backports.
Thanks,
--
Nicolás Sáenz
Re: [PATCH v2 2/4] Introduce event-loop-base abstract class
On Thu, Mar 03, 2022 at 03:58:20PM +0100, Nicolas Saenz Julienne wrote:
> @@ -2935,13 +2947,6 @@ qemu_syms = custom_target('qemu.syms', output:
> 'qemu.syms',
> capture: true,
> command: [undefsym, nm, '@INPUT@'])
>
> -qom_ss = qom_ss.apply(config_host, strict: false)
> -libqom = static_library('qom', qom_ss.sources() + genh,
> -dependencies: [qom_ss.dependencies()],
> -name_suffix: 'fa')
> -
> -qom = declare_dependency(link_whole: libqom)
> -
Why was it necessary to move qom_ss and subdir('hw') up? Can
event_loop_base be defined down here instead?
(The benefit of less code churn is it reduces the risk of patch conflicts.)
signature.asc
Description: PGP signature
[PATCH v2 2/4] Introduce event-loop-base abstract class
Introduce the 'event-loop-base' abstract class, it'll hold the
properties common to all event loops and provide the necessary hooks for
their creation and maintenance. Then have iothread inherit from it.
EventLoopBaseClass is defined as user creatable and provides a hook for
its children to attach themselves to the user creatable class 'complete'
function. It also provides an update_params() callback to propagate
property changes onto its children.
The new 'event-loop-base' class will live in the root directory, and it
imposes new compilation dependencies:
qom <- event-loop-base <- blockdev (iothread)
It is built with on its own using the link_whole option as there are no
direct function dependencies between the class and its children
(everything happens through the 'contructor' attribute). All this forced
some amount of reordering in meson.build, among other things the 'hw'
subdir is processed earlier as it introduces files into the 'qom' source
set.
No functional changes intended.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v1:
- Rename to event-loop-base
- Move event-loop-base into root directory
- Build event-loop-base on its own, use link_whole to avoid the problem
of the object file not being linked due to lacking direct calls from
dependencies.
- Move poll parameters into iothread, as main loop can't poll
- Update Authorship (I took what iothread.c had and added myself, I
hope that's fine)
- Introduce update_params() callback
event-loop-base.c| 104 +++
include/sysemu/event-loop-base.h | 36 +++
include/sysemu/iothread.h| 6 +-
iothread.c | 65 ++-
meson.build | 23 ---
5 files changed, 175 insertions(+), 59 deletions(-)
create mode 100644 event-loop-base.c
create mode 100644 include/sysemu/event-loop-base.h
diff --git a/event-loop-base.c b/event-loop-base.c
new file mode 100644
index 00..a924c73a7c
--- /dev/null
+++ b/event-loop-base.c
@@ -0,0 +1,104 @@
+/*
+ * QEMU event-loop base
+ *
+ * Copyright (C) 2022 Red Hat Inc
+ *
+ * Authors:
+ * Stefan Hajnoczi
+ * Nicolas Saenz Julienne
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/object_interfaces.h"
+#include "qapi/error.h"
+#include "sysemu/event-loop-base.h"
+
+typedef struct {
+const char *name;
+ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
+} EventLoopBaseParamInfo;
+
+static EventLoopBaseParamInfo aio_max_batch_info = {
+"aio-max-batch", offsetof(EventLoopBase, aio_max_batch),
+};
+
+static void event_loop_base_get_param(Object *obj, Visitor *v,
+const char *name, void *opaque, Error **errp)
+{
+EventLoopBase *event_loop_base = EVENT_LOOP_BASE(obj);
+EventLoopBaseParamInfo *info = opaque;
+int64_t *field = (void *)event_loop_base + info->offset;
+
+visit_type_int64(v, name, field, errp);
+}
+
+static void event_loop_base_set_param(Object *obj, Visitor *v,
+const char *name, void *opaque, Error **errp)
+{
+EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(obj);
+EventLoopBase *base = EVENT_LOOP_BASE(obj);
+EventLoopBaseParamInfo *info = opaque;
+int64_t *field = (void *)base + info->offset;
+int64_t value;
+
+if (!visit_type_int64(v, name, , errp)) {
+return;
+}
+
+if (value < 0) {
+error_setg(errp, "%s value must be in range [0, %" PRId64 "]",
+ info->name, INT64_MAX);
+return;
+}
+
+*field = value;
+
+if (bc->update_params) {
+bc->update_params(base, errp);
+}
+
+return;
+}
+
+static void event_loop_base_complete(UserCreatable *uc, Error **errp)
+{
+EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc);
+EventLoopBase *base = EVENT_LOOP_BASE(uc);
+
+if (bc->init) {
+bc->init(base, errp);
+}
+}
+
+static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
+{
+UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
+ucc->complete = event_loop_base_complete;
+
+object_class_property_add(klass, "aio-max-batch", "int",
+ event_loop_base_get_param,
+ event_loop_base_set_param,
+ NULL, _max_batch_info);
+}
+
+static const TypeInfo event_loop_base_info = {
+.name = TYPE_EVENT_LOOP_BASE,
+.parent = TYPE_OBJECT,
+.instance_size = sizeof(EventLoopBase),
+.class_size = sizeof(EventLoopBaseClass),
+.class_init = event_loop_base_class_init,
+.abstract = true,
+.interfaces = (InterfaceInfo[]) {
+{ TYPE_USER_CREATABLE },
+{ }
+}
+};
+
+static void register_types(void)
+{
+type_register_static(_loop_base_info);
+}
+type_init(register_types);
diff --git a/include/sysemu/event-loop-base.h
