* [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-06 7:45 ` Paolo Bonzini
2013-05-03 16:03 ` [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child Michael Roth
` (9 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This is similar in concept to "realize", though semantics are a
bit more open-ended:
And object might in some cases need a number of properties to be
specified before it can be "used"/"started"/etc. This can't always
be done via an open-ended new() function, the main example being objects
that around created via the command-line by -object.
To support these cases we allow a function, ->instance_init_completion,
to be registered that will be called by the -object constructor, or can
be called at the end of new() constructors and such.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/qom/object.h | 19 +++++++++++++++++++
qom/object.c | 21 +++++++++++++++++++++
vl.c | 2 ++
3 files changed, 42 insertions(+)
diff --git a/include/qom/object.h b/include/qom/object.h
index d0f99c5..86f1e2e 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -394,6 +394,11 @@ struct Object
* @instance_init: This function is called to initialize an object. The parent
* class will have already been initialized so the type is only responsible
* for initializing its own members.
+ * @instance_init_completion: This function is used mainly cases where an
+ * object has been instantiated via the command-line, and is called once all
+ * properties specified via command-line have been set for the object. This
+ * is not called automatically, but manually via @object_init_completion once
+ * the processing of said properties is completed.
* @instance_finalize: This function is called during object destruction. This
* is called before the parent @instance_finalize function has been called.
* An object should only free the members that are unique to its type in this
@@ -429,6 +434,7 @@ struct TypeInfo
size_t instance_size;
void (*instance_init)(Object *obj);
+ void (*instance_init_completion)(Object *obj);
void (*instance_finalize)(Object *obj);
bool abstract;
@@ -562,6 +568,19 @@ struct InterfaceClass
Object *object_new(const char *typename);
/**
+ * object_init_completion:
+ * @obj: The object to complete initialization of
+ *
+ * In cases where an object is instantiated from a command-line with a number
+ * of properties specified as parameters (generally via -object), or for cases
+ * where a new()/helper function is used to pass/set some minimal number of
+ * properties that are required prior to completion of object initialization,
+ * this function can be called to mark when that occurs to complete object
+ * initialization.
+ */
+void object_init_completion(Object *obj);
+
+/**
* object_new_with_type:
* @type: The type of the object to instantiate.
*
diff --git a/qom/object.c b/qom/object.c
index 75e6aac..c932f64 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -50,6 +50,7 @@ struct TypeImpl
void *class_data;
void (*instance_init)(Object *obj);
+ void (*instance_init_completion)(Object *obj);
void (*instance_finalize)(Object *obj);
bool abstract;
@@ -110,6 +111,7 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
ti->class_data = info->class_data;
ti->instance_init = info->instance_init;
+ ti->instance_init_completion = info->instance_init_completion;
ti->instance_finalize = info->instance_finalize;
ti->abstract = info->abstract;
@@ -422,6 +424,25 @@ Object *object_new(const char *typename)
return object_new_with_type(ti);
}
+
+static void object_init_completion_with_type(Object *obj, TypeImpl *ti)
+{
+ if (type_has_parent(ti)) {
+ object_init_completion_with_type(obj, type_get_parent(ti));
+ }
+
+ if (ti->instance_init_completion) {
+ ti->instance_init_completion(obj);
+ }
+}
+
+void object_init_completion(Object *obj)
+{
+ TypeImpl *ti = type_get_by_name(object_get_class(obj)->type->name);
+
+ object_init_completion_with_type(obj, ti);
+}
+
Object *object_dynamic_cast(Object *obj, const char *typename)
{
if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
diff --git a/vl.c b/vl.c
index 6e6225f..d454c86 100644
--- a/vl.c
+++ b/vl.c
@@ -2831,6 +2831,8 @@ static int object_create(QemuOpts *opts, void *opaque)
object_property_add_child(container_get(object_get_root(), "/objects"),
id, obj, NULL);
+ object_init_completion(obj);
+
return 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion
2013-05-03 16:03 ` [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion Michael Roth
@ 2013-05-06 7:45 ` Paolo Bonzini
2013-05-06 19:01 ` mdroth
0 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2013-05-06 7:45 UTC (permalink / raw)
To: Michael Roth; +Cc: aliguori, qemu-devel, stefanha, qemulist
Il 03/05/2013 18:03, Michael Roth ha scritto:
> This is similar in concept to "realize", though semantics are a
> bit more open-ended:
>
> And object might in some cases need a number of properties to be
> specified before it can be "used"/"started"/etc. This can't always
> be done via an open-ended new() function, the main example being objects
> that around created via the command-line by -object.
>
> To support these cases we allow a function, ->instance_init_completion,
> to be registered that will be called by the -object constructor, or can
> be called at the end of new() constructors and such.
This seems a lot like a realize property that cannot be set back to false...
Paolo
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> include/qom/object.h | 19 +++++++++++++++++++
> qom/object.c | 21 +++++++++++++++++++++
> vl.c | 2 ++
> 3 files changed, 42 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index d0f99c5..86f1e2e 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -394,6 +394,11 @@ struct Object
> * @instance_init: This function is called to initialize an object. The parent
> * class will have already been initialized so the type is only responsible
> * for initializing its own members.
> + * @instance_init_completion: This function is used mainly cases where an
> + * object has been instantiated via the command-line, and is called once all
> + * properties specified via command-line have been set for the object. This
> + * is not called automatically, but manually via @object_init_completion once
> + * the processing of said properties is completed.
> * @instance_finalize: This function is called during object destruction. This
> * is called before the parent @instance_finalize function has been called.
> * An object should only free the members that are unique to its type in this
> @@ -429,6 +434,7 @@ struct TypeInfo
>
> size_t instance_size;
> void (*instance_init)(Object *obj);
> + void (*instance_init_completion)(Object *obj);
> void (*instance_finalize)(Object *obj);
>
> bool abstract;
> @@ -562,6 +568,19 @@ struct InterfaceClass
> Object *object_new(const char *typename);
>
> /**
> + * object_init_completion:
> + * @obj: The object to complete initialization of
> + *
> + * In cases where an object is instantiated from a command-line with a number
> + * of properties specified as parameters (generally via -object), or for cases
> + * where a new()/helper function is used to pass/set some minimal number of
> + * properties that are required prior to completion of object initialization,
> + * this function can be called to mark when that occurs to complete object
> + * initialization.
> + */
> +void object_init_completion(Object *obj);
> +
> +/**
> * object_new_with_type:
> * @type: The type of the object to instantiate.
> *
> diff --git a/qom/object.c b/qom/object.c
> index 75e6aac..c932f64 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -50,6 +50,7 @@ struct TypeImpl
> void *class_data;
>
> void (*instance_init)(Object *obj);
> + void (*instance_init_completion)(Object *obj);
> void (*instance_finalize)(Object *obj);
>
> bool abstract;
> @@ -110,6 +111,7 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
> ti->class_data = info->class_data;
>
> ti->instance_init = info->instance_init;
> + ti->instance_init_completion = info->instance_init_completion;
> ti->instance_finalize = info->instance_finalize;
>
> ti->abstract = info->abstract;
> @@ -422,6 +424,25 @@ Object *object_new(const char *typename)
> return object_new_with_type(ti);
> }
>
> +
> +static void object_init_completion_with_type(Object *obj, TypeImpl *ti)
> +{
> + if (type_has_parent(ti)) {
> + object_init_completion_with_type(obj, type_get_parent(ti));
> + }
> +
> + if (ti->instance_init_completion) {
> + ti->instance_init_completion(obj);
> + }
> +}
> +
> +void object_init_completion(Object *obj)
> +{
> + TypeImpl *ti = type_get_by_name(object_get_class(obj)->type->name);
> +
> + object_init_completion_with_type(obj, ti);
> +}
> +
> Object *object_dynamic_cast(Object *obj, const char *typename)
> {
> if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
> diff --git a/vl.c b/vl.c
> index 6e6225f..d454c86 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2831,6 +2831,8 @@ static int object_create(QemuOpts *opts, void *opaque)
> object_property_add_child(container_get(object_get_root(), "/objects"),
> id, obj, NULL);
>
> + object_init_completion(obj);
> +
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion
2013-05-06 7:45 ` Paolo Bonzini
@ 2013-05-06 19:01 ` mdroth
0 siblings, 0 replies; 28+ messages in thread
From: mdroth @ 2013-05-06 19:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, stefanha, qemulist
On Mon, May 06, 2013 at 09:45:22AM +0200, Paolo Bonzini wrote:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
> > This is similar in concept to "realize", though semantics are a
> > bit more open-ended:
> >
> > And object might in some cases need a number of properties to be
> > specified before it can be "used"/"started"/etc. This can't always
> > be done via an open-ended new() function, the main example being objects
> > that around created via the command-line by -object.
> >
> > To support these cases we allow a function, ->instance_init_completion,
> > to be registered that will be called by the -object constructor, or can
> > be called at the end of new() constructors and such.
>
> This seems a lot like a realize property that cannot be set back to false...
I seem to recall some other conditions like properties not being
modifiable after being realized? In this case though I think event loops
should be startable/stoppable via properties (post-migration, for
instance, or maybe testing/debugging) with simple qom-set commands.
Not too sure honestly, mainly I just recalled realize() being pushed
down into DeviceState for specific reasons, and didn't want to confuse
this with being the same thing (even though it does seem very similar).
I'm not sure what the best approach is here.
>
> Paolo
>
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > include/qom/object.h | 19 +++++++++++++++++++
> > qom/object.c | 21 +++++++++++++++++++++
> > vl.c | 2 ++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index d0f99c5..86f1e2e 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -394,6 +394,11 @@ struct Object
> > * @instance_init: This function is called to initialize an object. The parent
> > * class will have already been initialized so the type is only responsible
> > * for initializing its own members.
> > + * @instance_init_completion: This function is used mainly cases where an
> > + * object has been instantiated via the command-line, and is called once all
> > + * properties specified via command-line have been set for the object. This
> > + * is not called automatically, but manually via @object_init_completion once
> > + * the processing of said properties is completed.
> > * @instance_finalize: This function is called during object destruction. This
> > * is called before the parent @instance_finalize function has been called.
> > * An object should only free the members that are unique to its type in this
> > @@ -429,6 +434,7 @@ struct TypeInfo
> >
> > size_t instance_size;
> > void (*instance_init)(Object *obj);
> > + void (*instance_init_completion)(Object *obj);
> > void (*instance_finalize)(Object *obj);
> >
> > bool abstract;
> > @@ -562,6 +568,19 @@ struct InterfaceClass
> > Object *object_new(const char *typename);
> >
> > /**
> > + * object_init_completion:
> > + * @obj: The object to complete initialization of
> > + *
> > + * In cases where an object is instantiated from a command-line with a number
> > + * of properties specified as parameters (generally via -object), or for cases
> > + * where a new()/helper function is used to pass/set some minimal number of
> > + * properties that are required prior to completion of object initialization,
> > + * this function can be called to mark when that occurs to complete object
> > + * initialization.
> > + */
> > +void object_init_completion(Object *obj);
> > +
> > +/**
> > * object_new_with_type:
> > * @type: The type of the object to instantiate.
> > *
> > diff --git a/qom/object.c b/qom/object.c
> > index 75e6aac..c932f64 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -50,6 +50,7 @@ struct TypeImpl
> > void *class_data;
> >
> > void (*instance_init)(Object *obj);
> > + void (*instance_init_completion)(Object *obj);
> > void (*instance_finalize)(Object *obj);
> >
> > bool abstract;
> > @@ -110,6 +111,7 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
> > ti->class_data = info->class_data;
> >
> > ti->instance_init = info->instance_init;
> > + ti->instance_init_completion = info->instance_init_completion;
> > ti->instance_finalize = info->instance_finalize;
> >
> > ti->abstract = info->abstract;
> > @@ -422,6 +424,25 @@ Object *object_new(const char *typename)
> > return object_new_with_type(ti);
> > }
> >
> > +
> > +static void object_init_completion_with_type(Object *obj, TypeImpl *ti)
> > +{
> > + if (type_has_parent(ti)) {
> > + object_init_completion_with_type(obj, type_get_parent(ti));
> > + }
> > +
> > + if (ti->instance_init_completion) {
> > + ti->instance_init_completion(obj);
> > + }
> > +}
> > +
> > +void object_init_completion(Object *obj)
> > +{
> > + TypeImpl *ti = type_get_by_name(object_get_class(obj)->type->name);
> > +
> > + object_init_completion_with_type(obj, ti);
> > +}
> > +
> > Object *object_dynamic_cast(Object *obj, const char *typename)
> > {
> > if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
> > diff --git a/vl.c b/vl.c
> > index 6e6225f..d454c86 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2831,6 +2831,8 @@ static int object_create(QemuOpts *opts, void *opaque)
> > object_property_add_child(container_get(object_get_root(), "/objects"),
> > id, obj, NULL);
> >
> > + object_init_completion(obj);
> > +
> > return 0;
> > }
> >
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-06 7:44 ` Paolo Bonzini
2013-05-03 16:03 ` [Qemu-devel] [PATCH 3/9] QSource: QEMU event source object Michael Roth
` (8 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This interface allows us to add a child property without specifying a
name. Instead, a unique name is created and passed back after adding
the property.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/qom/object.h | 16 ++++++++++++++++
qom/object.c | 25 +++++++++++++++++++++++++
2 files changed, 41 insertions(+)
diff --git a/include/qom/object.h b/include/qom/object.h
index 86f1e2e..ca0fce8 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1041,6 +1041,22 @@ void object_property_add_child(Object *obj, const char *name,
Object *child, struct Error **errp);
/**
+ * object_property_add_unnamed_child:
+ *
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @child: the child object
+ * @errp: if an error occurs, a pointer to an area to store the area
+ *
+ * Same as object_property_add_child, but will allocate a unique name to
+ * identify the child property.
+ *
+ * Returns: The name assigned to the child property, or NULL on failure.
+ */
+char *object_property_add_unnamed_child(Object *obj, Object *child,
+ struct Error **errp);
+
+/**
* object_property_add_link:
* @obj: the object to add a property to
* @name: the name of the property
diff --git a/qom/object.c b/qom/object.c
index c932f64..229a9a7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -926,6 +926,31 @@ static void object_finalize_child_property(Object *obj, const char *name,
object_unref(child);
}
+char *object_property_add_unnamed_child(Object *obj, Object *child, Error **errp)
+{
+ int idx = 0;
+ bool next_idx_found = false;
+ char name[64];
+ ObjectProperty *prop;
+
+ while (!next_idx_found) {
+ sprintf(name, "unnamed[%d]", idx);
+ QTAILQ_FOREACH(prop, &obj->properties, node) {
+ if (strcmp(name, prop->name) == 0) {
+ idx++;
+ break;
+ }
+ }
+ if (!prop) {
+ next_idx_found = true;
+ }
+ }
+
+ object_property_add_child(obj, name, child, errp);
+
+ return error_is_set(errp) ? NULL : g_strdup(name);
+}
+
void object_property_add_child(Object *obj, const char *name,
Object *child, Error **errp)
{
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child
2013-05-03 16:03 ` [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child Michael Roth
@ 2013-05-06 7:44 ` Paolo Bonzini
2013-05-06 18:48 ` mdroth
0 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2013-05-06 7:44 UTC (permalink / raw)
To: Michael Roth; +Cc: aliguori, qemu-devel, stefanha, qemulist
Il 03/05/2013 18:03, Michael Roth ha scritto:
> This interface allows us to add a child property without specifying a
> name. Instead, a unique name is created and passed back after adding
> the property.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> include/qom/object.h | 16 ++++++++++++++++
> qom/object.c | 25 +++++++++++++++++++++++++
> 2 files changed, 41 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 86f1e2e..ca0fce8 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1041,6 +1041,22 @@ void object_property_add_child(Object *obj, const char *name,
> Object *child, struct Error **errp);
>
> /**
> + * object_property_add_unnamed_child:
> + *
> + * @obj: the object to add a property to
> + * @name: the name of the property
> + * @child: the child object
> + * @errp: if an error occurs, a pointer to an area to store the area
> + *
> + * Same as object_property_add_child, but will allocate a unique name to
> + * identify the child property.
> + *
> + * Returns: The name assigned to the child property, or NULL on failure.
> + */
> +char *object_property_add_unnamed_child(Object *obj, Object *child,
> + struct Error **errp);
> +
> +/**
> * object_property_add_link:
> * @obj: the object to add a property to
> * @name: the name of the property
> diff --git a/qom/object.c b/qom/object.c
> index c932f64..229a9a7 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -926,6 +926,31 @@ static void object_finalize_child_property(Object *obj, const char *name,
> object_unref(child);
> }
>
> +char *object_property_add_unnamed_child(Object *obj, Object *child, Error **errp)
> +{
> + int idx = 0;
> + bool next_idx_found = false;
> + char name[64];
> + ObjectProperty *prop;
> +
> + while (!next_idx_found) {
> + sprintf(name, "unnamed[%d]", idx);
> + QTAILQ_FOREACH(prop, &obj->properties, node) {
> + if (strcmp(name, prop->name) == 0) {
> + idx++;
> + break;
> + }
> + }
> + if (!prop) {
> + next_idx_found = true;
> + }
> + }
> +
> + object_property_add_child(obj, name, child, errp);
> +
> + return error_is_set(errp) ? NULL : g_strdup(name);
> +}
This is O(n^3) for adding N children. O(n^2) would be not-that-great
but fine; can you take the occasion to convert the properties list to a
hashtable?
Paolo
> +
> void object_property_add_child(Object *obj, const char *name,
> Object *child, Error **errp)
> {
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child
2013-05-06 7:44 ` Paolo Bonzini
@ 2013-05-06 18:48 ` mdroth
2013-05-08 11:33 ` Stefan Hajnoczi
0 siblings, 1 reply; 28+ messages in thread
From: mdroth @ 2013-05-06 18:48 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, stefanha, qemulist
On Mon, May 06, 2013 at 09:44:13AM +0200, Paolo Bonzini wrote:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
> > This interface allows us to add a child property without specifying a
> > name. Instead, a unique name is created and passed back after adding
> > the property.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > include/qom/object.h | 16 ++++++++++++++++
> > qom/object.c | 25 +++++++++++++++++++++++++
> > 2 files changed, 41 insertions(+)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 86f1e2e..ca0fce8 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1041,6 +1041,22 @@ void object_property_add_child(Object *obj, const char *name,
> > Object *child, struct Error **errp);
> >
> > /**
> > + * object_property_add_unnamed_child:
> > + *
> > + * @obj: the object to add a property to
> > + * @name: the name of the property
> > + * @child: the child object
> > + * @errp: if an error occurs, a pointer to an area to store the area
> > + *
> > + * Same as object_property_add_child, but will allocate a unique name to
> > + * identify the child property.
> > + *
> > + * Returns: The name assigned to the child property, or NULL on failure.
> > + */
> > +char *object_property_add_unnamed_child(Object *obj, Object *child,
> > + struct Error **errp);
> > +
> > +/**
> > * object_property_add_link:
> > * @obj: the object to add a property to
> > * @name: the name of the property
> > diff --git a/qom/object.c b/qom/object.c
> > index c932f64..229a9a7 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -926,6 +926,31 @@ static void object_finalize_child_property(Object *obj, const char *name,
> > object_unref(child);
> > }
> >
> > +char *object_property_add_unnamed_child(Object *obj, Object *child, Error **errp)
> > +{
> > + int idx = 0;
> > + bool next_idx_found = false;
> > + char name[64];
> > + ObjectProperty *prop;
> > +
> > + while (!next_idx_found) {
> > + sprintf(name, "unnamed[%d]", idx);
> > + QTAILQ_FOREACH(prop, &obj->properties, node) {
> > + if (strcmp(name, prop->name) == 0) {
> > + idx++;
> > + break;
> > + }
> > + }
> > + if (!prop) {
> > + next_idx_found = true;
> > + }
> > + }
> > +
> > + object_property_add_child(obj, name, child, errp);
> > +
> > + return error_is_set(errp) ? NULL : g_strdup(name);
> > +}
>
> This is O(n^3) for adding N children. O(n^2) would be not-that-great
> but fine; can you take the occasion to convert the properties list to a
> hashtable?
Sure, I'll look into it.
>
> Paolo
>
> > +
> > void object_property_add_child(Object *obj, const char *name,
> > Object *child, Error **errp)
> > {
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child
2013-05-06 18:48 ` mdroth
@ 2013-05-08 11:33 ` Stefan Hajnoczi
0 siblings, 0 replies; 28+ messages in thread
From: Stefan Hajnoczi @ 2013-05-08 11:33 UTC (permalink / raw)
To: mdroth; +Cc: Paolo Bonzini, aliguori, qemu-devel, qemulist
On Mon, May 06, 2013 at 01:48:34PM -0500, mdroth wrote:
> On Mon, May 06, 2013 at 09:44:13AM +0200, Paolo Bonzini wrote:
> > Il 03/05/2013 18:03, Michael Roth ha scritto:
> > > This interface allows us to add a child property without specifying a
> > > name. Instead, a unique name is created and passed back after adding
> > > the property.
> > >
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > ---
> > > include/qom/object.h | 16 ++++++++++++++++
> > > qom/object.c | 25 +++++++++++++++++++++++++
> > > 2 files changed, 41 insertions(+)
> > >
> > > diff --git a/include/qom/object.h b/include/qom/object.h
> > > index 86f1e2e..ca0fce8 100644
> > > --- a/include/qom/object.h
> > > +++ b/include/qom/object.h
> > > @@ -1041,6 +1041,22 @@ void object_property_add_child(Object *obj, const char *name,
> > > Object *child, struct Error **errp);
> > >
> > > /**
> > > + * object_property_add_unnamed_child:
> > > + *
> > > + * @obj: the object to add a property to
> > > + * @name: the name of the property
> > > + * @child: the child object
> > > + * @errp: if an error occurs, a pointer to an area to store the area
> > > + *
> > > + * Same as object_property_add_child, but will allocate a unique name to
> > > + * identify the child property.
> > > + *
> > > + * Returns: The name assigned to the child property, or NULL on failure.
> > > + */
> > > +char *object_property_add_unnamed_child(Object *obj, Object *child,
> > > + struct Error **errp);
> > > +
> > > +/**
> > > * object_property_add_link:
> > > * @obj: the object to add a property to
> > > * @name: the name of the property
> > > diff --git a/qom/object.c b/qom/object.c
> > > index c932f64..229a9a7 100644
> > > --- a/qom/object.c
> > > +++ b/qom/object.c
> > > @@ -926,6 +926,31 @@ static void object_finalize_child_property(Object *obj, const char *name,
> > > object_unref(child);
> > > }
> > >
> > > +char *object_property_add_unnamed_child(Object *obj, Object *child, Error **errp)
> > > +{
> > > + int idx = 0;
> > > + bool next_idx_found = false;
> > > + char name[64];
> > > + ObjectProperty *prop;
> > > +
> > > + while (!next_idx_found) {
> > > + sprintf(name, "unnamed[%d]", idx);
> > > + QTAILQ_FOREACH(prop, &obj->properties, node) {
> > > + if (strcmp(name, prop->name) == 0) {
> > > + idx++;
> > > + break;
> > > + }
> > > + }
> > > + if (!prop) {
> > > + next_idx_found = true;
> > > + }
> > > + }
> > > +
> > > + object_property_add_child(obj, name, child, errp);
> > > +
> > > + return error_is_set(errp) ? NULL : g_strdup(name);
> > > +}
> >
> > This is O(n^3) for adding N children. O(n^2) would be not-that-great
> > but fine; can you take the occasion to convert the properties list to a
> > hashtable?
>
> Sure, I'll look into it.
Given that we already have the child pointer, perhaps just use the
uintptr_t child memory address as a unique name. It's guaranteed to be
unique unless you add the same child twice.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 3/9] QSource: QEMU event source object
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 1/9] qom: add qom_init_completion Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 2/9] qom: add object_property_add_unnamed_child Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 4/9] QContext: QEMU event loop context, abstract base class Michael Roth
` (7 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This references QContext, which will be introduced later (and also
references this). These will likely be squashed eventually, but we leave
them separate for now for easier review.
This implements a QOM-based event source object which is used in much
the same way as a GSource.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/qcontext/qsource.h | 63 +++++++++++++++++++
qcontext/qsource.c | 143 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 206 insertions(+)
create mode 100644 include/qcontext/qsource.h
create mode 100644 qcontext/qsource.c
diff --git a/include/qcontext/qsource.h b/include/qcontext/qsource.h
new file mode 100644
index 0000000..7c6dd75
--- /dev/null
+++ b/include/qcontext/qsource.h
@@ -0,0 +1,63 @@
+/*
+ * QSource: QEMU event source class
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QSOURCE_H
+#define QSOURCE_H
+
+#include "qom/object.h"
+#include "qcontext/qcontext.h"
+
+typedef struct QSource QSource;
+
+typedef bool (*QSourceCB)(QSource *qsource);
+
+typedef struct QSourceFuncs {
+ bool (*prepare)(QSource *qsource, int *timeout);
+ bool (*check)(QSource *qsource);
+ bool (*dispatch)(QSource *qsource);
+ void (*finalize)(QSource *qsource);
+} QSourceFuncs;
+
+typedef struct QSourceClass {
+ ObjectClass parent_class;
+
+ void (*add_poll)(QSource *qsource, GPollFD *pfd);
+ void (*remove_poll)(QSource *qsource, GPollFD *pfd);
+ void (*set_source_funcs)(QSource *qsource, QSourceFuncs funcs);
+ QSourceCB (*get_callback_func)(QSource *qsource);
+ void (*set_callback_func)(QSource *qsource, QSourceCB cb);
+ void (*set_user_data)(QSource *qsource, void *user_data);
+ void *(*get_user_data)(QSource *qsource);
+} QSourceClass;
+
+struct QSource {
+ /* <private */
+ Object parent_obj;
+
+ QSourceFuncs source_funcs;
+ QSourceCB callback_func;
+ GArray *poll_fds;
+ void *user_data;
+ struct QContext *ctx;
+ char *name;
+
+ /* <public> */
+};
+
+#define TYPE_QSOURCE "qsource"
+#define QSOURCE(obj) OBJECT_CHECK(QSource, (obj), TYPE_QSOURCE)
+#define QSOURCE_CLASS(klass) OBJECT_CLASS_CHECK(QSourceClass, (klass), TYPE_QSOURCE)
+#define QSOURCE_GET_CLASS(obj) OBJECT_GET_CLASS(QSourceClass, (obj), TYPE_QSOURCE)
+
+QSource *qsource_new(QSourceFuncs source_funcs, QSourceCB cb, const char *name,
+ void *opaque);
+
+#endif /* QSOURCE_H */
diff --git a/qcontext/qsource.c b/qcontext/qsource.c
new file mode 100644
index 0000000..1f3dcb5
--- /dev/null
+++ b/qcontext/qsource.c
@@ -0,0 +1,143 @@
+/*
+ * QSource: QEMU event source class
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * 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 <glib.h>
+#include "qom/object.h"
+#include "qcontext/qcontext.h"
+#include "qemu/module.h"
+
+/* FIXME: this basically causes us to destroy/rebuild an
+ * attached QSource/GSource every time we modify. What we
+ * should really have is an interface in the QContext for
+ * modifying an already attached source to avoid so much
+ * churn for simple action like adding poll fds to an
+ * source. The alternative is to require users to
+ * explicitly detach QSources before modifying them, but
+ * updating poll FDs/callbacks etc is a common operation
+ * for QSource/GSource callbacks so this limits functionality
+ * substantially
+ */
+static void qsource_update(QSource *qsource)
+{
+ QContext *ctx = qsource->ctx;
+
+ if (ctx) {
+ qcontext_detach(ctx, qsource, NULL);
+ qcontext_attach(ctx, qsource, NULL);
+ }
+}
+
+static void qsource_add_poll(QSource *qsource, GPollFD *pfd)
+{
+ g_array_append_val(qsource->poll_fds, pfd);
+ qsource_update(qsource);
+}
+
+static void qsource_remove_poll(QSource *qsource, GPollFD *pfd)
+{
+ bool done = false;
+
+ while (!done) {
+ done = true;
+ guint i;
+ for (i = 0; i < qsource->poll_fds->len; i++) {
+ if (g_array_index(qsource->poll_fds, GPollFD *, i) == pfd) {
+ g_array_remove_index(qsource->poll_fds, i);
+ /* iterate again to make sure we get them all */
+ done = false;
+ break;
+ }
+ }
+ }
+
+ qsource_update(qsource);
+}
+
+static void qsource_set_source_funcs(QSource *qsource, QSourceFuncs funcs)
+{
+ qsource->source_funcs = funcs;
+ qsource_update(qsource);
+}
+
+static QSourceCB qsource_get_callback_func(QSource *qsource)
+{
+ return qsource->callback_func;
+}
+
+static void qsource_set_callback_func(QSource *qsource, QSourceCB callback_func)
+{
+ qsource->callback_func = callback_func;
+ qsource_update(qsource);
+}
+
+static void qsource_set_user_data(QSource *qsource, void *user_data)
+{
+ qsource->user_data = user_data;
+ qsource_update(qsource);
+}
+
+static void *qsource_get_user_data(QSource *qsource)
+{
+ return qsource->user_data;
+}
+
+static void qsource_initfn(Object *obj)
+{
+ QSource *qsource = QSOURCE(obj);
+ qsource->poll_fds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
+ qsource->ctx = NULL;
+}
+
+static void qsource_class_initfn(ObjectClass *class, void *data)
+{
+ QSourceClass *k = QSOURCE_CLASS(class);
+
+ k->add_poll = qsource_add_poll;
+ k->remove_poll = qsource_remove_poll;
+ k->set_source_funcs = qsource_set_source_funcs;
+ k->get_callback_func = qsource_get_callback_func;
+ k->set_callback_func = qsource_set_callback_func;
+ k->get_user_data = qsource_get_user_data;
+ k->set_user_data = qsource_set_user_data;
+}
+
+TypeInfo qsource_info = {
+ .name = TYPE_QSOURCE,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(QSource),
+ .class_size = sizeof(QSourceClass),
+ .instance_init = qsource_initfn,
+ .class_init = qsource_class_initfn,
+ .interfaces = (InterfaceInfo[]) {
+ { },
+ },
+};
+
+static void qsource_register_types(void)
+{
+ type_register_static(&qsource_info);
+}
+
+type_init(qsource_register_types)
+
+QSource *qsource_new(QSourceFuncs funcs, QSourceCB cb, const char *name, void *opaque)
+{
+ QSource *qsource = QSOURCE(object_new(TYPE_QSOURCE));
+ QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource);
+
+ qsource->name = g_strdup(name);
+
+ qsourcek->set_source_funcs(qsource, funcs);
+ qsourcek->set_callback_func(qsource, cb);
+ qsourcek->set_user_data(qsource, opaque);
+
+ return qsource;
+}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 4/9] QContext: QEMU event loop context, abstract base class
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (2 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 3/9] QSource: QEMU event source object Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 5/9] GlibQContext: a QContext wrapper around GMainContexts Michael Roth
` (6 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This class provided interfaces and helper functions for an event loop
context modelled closely on GLib's GMainContext. It drives a number of
QSources, which are in turn modelled on GLib GSources, and driven in
very similar fashion (prepare/poll/check/dispatch interfaces). It also
provides a mechanism for creating a thread and driving itself.
QContexts are attached to the QOM composition tree via unique paths and
make use of that to provide global registration/query mechanisms.
QContexts implementations can be instantiated, referenced, and
run via the -object command-line parameter.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/qcontext/qcontext.h | 76 +++++++++++++
qcontext/qcontext.c | 252 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 328 insertions(+)
create mode 100644 include/qcontext/qcontext.h
create mode 100644 qcontext/qcontext.c
diff --git a/include/qcontext/qcontext.h b/include/qcontext/qcontext.h
new file mode 100644
index 0000000..845d1a0
--- /dev/null
+++ b/include/qcontext/qcontext.h
@@ -0,0 +1,76 @@
+/*
+ * QContext: QEMU event loop context class
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QCONTEXT_H
+#define QCONTEXT_H
+
+#include "qom/object.h"
+#include "qcontext/qsource.h"
+#include "qapi/error.h"
+#include "qemu/thread.h"
+
+/* QContext base class */
+
+typedef struct QContext QContext;
+
+typedef struct QContextClass {
+ ObjectClass parent_class;
+
+ /* called after QContext id property has been set */
+ void (*set_id_hook)(QContext *ctx, const char *name, Error **errp);
+
+ /* QContext event loop functions, abstract interfaces */
+ bool (*prepare)(QContext *ctx, int *timeout);
+ bool (*poll)(QContext *ctx, int timeout);
+ bool (*check)(QContext *ctx);
+ void (*dispatch)(QContext *ctx);
+ void (*notify)(QContext *ctx);
+
+ /* QSource registration, abstract interfaces */
+ void (*attach)(QContext *ctx, QSource *qsource, Error **errp);
+ void (*detach)(QContext *ctx, QSource *qsource, Error **errp);
+ QSource *(*find_source_by_name)(QContext *ctx, const char *name);
+} QContextClass;
+
+struct QContext {
+ Object parent_obj;
+ Object *container;
+ char *id;
+ QemuThread thread;
+ bool threaded;
+ bool should_run;
+};
+
+#define TYPE_QCONTEXT "qcontext"
+#define QCONTEXT(obj) OBJECT_CHECK(QContext, (obj), TYPE_QCONTEXT)
+#define QCONTEXT_CLASS(klass) OBJECT_CLASS_CHECK(QContextClass, (klass), TYPE_QCONTEXT)
+#define QCONTEXT_GET_CLASS(obj) OBJECT_GET_CLASS(QContextClass, (obj), TYPE_QCONTEXT)
+
+/* wrapper functions for object methods */
+
+bool qcontext_prepare(QContext *ctx, int *timeout);
+bool qcontext_poll(QContext *ctx, int timeout);
+bool qcontext_check(QContext *ctx);
+void qcontext_dispatch(QContext *ctx);
+void qcontext_notify(QContext *ctx);
+void qcontext_attach(QContext *ctx, QSource *qsource, Error **errp);
+void qcontext_detach(QContext *ctx, QSource *qsource, Error **errp);
+QSource *qcontext_find_source_by_name(QContext *ctx, const char *name);
+
+/* helper functions for working with qcontexts */
+
+QContext *qcontext_find_by_name(const char *name, Error **errp);
+bool qcontext_iterate(QContext *ctx, bool blocking);
+void qcontext_create_thread(QContext *ctx);
+void qcontext_stop_thread(QContext *ctx);
+void qcontext_register_types(void);
+
+#endif /* QCONTEXT_H */
diff --git a/qcontext/qcontext.c b/qcontext/qcontext.c
new file mode 100644
index 0000000..88cc86f
--- /dev/null
+++ b/qcontext/qcontext.c
@@ -0,0 +1,252 @@
+/*
+ * QContext: QEMU event loop context class
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * 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 <stdio.h>
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "qcontext/qcontext.h"
+#include "qapi/error.h"
+#include "string.h"
+
+/* TODO: this is for compatibility with -object, but really these
+ * should probably live in /qcontexts or something
+ */
+#define QCONTEXT_ROOT_CONTAINER "/objects"
+
+/* QContext property accessors */
+
+static char *qcontext_get_id(Object *obj, Error **errp)
+{
+ QContext *ctx = QCONTEXT(obj);
+
+ return ctx->id ? g_strdup(ctx->id) : NULL;
+}
+
+static void qcontext_set_id(Object *obj, const char *id, Error **errp)
+{
+ QContext *ctx = QCONTEXT(obj);
+ QContextClass *ctxk = QCONTEXT_GET_CLASS(ctx);
+ Object *root_container = container_get(object_get_root(),
+ QCONTEXT_ROOT_CONTAINER);
+
+ if (id) {
+ object_property_add_child(root_container, id, OBJECT(ctx), errp);
+ ctx->id = g_strdup(id);
+ } else {
+ ctx->id = object_property_add_unnamed_child(root_container,
+ OBJECT(ctx), errp);
+ }
+
+ if (ctxk->set_id_hook) {
+ ctxk->set_id_hook(ctx, id, errp);
+ }
+}
+
+static char *qcontext_get_threaded(Object *obj, Error **errp)
+{
+ QContext *ctx = QCONTEXT(obj);
+
+ return ctx->threaded ? g_strdup("yes") : g_strdup("no");
+}
+
+static void qcontext_set_threaded(Object *obj, const char *threaded,
+ Error **errp)
+{
+ QContext *ctx = QCONTEXT(obj);
+
+ if (strcmp(threaded, "yes") == 0) {
+ ctx->threaded = true;
+ ctx->should_run = true;
+ } else if (strcmp(threaded, "no") == 0) {
+ ctx->threaded = false;
+ ctx->should_run = false;
+ } else {
+ error_setg(errp,
+ "invalid value for \"threaded\","
+ " must specify \"yes\" or \"no\"");
+ }
+}
+
+/* QOM interfaces */
+
+static void qcontext_initfn(Object *obj)
+{
+ /* note: controlling these as properties is somewhat awkward. these are
+ * really static initialization parameters, but we do it this way so we
+ * can instantiate from the command-line via -object.
+ */
+ object_property_add_str(obj, "id",
+ qcontext_get_id,
+ qcontext_set_id,
+ NULL);
+ object_property_add_str(obj, "threaded",
+ qcontext_get_threaded,
+ qcontext_set_threaded,
+ NULL);
+}
+
+static void qcontext_init_completionfn(Object *obj)
+{
+ QContext *ctx = QCONTEXT(obj);
+ QContextClass *ctxk = QCONTEXT_GET_CLASS(ctx);
+ gchar *path, *id;
+
+ /* this means we were created via -object, and were added to
+ * the qom tree outside of the "id" property setter. Update
+ * our internal structures to reflect this, and execute the
+ * set_id_hook() accordingly
+ */
+ if (!ctx->id) {
+ path = object_get_canonical_path(obj);
+ id = g_strrstr(path, "/") + 1;
+ ctx->id = g_strdup(id);
+ g_free(path);
+ if (ctxk->set_id_hook) {
+ ctxk->set_id_hook(ctx, ctx->id, NULL);
+ }
+ }
+
+ if (ctx->threaded) {
+ qcontext_create_thread(ctx);
+ }
+}
+
+static void qcontext_finalizefn(Object *obj)
+{
+ QContext *ctx = QCONTEXT(obj);
+
+ if (ctx->threaded) {
+ qcontext_stop_thread(ctx);
+ }
+}
+
+static void qcontext_class_initfn(ObjectClass *class, void *data)
+{
+}
+
+static const TypeInfo qcontext_type_info = {
+ .name = TYPE_QCONTEXT,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(QContext),
+ .instance_init = qcontext_initfn,
+ .instance_init_completion = qcontext_init_completionfn,
+ .instance_finalize = qcontext_finalizefn,
+ .class_size = sizeof(QContextClass),
+ .class_init = qcontext_class_initfn,
+ .abstract = true, /* this should be abstract, just for testing */
+};
+
+void qcontext_register_types(void)
+{
+ type_register_static(&qcontext_type_info);
+}
+
+/* FIXME: for some very strange reason, the constructor function this
+ * generates doesn't get executed for qemu. it does for test-qcontext
+ * though, and in both cases the constructor for glib-qcontext gets
+ * executed okay, so for now just do registration for qcontext there
+ * as well by making qcontext_register_types() a global and calling it
+ * from there
+ */
+//type_init(qcontext_register_types)
+
+/* QContext method wrappers. Somewhat redundant but it saves on typing */
+
+bool qcontext_prepare(QContext *ctx, int *timeout)
+{
+ return QCONTEXT_GET_CLASS(ctx)->prepare(ctx, timeout);
+}
+
+bool qcontext_poll(QContext *ctx, int timeout)
+{
+ return QCONTEXT_GET_CLASS(ctx)->poll(ctx, timeout);
+}
+
+bool qcontext_check(QContext *ctx)
+{
+ return QCONTEXT_GET_CLASS(ctx)->check(ctx);
+}
+
+void qcontext_dispatch(QContext *ctx)
+{
+ QCONTEXT_GET_CLASS(ctx)->dispatch(ctx);
+}
+
+void qcontext_notify(QContext *ctx)
+{
+ QCONTEXT_GET_CLASS(ctx)->notify(ctx);
+}
+
+void qcontext_attach(QContext *ctx, QSource *source, Error **errp)
+{
+ QCONTEXT_GET_CLASS(ctx)->attach(ctx, source, errp);
+}
+
+void qcontext_detach(QContext *ctx, QSource *source, Error **errp)
+{
+ QCONTEXT_GET_CLASS(ctx)->detach(ctx, source, errp);
+}
+
+QSource *qcontext_find_source_by_name(QContext *ctx, const char *name)
+{
+ return QCONTEXT_GET_CLASS(ctx)->find_source_by_name(ctx, name);
+}
+
+/* Helper functions for working with QContexts */
+
+QContext *qcontext_find_by_name(const char *name, Error **errp)
+{
+ char path[256];
+
+ sprintf(path, "%s/%s", QCONTEXT_ROOT_CONTAINER, name);
+ return QCONTEXT(object_resolve_path_type(path, TYPE_QCONTEXT, NULL));
+}
+
+bool qcontext_iterate(QContext *ctx, bool blocking)
+{
+ int timeout = 0;
+
+ if (qcontext_prepare(ctx, &timeout)) {
+ qcontext_dispatch(ctx);
+ return true;
+ }
+
+ if (qcontext_poll(ctx, blocking ? timeout : 0) &&
+ qcontext_check(ctx)) {
+ qcontext_dispatch(ctx);
+ return true;
+ }
+
+ return false;
+}
+
+static void *qcontext_thread_fn(void *opaque)
+{
+ QContext *ctx = opaque;
+ while (ctx->should_run) {
+ qcontext_iterate(ctx, true);
+ }
+ return NULL;
+}
+
+void qcontext_create_thread(QContext *ctx)
+{
+ qemu_thread_create(&ctx->thread, qcontext_thread_fn,
+ ctx, QEMU_THREAD_JOINABLE);
+}
+
+void qcontext_stop_thread(QContext *ctx)
+{
+ ctx->should_run = false;
+ qcontext_notify(ctx);
+ qemu_thread_join(&ctx->thread);
+ ctx->threaded = false;
+}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 5/9] GlibQContext: a QContext wrapper around GMainContexts
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (3 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 4/9] QContext: QEMU event loop context, abstract base class Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 6/9] QContext: add unit tests Michael Roth
` (5 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This QContext implementation wraps a GMainContext and maps most
interfaces to the obvious/corresponding GMainContext interfaces.
It can be used as a near drop-in replacement for event loops based
directly around GMainContexts, and provides support for direct handling
of GSources (which is unique to GlibQContext, since GSources are opaque
outside of GLib)
This can be used as a stop-gap between GLib event loops and QContext
implementations that don't rely on GLib. Most GSources can be trivially
ported to QSources to decouple such event loops from GLib, though due to
the use of things like GIOChannels where we can't easily take this
approach, we'll likely have at least one GlibQContext event loop for the
forseeable future.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/qcontext/glib-qcontext.h | 59 ++++++++
qcontext/glib-qcontext.c | 280 ++++++++++++++++++++++++++++++++++++++
2 files changed, 339 insertions(+)
create mode 100644 include/qcontext/glib-qcontext.h
create mode 100644 qcontext/glib-qcontext.c
diff --git a/include/qcontext/glib-qcontext.h b/include/qcontext/glib-qcontext.h
new file mode 100644
index 0000000..4c01c8a
--- /dev/null
+++ b/include/qcontext/glib-qcontext.h
@@ -0,0 +1,59 @@
+/*
+ * GlibQContext: GLib-based QContext implementation
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef GLIB_QCONTEXT_H
+#define GLIB_QCONTEXT_H
+
+#include <glib.h>
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "qcontext/qcontext.h"
+
+/* QContextGlib implementation */
+
+#define TYPE_GLIB_QCONTEXT "glib-qcontext"
+#define GLIB_QCONTEXT(obj) \
+ OBJECT_CHECK(GlibQContext, (obj), TYPE_GLIB_QCONTEXT)
+#define GLIB_QCONTEXT_CLASS(klass) \
+ OBJECT_CLASS_CHECK(GlibQContextClass, (klass), TYPE_GLIB_QCONTEXT)
+#define GLIB_QCONTEXT_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(GlibQContextClass, (obj), TYPE_GLIB_QCONTEXT)
+
+#define GLIB_QCONTEXT_MAX_POLL_FDS (2 * 1024)
+
+typedef struct GlibQSource GlibQSource;
+
+typedef struct GlibQContext {
+ /* <private */
+ QContext parent;
+
+ char *test;
+ GMainContext *g_main_context;
+ int max_priority;
+ GPollFD poll_fds[GLIB_QCONTEXT_MAX_POLL_FDS];
+ gint n_poll_fds;
+ QTAILQ_HEAD(, GlibQSource) sources;
+
+ /* <public> */
+} GlibQContext;
+
+typedef struct GlibQContextClass {
+ QContextClass parent;
+
+ void (*init)(GlibQContext *gctx, const char *name, Error **errp);
+ void (*set_context)(GlibQContext *gctx, GMainContext *ctx);
+ GMainContext *(*get_context)(GlibQContext *gctx);
+} GlibQContextClass;
+
+GlibQContext *glib_qcontext_new(const char *id, bool threaded, Error **errp);
+GMainContext *glib_qcontext_get_context(GlibQContext *gctx);
+
+#endif /* GLIB_QCONTEXT_H */
diff --git a/qcontext/glib-qcontext.c b/qcontext/glib-qcontext.c
new file mode 100644
index 0000000..bd9b245
--- /dev/null
+++ b/qcontext/glib-qcontext.c
@@ -0,0 +1,280 @@
+/*
+ * GlibQContext: GLib-based QContext implementation
+ *
+ * Copyright IBM Corp. 2013
+ *
+ * Authors:
+ * Michael Roth <mdroth@linux.vnet.ibm.com>
+ *
+ * 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 <glib.h>
+#include <string.h>
+#include "qom/object.h"
+#include "qcontext/qcontext.h"
+#include "qcontext/glib-qcontext.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+
+struct GlibQSource {
+ GSource source;
+ guint source_id;
+ char *name;
+ QSource *qsource;
+ QTAILQ_ENTRY(GlibQSource) next;
+};
+
+static gboolean glib_qcontext_gsource_prepare(GSource *source, int *timeout)
+{
+ GlibQSource *gqsource = (GlibQSource *)source;
+ QSource *qsource = gqsource->qsource;
+
+ return qsource->source_funcs.prepare(qsource, timeout);
+}
+
+static gboolean glib_qcontext_gsource_check(GSource *source)
+{
+ GlibQSource *gqsource = (GlibQSource *)source;
+ QSource *qsource = gqsource->qsource;
+
+ return qsource->source_funcs.check(qsource);
+}
+
+static gboolean glib_qcontext_gsource_dispatch(GSource *source, GSourceFunc cb,
+ gpointer user_data)
+{
+ GlibQSource *gqsource = (GlibQSource *)source;
+ QSource *qsource = gqsource->qsource;
+
+ return qsource->source_funcs.dispatch(qsource);
+}
+
+static void glib_qcontext_gsource_finalize(GSource *source)
+{
+ GlibQSource *gqsource = (GlibQSource *)source;
+ QSource *qsource = gqsource->qsource;
+
+ qsource->source_funcs.finalize(qsource);
+}
+
+GSourceFuncs glib_gsource_funcs = {
+ glib_qcontext_gsource_prepare,
+ glib_qcontext_gsource_check,
+ glib_qcontext_gsource_dispatch,
+ glib_qcontext_gsource_finalize,
+};
+
+/* external interfaces */
+
+static bool glib_qcontext_prepare(QContext *ctx, int *timeout)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+ gint calculated_timeout = 0;
+ gboolean ret = g_main_context_prepare(gctx->g_main_context,
+ &gctx->max_priority);
+
+ gctx->n_poll_fds = g_main_context_query(gctx->g_main_context,
+ gctx->max_priority,
+ &calculated_timeout,
+ gctx->poll_fds,
+ GLIB_QCONTEXT_MAX_POLL_FDS);
+ if (timeout) {
+ *timeout = calculated_timeout;
+ }
+
+ return ret;
+}
+
+static bool glib_qcontext_poll(QContext *ctx, int timeout)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+
+ return g_poll(gctx->poll_fds, gctx->n_poll_fds, timeout) > 0;
+}
+
+static bool glib_qcontext_check(QContext *ctx)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+
+ return g_main_context_check(gctx->g_main_context,
+ gctx->max_priority,
+ gctx->poll_fds,
+ gctx->n_poll_fds);
+}
+
+static void glib_qcontext_dispatch(QContext *ctx)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+ g_main_context_dispatch(gctx->g_main_context);
+}
+
+static void glib_qcontext_notify(QContext *ctx)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+ GlibQContextClass *gctxk = GLIB_QCONTEXT_GET_CLASS(gctx);
+ g_main_context_wakeup(gctxk->get_context(gctx));
+}
+
+
+static void glib_qcontext_attach(QContext *ctx, QSource *qsource, Error **errp)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+ GSource *source = g_source_new(&glib_gsource_funcs, sizeof(GlibQSource)+512);
+ GlibQSource *gqsource = NULL, *new_gqsource = (GlibQSource *)source;
+ guint i;
+
+ if (qsource->name) {
+ QTAILQ_FOREACH(gqsource, &gctx->sources, next) {
+ if (strcmp(gqsource->name, qsource->name) == 0) {
+ error_setg(errp, "duplicate name associated with source");
+ g_source_destroy(source);
+ return;
+ }
+ }
+ }
+
+ for (i = 0; i < qsource->poll_fds->len; i++) {
+ GPollFD *pfd = g_array_index(qsource->poll_fds, GPollFD *, i);
+ g_source_add_poll(source, pfd);
+ }
+
+ new_gqsource->qsource = qsource;
+ new_gqsource->source_id = g_source_attach(source, gctx->g_main_context);
+ new_gqsource->name = g_strdup(qsource->name);
+ QTAILQ_INSERT_TAIL(&gctx->sources, new_gqsource, next);
+ qsource->ctx = ctx;
+}
+
+static void glib_qcontext_detach(QContext *ctx, QSource *qsource, Error **errp)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+ GlibQSource *gqsource = NULL;
+
+ QTAILQ_FOREACH(gqsource, &gctx->sources, next) {
+ if (gqsource->qsource == qsource) {
+ break;
+ }
+ }
+
+ if (gqsource) {
+ g_free(gqsource->name);
+ g_source_remove(gqsource->source_id);
+ QTAILQ_REMOVE(&gctx->sources, gqsource, next);
+ }
+
+ qsource->ctx = NULL;
+}
+
+static QSource *glib_qcontext_find_source_by_name(QContext *ctx, const char *name)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+ GlibQSource *gqsource = NULL;
+
+ QTAILQ_FOREACH(gqsource, &gctx->sources, next) {
+ if (strcmp(gqsource->name, name) == 0) {
+ break;
+ }
+ }
+
+ return gqsource->qsource;
+}
+
+static void glib_qcontext_set_id_hook(QContext *ctx, const char *id,
+ Error **errp)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(ctx);
+
+ if (strcmp(id, "main") != 0) {
+ gctx->g_main_context = g_main_context_new();
+ }
+}
+
+/* QOM-driven interfaces */
+static void glib_qcontext_initfn(Object *obj)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(obj);
+
+ /* TODO: this will be replaced with a new context if we set an ID
+ * property other than "main". there's no guarantee we won't attempt
+ * to spawn a main loop thread for this context (also done via a dynamic
+ * property so we can be fully instantiated via -object) before this
+ * happens though. This means we can accidentally execute a number of
+ * iterations of the default glib context (bad, since that requires
+ * special handling of BQL) before we switch over to the intended
+ * context.
+ *
+ * We seem to need a realizefn for Objects...
+ */
+ gctx->g_main_context = g_main_context_default();
+ QTAILQ_INIT(&gctx->sources);
+}
+
+static void glib_qcontext_class_initfn(ObjectClass *class, void *data)
+{
+ QContextClass *ctxk = QCONTEXT_CLASS(class);
+ GlibQContextClass *gctxk = GLIB_QCONTEXT_CLASS(class);
+
+ ctxk->prepare = glib_qcontext_prepare;
+ ctxk->poll = glib_qcontext_poll;
+ ctxk->check = glib_qcontext_check;
+ ctxk->dispatch = glib_qcontext_dispatch;
+ ctxk->notify = glib_qcontext_notify;
+
+ ctxk->attach = glib_qcontext_attach;
+ ctxk->detach = glib_qcontext_detach;
+ ctxk->find_source_by_name = glib_qcontext_find_source_by_name;
+ ctxk->set_id_hook = glib_qcontext_set_id_hook;
+
+ gctxk->get_context = glib_qcontext_get_context;
+}
+
+static const TypeInfo glib_qcontext_info = {
+ .name = TYPE_GLIB_QCONTEXT,
+ .parent = TYPE_QCONTEXT,
+ .instance_size = sizeof(GlibQContext),
+ .class_size = sizeof(GlibQContextClass),
+ .instance_init = glib_qcontext_initfn,
+ .class_init = glib_qcontext_class_initfn,
+ .interfaces = (InterfaceInfo[]) {
+ { },
+ },
+};
+
+static void glib_qcontext_register_types(void)
+{
+ type_register_static(&glib_qcontext_info);
+}
+
+type_init(glib_qcontext_register_types)
+type_init(qcontext_register_types)
+
+GlibQContext *glib_qcontext_new(const char *id, bool threaded, Error **errp)
+{
+ GlibQContext *gctx = GLIB_QCONTEXT(object_new(TYPE_GLIB_QCONTEXT));
+
+ object_property_set_str(OBJECT(gctx), id, "id", errp);
+ if (error_is_set(errp)) {
+ object_unref(OBJECT(gctx));
+ g_warning("marker 0");
+ return NULL;
+ }
+
+ object_property_set_str(OBJECT(gctx),
+ threaded ? "yes" : "no",
+ "threaded", errp);
+ if (error_is_set(errp)) {
+ object_unref(OBJECT(gctx));
+ g_warning("marker 1");
+ return NULL;
+ }
+
+ object_init_completion(OBJECT(gctx));
+
+ return gctx;
+}
+
+GMainContext *glib_qcontext_get_context(GlibQContext *gctx)
+{
+ return gctx->g_main_context;
+}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 6/9] QContext: add unit tests
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (4 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 5/9] GlibQContext: a QContext wrapper around GMainContexts Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource Michael Roth
` (4 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile.objs | 6 +-
qcontext/Makefile.objs | 1 +
qom/Makefile.objs | 6 +-
tests/Makefile | 7 ++
tests/test-qcontext.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 276 insertions(+), 3 deletions(-)
create mode 100644 qcontext/Makefile.objs
create mode 100644 tests/test-qcontext.c
diff --git a/Makefile.objs b/Makefile.objs
index fcb303a..3c571ee 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -1,7 +1,7 @@
#######################################################################
# Common libraries for tools and emulators
stub-obj-y = stubs/
-util-obj-y = util/ qobject/ qapi/ trace/
+util-obj-y = util/ qobject/ qapi/ trace/ qcontext/ qom/
#######################################################################
# block-obj-y is code used by both qemu system emulation and qemu-img
@@ -98,6 +98,10 @@ common-obj-y += disas/
# by libqemuutil.a. These should be moved to a separate .json schema.
qga-obj-y = qga/ qapi-types.o qapi-visit.o
+######################################################################
+# qom
+qom-obj-y = qom/ qobject
+
vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
diff --git a/qcontext/Makefile.objs b/qcontext/Makefile.objs
new file mode 100644
index 0000000..778af68
--- /dev/null
+++ b/qcontext/Makefile.objs
@@ -0,0 +1 @@
+util-obj-y = qcontext.o glib-qcontext.o qsource.o
diff --git a/qom/Makefile.objs b/qom/Makefile.objs
index 6a93ac7..b29b5f1 100644
--- a/qom/Makefile.objs
+++ b/qom/Makefile.objs
@@ -1,2 +1,4 @@
-common-obj-y = object.o container.o qom-qobject.o
-common-obj-y += cpu.o
+qom-obj-y = object.o container.o qom-qobject.o
+qom-obj-y += cpu.o
+common-obj-y = $(qom-obj-y)
+util-obj-y = $(qom-obj-y)
diff --git a/tests/Makefile b/tests/Makefile
index 72bf2cd..818196e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -31,6 +31,12 @@ gcov-files-test-iov-y = util/iov.c
check-unit-y += tests/test-aio$(EXESUF)
gcov-files-test-aio-$(CONFIG_WIN32) = aio-win32.c
gcov-files-test-aio-$(CONFIG_POSIX) = aio-posix.c
+check-unit-y += tests/test-qcontext$(EXESUF)
+gcov-files-test-qcontext-y = util/qcontext.c
+gcov-files-test-qcontext-y = util/glib-qcontext.c
+gcov-files-test-qcontext-y = util/qsource.c
+gcov-files-test-qcontext-y = iohandler.c
+gcov-files-test-qcontext-y = main-loop.c
check-unit-y += tests/test-thread-pool$(EXESUF)
gcov-files-test-thread-pool-y = thread-pool.c
gcov-files-test-hbitmap-y = util/hbitmap.c
@@ -91,6 +97,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
+tests/test-qcontext$(EXESUF): tests/test-qcontext.o libqemuutil.a libqemustub.a qom/object.o qom/container.o qom/qom-qobject.o qcontext/qcontext.o qcontext/glib-qcontext.o qcontext/qsource.o $(block-obj-y)
tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(block-obj-y) libqemuutil.a libqemustub.a
tests/test-iov$(EXESUF): tests/test-iov.o libqemuutil.a
tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o libqemuutil.a libqemustub.a
diff --git a/tests/test-qcontext.c b/tests/test-qcontext.c
new file mode 100644
index 0000000..52c5f54
--- /dev/null
+++ b/tests/test-qcontext.c
@@ -0,0 +1,259 @@
+#include <glib.h>
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "qcontext/qcontext.h"
+#include "qcontext/glib-qcontext.h"
+#include "qcontext/qsource.h"
+#include "qemu/event_notifier.h"
+
+typedef struct TestEventState {
+ QSource *qsource;
+ GPollFD poll_fds[8];
+ int n_poll_fds;
+ bool dispatched;
+ bool skip_poll;
+#define CB_VALUE_PASS 42
+#define CB_VALUE_FAIL 0
+ int cb_value;
+} TestEventState;
+
+static bool test_event_prepare(QSource *evt, int *timeout)
+{
+ QSourceClass *evtk = QSOURCE_GET_CLASS(evt);
+ TestEventState *s = evtk->get_user_data(evt);
+
+ return s->skip_poll;
+}
+
+static bool test_event_check(QSource *qevt)
+{
+ QSourceClass *qevtk = QSOURCE_GET_CLASS(qevt);
+ TestEventState *s = qevtk->get_user_data(qevt);
+ int i;
+ bool needs_dispatch = false;
+
+ if (!s->skip_poll) {
+ for (i = 0; i < s->n_poll_fds; i++) {
+ if (s->poll_fds[i].revents & s->poll_fds[i].events) {
+ needs_dispatch = true;
+ }
+ }
+ }
+
+ return needs_dispatch;
+}
+
+static bool test_event_dispatch(QSource *evt)
+{
+ QSourceClass *evtk = QSOURCE_GET_CLASS(evt);
+ QSourceCB cb = evtk->get_callback_func(evt);
+
+ if (cb) {
+ return cb(evt);
+ }
+
+ return true;
+}
+
+static void test_event_finalize(QSource *qsource)
+{
+}
+
+static bool test_cb(QSource *evt)
+{
+ QSourceClass *evtk = QSOURCE_GET_CLASS(evt);
+ TestEventState *s = evtk->get_user_data(evt);
+
+ s->cb_value = CB_VALUE_PASS;
+ s->dispatched = true;
+
+ if (!s->skip_poll) {
+ int i;
+ for (i = 0; i < s->n_poll_fds; i++) {
+ /* unless we short-circuited execution, we should've
+ * only dispatched if the corresponding events we're
+ * listening for were set in the poll() call
+ */
+ if (!(s->poll_fds[i].revents & s->poll_fds[i].events)) {
+ s->cb_value = CB_VALUE_FAIL;
+ }
+ }
+ }
+
+ return true;
+}
+
+QSourceFuncs test_funcs = {
+ test_event_prepare,
+ test_event_check,
+ test_event_dispatch,
+ test_event_finalize,
+};
+
+static void test_qcontext_init(void)
+{
+ Error *err = NULL;
+ GlibQContext *ctx;
+
+ ctx = glib_qcontext_new("test", false, &err);
+ g_assert(!err);
+
+ object_unref(OBJECT(ctx));
+}
+
+static void test_qsource_init(void)
+{
+ QSource *event = qsource_new(test_funcs, test_cb, NULL, NULL);
+
+ object_unref(OBJECT(event));
+}
+
+static void test_qcontext_attach(void)
+{
+ GlibQContext *ctx;
+ QContextClass *ctxk;
+ QSource *evt;
+ Error *err = NULL;
+
+ ctx = glib_qcontext_new("test2", false, &err);
+ if (err) {
+ g_warning("error: %s", error_get_pretty(err));
+ g_assert(0);
+ }
+ ctxk = QCONTEXT_GET_CLASS(QCONTEXT(ctx));
+ evt = qsource_new(test_funcs, test_cb, NULL, NULL);
+
+ ctxk->attach(QCONTEXT(ctx), evt, NULL);
+ ctxk->detach(QCONTEXT(ctx), evt, NULL);
+
+ object_unref(OBJECT(evt));
+ object_unref(OBJECT(ctx));
+}
+
+static void test_qcontext_iterate(void)
+{
+ GlibQContext *ctx;
+ QContextClass *ctxk;
+ QSource *evt;
+ QSourceClass *evtk;
+ Error *err = NULL;
+ EventNotifier notifier1, notifier2;
+ TestEventState s = { 0 };
+
+ /* TODO: generalize this test case to act on any QContext
+ * sub-class so we can re-use it for non-glib implementations
+ */
+ ctx = glib_qcontext_new("test3", false, &err);
+ if (err) {
+ g_warning("error: %s", error_get_pretty(err));
+ g_assert(0);
+ }
+ ctxk = QCONTEXT_GET_CLASS(QCONTEXT(ctx));
+
+ /* test first iteration. glib uses an internal GPollFD to
+ * trigger wake-up when GSources/GPollFDs are added to a
+ * context, so poll may return true even before we add
+ * QSources to the associated GlibQContext. Since this is
+ * an implementation detail of glib we don't explicitly
+ * test for poll() return value here, just the other
+ * interfaces
+ */
+ g_assert(ctxk->prepare(QCONTEXT(ctx), NULL) == false);
+ ctxk->poll(QCONTEXT(ctx), 0);
+ g_assert(ctxk->check(QCONTEXT(ctx)) == false);
+ ctxk->dispatch(QCONTEXT(ctx));
+
+ /* attach some events to the context and initialize
+ * test state to probe callback behavior
+ */
+ event_notifier_init(¬ifier1, false);
+ event_notifier_init(¬ifier2, false);
+ s.poll_fds[0].fd = event_notifier_get_fd(¬ifier1);
+ s.poll_fds[1].fd = event_notifier_get_fd(¬ifier2);
+ s.poll_fds[0].events = s.poll_fds[1].events = G_IO_IN;
+ s.n_poll_fds = 2;
+ s.skip_poll = false;
+ s.dispatched = false;
+
+ evt = qsource_new(test_funcs, test_cb, NULL, &s);
+ evtk = QSOURCE_GET_CLASS(evt);
+ evtk->add_poll(evt, &s.poll_fds[0]);
+ evtk->add_poll(evt, &s.poll_fds[1]);
+ ctxk->attach(QCONTEXT(ctx), evt, NULL);
+
+ /* looping with events attached, but no GPollFD events set */
+ g_assert(ctxk->prepare(QCONTEXT(ctx), NULL) == false);
+ /* poll() should return true when we add events to a QContext */
+ g_assert(ctxk->poll(QCONTEXT(ctx), 0) == true);
+ g_assert(ctxk->check(QCONTEXT(ctx)) == false);
+ ctxk->dispatch(QCONTEXT(ctx));
+ /* no events, so no callbacks should have been dispatched for
+ * our GPollFDs
+ */
+ g_assert(!s.dispatched);
+
+ /* try again with some G_IO_IN events set */
+ event_notifier_set(¬ifier1);
+ event_notifier_set(¬ifier2);
+ s.dispatched = false;
+
+ g_assert(ctxk->prepare(QCONTEXT(ctx), NULL) == false);
+ g_assert(ctxk->poll(QCONTEXT(ctx), 0) == true);
+ g_assert(ctxk->check(QCONTEXT(ctx)) == true);
+ ctxk->dispatch(QCONTEXT(ctx));
+ g_assert(s.dispatched);
+
+ s.dispatched = false;
+
+ /* try again with events cleared */
+ event_notifier_test_and_clear(¬ifier1);
+ event_notifier_test_and_clear(¬ifier2);
+
+ g_assert(ctxk->prepare(QCONTEXT(ctx), NULL) == false);
+ g_assert(ctxk->poll(QCONTEXT(ctx), 0) == true);
+ g_assert(ctxk->check(QCONTEXT(ctx)) == false);
+ ctxk->dispatch(QCONTEXT(ctx));
+ g_assert(!s.dispatched);
+
+ /* try again with short-circuited dispatch */
+ s.skip_poll = true;
+
+ g_assert(ctxk->prepare(QCONTEXT(ctx), NULL) == true);
+ g_assert(ctxk->poll(QCONTEXT(ctx), 0) == false);
+ g_assert(ctxk->check(QCONTEXT(ctx)) == true);
+ ctxk->dispatch(QCONTEXT(ctx));
+ g_assert(s.dispatched);
+ g_assert(s.cb_value == CB_VALUE_PASS);
+ s.skip_poll = false;
+
+ s.dispatched = false;
+
+ /* again with all QSources removed */
+ ctxk->detach(QCONTEXT(ctx), evt, NULL);
+
+ g_assert(ctxk->prepare(QCONTEXT(ctx), NULL) == false);
+ g_assert(ctxk->poll(QCONTEXT(ctx), 0) == true);
+ g_assert(ctxk->check(QCONTEXT(ctx)) == false);
+ ctxk->dispatch(QCONTEXT(ctx));
+ g_assert(!s.dispatched);
+
+ /* cleanup */
+ evtk->remove_poll(evt, &s.poll_fds[0]);
+ evtk->remove_poll(evt, &s.poll_fds[1]);
+
+ object_unref(OBJECT(evt));
+ object_unref(OBJECT(ctx));
+}
+
+int main(int argc, char **argv)
+{
+ module_call_init(MODULE_INIT_QOM);
+
+ g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/qcontext/init", test_qcontext_init);
+ g_test_add_func("/qsource/init", test_qsource_init);
+ g_test_add_func("/qcontext/attach", test_qcontext_attach);
+ g_test_add_func("/qcontext/iterate", test_qcontext_iterate);
+
+ return g_test_run();
+}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (5 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 6/9] QContext: add unit tests Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-06 7:53 ` Paolo Bonzini
2013-05-03 16:03 ` [Qemu-devel] [PATCH 8/9] main-loop: drive main event loop via QContext Michael Roth
` (3 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This introduces a GlibQContext wrapper around the main GMainContext
event loop, and associates iohandlers with it via a QSource (which
GlibQContext creates a GSource from so that it can be driven via
GLib. A subsequent patch will drive the GlibQContext directly)
We also add "QContext-aware" functionality to iohandler interfaces
so that they can be bound to other QContext event loops, and add
non-global set_fd_handler() interfaces to facilitate this. This is made
possible by simply searching a given QContext for a QSource by the name
of "iohandler" so that we can attach event handlers to the associated
IOHandlerState.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
include/qemu/main-loop.h | 31 +++++-
iohandler.c | 238 ++++++++++++++++++++++++++++++++--------------
main-loop.c | 21 +++-
3 files changed, 213 insertions(+), 77 deletions(-)
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 6f0200a..dbadf9f 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -26,6 +26,7 @@
#define QEMU_MAIN_LOOP_H 1
#include "block/aio.h"
+#include "qcontext/qcontext.h"
#define SIG_IPI SIGUSR1
@@ -168,9 +169,24 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque);
/* async I/O support */
+#define QSOURCE_IOHANDLER "iohandler"
+
typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
typedef int IOCanReadHandler(void *opaque);
+QContext *qemu_get_qcontext(void);
+/**
+ * iohandler_attach: Attach a QSource to a QContext
+ *
+ * This enables the use of IOHandler interfaces such as
+ * set_fd_handler() on the given QContext. IOHandler lists will be
+ * tracked/handled/dispatched based on a named QSource that is added to
+ * the QContext
+ *
+ * @ctx: A QContext to add an IOHandler QSource to
+ */
+void iohandler_attach(QContext *ctx);
+
/**
* qemu_set_fd_handler2: Register a file descriptor with the main loop
*
@@ -217,6 +233,13 @@ int qemu_set_fd_handler2(int fd,
IOHandler *fd_write,
void *opaque);
+int set_fd_handler2(QContext *ctx,
+ int fd,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque);
+
/**
* qemu_set_fd_handler: Register a file descriptor with the main loop
*
@@ -250,6 +273,12 @@ int qemu_set_fd_handler(int fd,
IOHandler *fd_write,
void *opaque);
+int set_fd_handler(QContext *ctx,
+ int fd,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque);
+
#ifdef CONFIG_POSIX
/**
* qemu_add_child_watch: Register a child process for reaping.
@@ -302,8 +331,6 @@ void qemu_mutex_unlock_iothread(void);
/* internal interfaces */
void qemu_fd_register(int fd);
-void qemu_iohandler_fill(GArray *pollfds);
-void qemu_iohandler_poll(GArray *pollfds, int rc);
QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
void qemu_bh_schedule_idle(QEMUBH *bh);
diff --git a/iohandler.c b/iohandler.c
index ae2ef8f..8625272 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -41,38 +41,170 @@ typedef struct IOHandlerRecord {
int fd;
int pollfds_idx;
bool deleted;
+ GPollFD pfd;
+ bool pfd_added;
} IOHandlerRecord;
-static QLIST_HEAD(, IOHandlerRecord) io_handlers =
- QLIST_HEAD_INITIALIZER(io_handlers);
+typedef struct IOHandlerState {
+ QLIST_HEAD(, IOHandlerRecord) io_handlers;
+} IOHandlerState;
+static bool iohandler_prepare(QSource *qsource, int *timeout)
+{
+ QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource);
+ IOHandlerState *s = qsourcek->get_user_data(qsource);
+ IOHandlerRecord *ioh;
-/* XXX: fd_read_poll should be suppressed, but an API change is
- necessary in the character devices to suppress fd_can_read(). */
-int qemu_set_fd_handler2(int fd,
- IOCanReadHandler *fd_read_poll,
- IOHandler *fd_read,
- IOHandler *fd_write,
- void *opaque)
+ QLIST_FOREACH(ioh, &s->io_handlers, next) {
+ int events = 0;
+
+ if (ioh->deleted)
+ continue;
+
+ if (ioh->fd_read &&
+ (!ioh->fd_read_poll ||
+ ioh->fd_read_poll(ioh->opaque) != 0)) {
+ events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
+ }
+ if (ioh->fd_write) {
+ events |= G_IO_OUT | G_IO_ERR;
+ }
+ if (events) {
+ ioh->pfd.fd = ioh->fd;
+ ioh->pfd.events = events;
+ if (!ioh->pfd_added) {
+ qsourcek->add_poll(qsource, &ioh->pfd);
+ ioh->pfd_added = true;
+ }
+ } else {
+ ioh->pfd.events = 0;
+ ioh->pfd.revents = 0;
+ }
+ }
+ *timeout = 10;
+ return false;
+}
+
+static bool iohandler_check(QSource *qsource)
{
+ QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
+ IOHandlerState *s = sourcek->get_user_data(qsource);
IOHandlerRecord *ioh;
+ QLIST_FOREACH(ioh, &s->io_handlers, next) {
+ if (ioh->deleted) {
+ continue;
+ }
+ if (ioh->pfd.revents) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+static bool iohandler_dispatch(QSource *qsource)
+{
+ QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
+ IOHandlerState *s = sourcek->get_user_data(qsource);
+ IOHandlerRecord *pioh, *ioh;
+
+ QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
+ int revents = ioh->pfd.revents;
+ if (!ioh->deleted && ioh->fd_read &&
+ (revents && (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
+ ioh->fd_read(ioh->opaque);
+ }
+
+ if (!ioh->deleted && ioh->fd_write &&
+ (revents & (G_IO_OUT | G_IO_ERR))) {
+ ioh->fd_write(ioh->opaque);
+ }
+
+ /* Do this last in case read/write handlers marked it for deletion */
+ if (ioh->deleted) {
+ if (ioh->pfd_added) {
+ sourcek->remove_poll(qsource, &ioh->pfd);
+ }
+ QLIST_REMOVE(ioh, next);
+ g_free(ioh);
+ }
+ }
+
+ return true;
+}
+
+static void iohandler_finalize(QSource *qsource)
+{
+ QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
+ IOHandlerState *s = sourcek->get_user_data(qsource);
+ IOHandlerRecord *pioh, *ioh;
+
+ QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
+ if (ioh->pfd_added) {
+ sourcek->remove_poll(qsource, &ioh->pfd);
+ }
+ QLIST_REMOVE(ioh, next);
+ g_free(ioh);
+ }
+
+ g_free(s);
+}
+
+static const QSourceFuncs iohandler_funcs = {
+ iohandler_prepare,
+ iohandler_check,
+ iohandler_dispatch,
+ iohandler_finalize,
+};
+
+void iohandler_attach(QContext *ctx)
+{
+ IOHandlerState *s;
+ QSource *qsource;
+
+ s = g_new0(IOHandlerState, 1);
+ QLIST_INIT(&s->io_handlers);
+
+ qsource = qsource_new(iohandler_funcs, NULL, QSOURCE_IOHANDLER, s);
+ qcontext_attach(ctx, qsource, NULL);
+}
+
+int set_fd_handler2(QContext *ctx,
+ int fd,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
+{
+ QSourceClass *sourcek;
+ QSource *source;
+ IOHandlerState *s;
+ IOHandlerRecord *ioh;
+
+ source = qcontext_find_source_by_name(ctx, QSOURCE_IOHANDLER);
+ if (!source) {
+ assert(0);
+ }
+ sourcek = QSOURCE_GET_CLASS(source);
+ s = sourcek->get_user_data(source);
+
assert(fd >= 0);
if (!fd_read && !fd_write) {
- QLIST_FOREACH(ioh, &io_handlers, next) {
+ QLIST_FOREACH(ioh, &s->io_handlers, next) {
if (ioh->fd == fd) {
ioh->deleted = 1;
break;
}
}
} else {
- QLIST_FOREACH(ioh, &io_handlers, next) {
+ QLIST_FOREACH(ioh, &s->io_handlers, next) {
if (ioh->fd == fd)
goto found;
}
ioh = g_malloc0(sizeof(IOHandlerRecord));
- QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+ QLIST_INSERT_HEAD(&s->io_handlers, ioh, next);
found:
ioh->fd = fd;
ioh->fd_read_poll = fd_read_poll;
@@ -86,74 +218,34 @@ int qemu_set_fd_handler2(int fd,
return 0;
}
-int qemu_set_fd_handler(int fd,
- IOHandler *fd_read,
- IOHandler *fd_write,
- void *opaque)
+int set_fd_handler(QContext *ctx,
+ int fd,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
{
- return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+ return set_fd_handler2(ctx, fd, NULL, fd_read, fd_write, opaque);
}
-void qemu_iohandler_fill(GArray *pollfds)
+/* XXX: fd_read_poll should be suppressed, but an API change is
+ necessary in the character devices to suppress fd_can_read(). */
+int qemu_set_fd_handler2(int fd,
+ IOCanReadHandler *fd_read_poll,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
{
- IOHandlerRecord *ioh;
-
- QLIST_FOREACH(ioh, &io_handlers, next) {
- int events = 0;
-
- if (ioh->deleted)
- continue;
- if (ioh->fd_read &&
- (!ioh->fd_read_poll ||
- ioh->fd_read_poll(ioh->opaque) != 0)) {
- events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
- }
- if (ioh->fd_write) {
- events |= G_IO_OUT | G_IO_ERR;
- }
- if (events) {
- GPollFD pfd = {
- .fd = ioh->fd,
- .events = events,
- };
- ioh->pollfds_idx = pollfds->len;
- g_array_append_val(pollfds, pfd);
- } else {
- ioh->pollfds_idx = -1;
- }
- }
+ return set_fd_handler2(qemu_get_qcontext(), fd,
+ fd_read_poll, fd_read, fd_write,
+ opaque);
}
-void qemu_iohandler_poll(GArray *pollfds, int ret)
+int qemu_set_fd_handler(int fd,
+ IOHandler *fd_read,
+ IOHandler *fd_write,
+ void *opaque)
{
- if (ret > 0) {
- IOHandlerRecord *pioh, *ioh;
-
- QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
- int revents = 0;
-
- if (!ioh->deleted && ioh->pollfds_idx != -1) {
- GPollFD *pfd = &g_array_index(pollfds, GPollFD,
- ioh->pollfds_idx);
- revents = pfd->revents;
- }
-
- if (!ioh->deleted && ioh->fd_read &&
- (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
- ioh->fd_read(ioh->opaque);
- }
- if (!ioh->deleted && ioh->fd_write &&
- (revents & (G_IO_OUT | G_IO_ERR))) {
- ioh->fd_write(ioh->opaque);
- }
-
- /* Do this last in case read/write handlers marked it for deletion */
- if (ioh->deleted) {
- QLIST_REMOVE(ioh, next);
- g_free(ioh);
- }
- }
- }
+ return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
}
/* reaping of zombies. right now we're not passing the status to
diff --git a/main-loop.c b/main-loop.c
index f46aece..ae284a6 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -27,6 +27,8 @@
#include "slirp/slirp.h"
#include "qemu/main-loop.h"
#include "block/aio.h"
+#include "qcontext/qcontext.h"
+#include "qcontext/glib-qcontext.h"
#ifndef _WIN32
@@ -107,6 +109,13 @@ static int qemu_signal_init(void)
}
#endif
+static QContext *qemu_qcontext;
+
+QContext *qemu_get_qcontext(void)
+{
+ return qemu_qcontext;
+}
+
static AioContext *qemu_aio_context;
AioContext *qemu_get_aio_context(void)
@@ -128,6 +137,7 @@ int qemu_init_main_loop(void)
{
int ret;
GSource *src;
+ Error *err = NULL;
init_clocks();
if (init_timer_alarm() < 0) {
@@ -135,6 +145,15 @@ int qemu_init_main_loop(void)
exit(1);
}
+ qemu_qcontext = QCONTEXT(glib_qcontext_new("main", false, &err));
+ if (err) {
+ g_warning("error initializing main qcontext");
+ error_free(err);
+ return -1;
+ }
+
+ iohandler_attach(qemu_qcontext);
+
ret = qemu_signal_init();
if (ret) {
return ret;
@@ -464,9 +483,7 @@ int main_loop_wait(int nonblocking)
slirp_update_timeout(&timeout);
slirp_pollfds_fill(gpollfds);
#endif
- qemu_iohandler_fill(gpollfds);
ret = os_host_main_loop_wait(timeout);
- qemu_iohandler_poll(gpollfds, ret);
#ifdef CONFIG_SLIRP
slirp_pollfds_poll(gpollfds, (ret < 0));
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource
2013-05-03 16:03 ` [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource Michael Roth
@ 2013-05-06 7:53 ` Paolo Bonzini
2013-05-06 19:03 ` mdroth
2013-08-15 6:07 ` Wenchao Xia
0 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2013-05-06 7:53 UTC (permalink / raw)
To: Michael Roth; +Cc: aliguori, qemu-devel, stefanha, qemulist
Il 03/05/2013 18:03, Michael Roth ha scritto:
> This introduces a GlibQContext wrapper around the main GMainContext
> event loop, and associates iohandlers with it via a QSource (which
> GlibQContext creates a GSource from so that it can be driven via
> GLib. A subsequent patch will drive the GlibQContext directly)
>
> We also add "QContext-aware" functionality to iohandler interfaces
> so that they can be bound to other QContext event loops, and add
> non-global set_fd_handler() interfaces to facilitate this. This is made
> possible by simply searching a given QContext for a QSource by the name
> of "iohandler" so that we can attach event handlers to the associated
> IOHandlerState.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
This patch is why I think that this is a bit overengineered. The main
loop is always glib-based, there should be no need to go through the
QSource abstraction.
BTW, this is broken for Win32. The right thing to do here is to first
convert iohandler to a GSource in such a way that it works for both
POSIX and Win32, and then (if needed) we can later convert GSource to
QSource.
Paolo
> ---
> include/qemu/main-loop.h | 31 +++++-
> iohandler.c | 238 ++++++++++++++++++++++++++++++++--------------
> main-loop.c | 21 +++-
> 3 files changed, 213 insertions(+), 77 deletions(-)
>
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index 6f0200a..dbadf9f 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -26,6 +26,7 @@
> #define QEMU_MAIN_LOOP_H 1
>
> #include "block/aio.h"
> +#include "qcontext/qcontext.h"
>
> #define SIG_IPI SIGUSR1
>
> @@ -168,9 +169,24 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque);
>
> /* async I/O support */
>
> +#define QSOURCE_IOHANDLER "iohandler"
> +
> typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
> typedef int IOCanReadHandler(void *opaque);
>
> +QContext *qemu_get_qcontext(void);
> +/**
> + * iohandler_attach: Attach a QSource to a QContext
> + *
> + * This enables the use of IOHandler interfaces such as
> + * set_fd_handler() on the given QContext. IOHandler lists will be
> + * tracked/handled/dispatched based on a named QSource that is added to
> + * the QContext
> + *
> + * @ctx: A QContext to add an IOHandler QSource to
> + */
> +void iohandler_attach(QContext *ctx);
> +
> /**
> * qemu_set_fd_handler2: Register a file descriptor with the main loop
> *
> @@ -217,6 +233,13 @@ int qemu_set_fd_handler2(int fd,
> IOHandler *fd_write,
> void *opaque);
>
> +int set_fd_handler2(QContext *ctx,
> + int fd,
> + IOCanReadHandler *fd_read_poll,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque);
> +
> /**
> * qemu_set_fd_handler: Register a file descriptor with the main loop
> *
> @@ -250,6 +273,12 @@ int qemu_set_fd_handler(int fd,
> IOHandler *fd_write,
> void *opaque);
>
> +int set_fd_handler(QContext *ctx,
> + int fd,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque);
> +
> #ifdef CONFIG_POSIX
> /**
> * qemu_add_child_watch: Register a child process for reaping.
> @@ -302,8 +331,6 @@ void qemu_mutex_unlock_iothread(void);
> /* internal interfaces */
>
> void qemu_fd_register(int fd);
> -void qemu_iohandler_fill(GArray *pollfds);
> -void qemu_iohandler_poll(GArray *pollfds, int rc);
>
> QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
> void qemu_bh_schedule_idle(QEMUBH *bh);
> diff --git a/iohandler.c b/iohandler.c
> index ae2ef8f..8625272 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -41,38 +41,170 @@ typedef struct IOHandlerRecord {
> int fd;
> int pollfds_idx;
> bool deleted;
> + GPollFD pfd;
> + bool pfd_added;
> } IOHandlerRecord;
>
> -static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> - QLIST_HEAD_INITIALIZER(io_handlers);
> +typedef struct IOHandlerState {
> + QLIST_HEAD(, IOHandlerRecord) io_handlers;
> +} IOHandlerState;
>
> +static bool iohandler_prepare(QSource *qsource, int *timeout)
> +{
> + QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource);
> + IOHandlerState *s = qsourcek->get_user_data(qsource);
> + IOHandlerRecord *ioh;
>
> -/* XXX: fd_read_poll should be suppressed, but an API change is
> - necessary in the character devices to suppress fd_can_read(). */
> -int qemu_set_fd_handler2(int fd,
> - IOCanReadHandler *fd_read_poll,
> - IOHandler *fd_read,
> - IOHandler *fd_write,
> - void *opaque)
> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> + int events = 0;
> +
> + if (ioh->deleted)
> + continue;
> +
> + if (ioh->fd_read &&
> + (!ioh->fd_read_poll ||
> + ioh->fd_read_poll(ioh->opaque) != 0)) {
> + events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
> + }
> + if (ioh->fd_write) {
> + events |= G_IO_OUT | G_IO_ERR;
> + }
> + if (events) {
> + ioh->pfd.fd = ioh->fd;
> + ioh->pfd.events = events;
> + if (!ioh->pfd_added) {
> + qsourcek->add_poll(qsource, &ioh->pfd);
> + ioh->pfd_added = true;
> + }
> + } else {
> + ioh->pfd.events = 0;
> + ioh->pfd.revents = 0;
> + }
> + }
> + *timeout = 10;
> + return false;
> +}
> +
> +static bool iohandler_check(QSource *qsource)
> {
> + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
> + IOHandlerState *s = sourcek->get_user_data(qsource);
> IOHandlerRecord *ioh;
>
> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> + if (ioh->deleted) {
> + continue;
> + }
> + if (ioh->pfd.revents) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static bool iohandler_dispatch(QSource *qsource)
> +{
> + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
> + IOHandlerState *s = sourcek->get_user_data(qsource);
> + IOHandlerRecord *pioh, *ioh;
> +
> + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
> + int revents = ioh->pfd.revents;
> + if (!ioh->deleted && ioh->fd_read &&
> + (revents && (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
> + ioh->fd_read(ioh->opaque);
> + }
> +
> + if (!ioh->deleted && ioh->fd_write &&
> + (revents & (G_IO_OUT | G_IO_ERR))) {
> + ioh->fd_write(ioh->opaque);
> + }
> +
> + /* Do this last in case read/write handlers marked it for deletion */
> + if (ioh->deleted) {
> + if (ioh->pfd_added) {
> + sourcek->remove_poll(qsource, &ioh->pfd);
> + }
> + QLIST_REMOVE(ioh, next);
> + g_free(ioh);
> + }
> + }
> +
> + return true;
> +}
> +
> +static void iohandler_finalize(QSource *qsource)
> +{
> + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
> + IOHandlerState *s = sourcek->get_user_data(qsource);
> + IOHandlerRecord *pioh, *ioh;
> +
> + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
> + if (ioh->pfd_added) {
> + sourcek->remove_poll(qsource, &ioh->pfd);
> + }
> + QLIST_REMOVE(ioh, next);
> + g_free(ioh);
> + }
> +
> + g_free(s);
> +}
> +
> +static const QSourceFuncs iohandler_funcs = {
> + iohandler_prepare,
> + iohandler_check,
> + iohandler_dispatch,
> + iohandler_finalize,
> +};
> +
> +void iohandler_attach(QContext *ctx)
> +{
> + IOHandlerState *s;
> + QSource *qsource;
> +
> + s = g_new0(IOHandlerState, 1);
> + QLIST_INIT(&s->io_handlers);
> +
> + qsource = qsource_new(iohandler_funcs, NULL, QSOURCE_IOHANDLER, s);
> + qcontext_attach(ctx, qsource, NULL);
> +}
> +
> +int set_fd_handler2(QContext *ctx,
> + int fd,
> + IOCanReadHandler *fd_read_poll,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque)
> +{
> + QSourceClass *sourcek;
> + QSource *source;
> + IOHandlerState *s;
> + IOHandlerRecord *ioh;
> +
> + source = qcontext_find_source_by_name(ctx, QSOURCE_IOHANDLER);
> + if (!source) {
> + assert(0);
> + }
> + sourcek = QSOURCE_GET_CLASS(source);
> + s = sourcek->get_user_data(source);
> +
> assert(fd >= 0);
>
> if (!fd_read && !fd_write) {
> - QLIST_FOREACH(ioh, &io_handlers, next) {
> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> if (ioh->fd == fd) {
> ioh->deleted = 1;
> break;
> }
> }
> } else {
> - QLIST_FOREACH(ioh, &io_handlers, next) {
> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> if (ioh->fd == fd)
> goto found;
> }
> ioh = g_malloc0(sizeof(IOHandlerRecord));
> - QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> + QLIST_INSERT_HEAD(&s->io_handlers, ioh, next);
> found:
> ioh->fd = fd;
> ioh->fd_read_poll = fd_read_poll;
> @@ -86,74 +218,34 @@ int qemu_set_fd_handler2(int fd,
> return 0;
> }
>
> -int qemu_set_fd_handler(int fd,
> - IOHandler *fd_read,
> - IOHandler *fd_write,
> - void *opaque)
> +int set_fd_handler(QContext *ctx,
> + int fd,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque)
> {
> - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> + return set_fd_handler2(ctx, fd, NULL, fd_read, fd_write, opaque);
> }
>
> -void qemu_iohandler_fill(GArray *pollfds)
> +/* XXX: fd_read_poll should be suppressed, but an API change is
> + necessary in the character devices to suppress fd_can_read(). */
> +int qemu_set_fd_handler2(int fd,
> + IOCanReadHandler *fd_read_poll,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque)
> {
> - IOHandlerRecord *ioh;
> -
> - QLIST_FOREACH(ioh, &io_handlers, next) {
> - int events = 0;
> -
> - if (ioh->deleted)
> - continue;
> - if (ioh->fd_read &&
> - (!ioh->fd_read_poll ||
> - ioh->fd_read_poll(ioh->opaque) != 0)) {
> - events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
> - }
> - if (ioh->fd_write) {
> - events |= G_IO_OUT | G_IO_ERR;
> - }
> - if (events) {
> - GPollFD pfd = {
> - .fd = ioh->fd,
> - .events = events,
> - };
> - ioh->pollfds_idx = pollfds->len;
> - g_array_append_val(pollfds, pfd);
> - } else {
> - ioh->pollfds_idx = -1;
> - }
> - }
> + return set_fd_handler2(qemu_get_qcontext(), fd,
> + fd_read_poll, fd_read, fd_write,
> + opaque);
> }
>
> -void qemu_iohandler_poll(GArray *pollfds, int ret)
> +int qemu_set_fd_handler(int fd,
> + IOHandler *fd_read,
> + IOHandler *fd_write,
> + void *opaque)
> {
> - if (ret > 0) {
> - IOHandlerRecord *pioh, *ioh;
> -
> - QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> - int revents = 0;
> -
> - if (!ioh->deleted && ioh->pollfds_idx != -1) {
> - GPollFD *pfd = &g_array_index(pollfds, GPollFD,
> - ioh->pollfds_idx);
> - revents = pfd->revents;
> - }
> -
> - if (!ioh->deleted && ioh->fd_read &&
> - (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
> - ioh->fd_read(ioh->opaque);
> - }
> - if (!ioh->deleted && ioh->fd_write &&
> - (revents & (G_IO_OUT | G_IO_ERR))) {
> - ioh->fd_write(ioh->opaque);
> - }
> -
> - /* Do this last in case read/write handlers marked it for deletion */
> - if (ioh->deleted) {
> - QLIST_REMOVE(ioh, next);
> - g_free(ioh);
> - }
> - }
> - }
> + return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> }
>
> /* reaping of zombies. right now we're not passing the status to
> diff --git a/main-loop.c b/main-loop.c
> index f46aece..ae284a6 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -27,6 +27,8 @@
> #include "slirp/slirp.h"
> #include "qemu/main-loop.h"
> #include "block/aio.h"
> +#include "qcontext/qcontext.h"
> +#include "qcontext/glib-qcontext.h"
>
> #ifndef _WIN32
>
> @@ -107,6 +109,13 @@ static int qemu_signal_init(void)
> }
> #endif
>
> +static QContext *qemu_qcontext;
> +
> +QContext *qemu_get_qcontext(void)
> +{
> + return qemu_qcontext;
> +}
> +
> static AioContext *qemu_aio_context;
>
> AioContext *qemu_get_aio_context(void)
> @@ -128,6 +137,7 @@ int qemu_init_main_loop(void)
> {
> int ret;
> GSource *src;
> + Error *err = NULL;
>
> init_clocks();
> if (init_timer_alarm() < 0) {
> @@ -135,6 +145,15 @@ int qemu_init_main_loop(void)
> exit(1);
> }
>
> + qemu_qcontext = QCONTEXT(glib_qcontext_new("main", false, &err));
> + if (err) {
> + g_warning("error initializing main qcontext");
> + error_free(err);
> + return -1;
> + }
> +
> + iohandler_attach(qemu_qcontext);
> +
> ret = qemu_signal_init();
> if (ret) {
> return ret;
> @@ -464,9 +483,7 @@ int main_loop_wait(int nonblocking)
> slirp_update_timeout(&timeout);
> slirp_pollfds_fill(gpollfds);
> #endif
> - qemu_iohandler_fill(gpollfds);
> ret = os_host_main_loop_wait(timeout);
> - qemu_iohandler_poll(gpollfds, ret);
> #ifdef CONFIG_SLIRP
> slirp_pollfds_poll(gpollfds, (ret < 0));
> #endif
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource
2013-05-06 7:53 ` Paolo Bonzini
@ 2013-05-06 19:03 ` mdroth
2013-08-15 6:07 ` Wenchao Xia
1 sibling, 0 replies; 28+ messages in thread
From: mdroth @ 2013-05-06 19:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, stefanha, qemulist
On Mon, May 06, 2013 at 09:53:12AM +0200, Paolo Bonzini wrote:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
> > This introduces a GlibQContext wrapper around the main GMainContext
> > event loop, and associates iohandlers with it via a QSource (which
> > GlibQContext creates a GSource from so that it can be driven via
> > GLib. A subsequent patch will drive the GlibQContext directly)
> >
> > We also add "QContext-aware" functionality to iohandler interfaces
> > so that they can be bound to other QContext event loops, and add
> > non-global set_fd_handler() interfaces to facilitate this. This is made
> > possible by simply searching a given QContext for a QSource by the name
> > of "iohandler" so that we can attach event handlers to the associated
> > IOHandlerState.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> This patch is why I think that this is a bit overengineered. The main
> loop is always glib-based, there should be no need to go through the
> QSource abstraction.
>
> BTW, this is broken for Win32. The right thing to do here is to first
> convert iohandler to a GSource in such a way that it works for both
> POSIX and Win32, and then (if needed) we can later convert GSource to
> QSource.
Yup, forgot to note that Win32 was broken and on my TODO. I'll work on
that and stick to using GSources for now.
>
> Paolo
>
> > ---
> > include/qemu/main-loop.h | 31 +++++-
> > iohandler.c | 238 ++++++++++++++++++++++++++++++++--------------
> > main-loop.c | 21 +++-
> > 3 files changed, 213 insertions(+), 77 deletions(-)
> >
> > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> > index 6f0200a..dbadf9f 100644
> > --- a/include/qemu/main-loop.h
> > +++ b/include/qemu/main-loop.h
> > @@ -26,6 +26,7 @@
> > #define QEMU_MAIN_LOOP_H 1
> >
> > #include "block/aio.h"
> > +#include "qcontext/qcontext.h"
> >
> > #define SIG_IPI SIGUSR1
> >
> > @@ -168,9 +169,24 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque);
> >
> > /* async I/O support */
> >
> > +#define QSOURCE_IOHANDLER "iohandler"
> > +
> > typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
> > typedef int IOCanReadHandler(void *opaque);
> >
> > +QContext *qemu_get_qcontext(void);
> > +/**
> > + * iohandler_attach: Attach a QSource to a QContext
> > + *
> > + * This enables the use of IOHandler interfaces such as
> > + * set_fd_handler() on the given QContext. IOHandler lists will be
> > + * tracked/handled/dispatched based on a named QSource that is added to
> > + * the QContext
> > + *
> > + * @ctx: A QContext to add an IOHandler QSource to
> > + */
> > +void iohandler_attach(QContext *ctx);
> > +
> > /**
> > * qemu_set_fd_handler2: Register a file descriptor with the main loop
> > *
> > @@ -217,6 +233,13 @@ int qemu_set_fd_handler2(int fd,
> > IOHandler *fd_write,
> > void *opaque);
> >
> > +int set_fd_handler2(QContext *ctx,
> > + int fd,
> > + IOCanReadHandler *fd_read_poll,
> > + IOHandler *fd_read,
> > + IOHandler *fd_write,
> > + void *opaque);
> > +
> > /**
> > * qemu_set_fd_handler: Register a file descriptor with the main loop
> > *
> > @@ -250,6 +273,12 @@ int qemu_set_fd_handler(int fd,
> > IOHandler *fd_write,
> > void *opaque);
> >
> > +int set_fd_handler(QContext *ctx,
> > + int fd,
> > + IOHandler *fd_read,
> > + IOHandler *fd_write,
> > + void *opaque);
> > +
> > #ifdef CONFIG_POSIX
> > /**
> > * qemu_add_child_watch: Register a child process for reaping.
> > @@ -302,8 +331,6 @@ void qemu_mutex_unlock_iothread(void);
> > /* internal interfaces */
> >
> > void qemu_fd_register(int fd);
> > -void qemu_iohandler_fill(GArray *pollfds);
> > -void qemu_iohandler_poll(GArray *pollfds, int rc);
> >
> > QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
> > void qemu_bh_schedule_idle(QEMUBH *bh);
> > diff --git a/iohandler.c b/iohandler.c
> > index ae2ef8f..8625272 100644
> > --- a/iohandler.c
> > +++ b/iohandler.c
> > @@ -41,38 +41,170 @@ typedef struct IOHandlerRecord {
> > int fd;
> > int pollfds_idx;
> > bool deleted;
> > + GPollFD pfd;
> > + bool pfd_added;
> > } IOHandlerRecord;
> >
> > -static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> > - QLIST_HEAD_INITIALIZER(io_handlers);
> > +typedef struct IOHandlerState {
> > + QLIST_HEAD(, IOHandlerRecord) io_handlers;
> > +} IOHandlerState;
> >
> > +static bool iohandler_prepare(QSource *qsource, int *timeout)
> > +{
> > + QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource);
> > + IOHandlerState *s = qsourcek->get_user_data(qsource);
> > + IOHandlerRecord *ioh;
> >
> > -/* XXX: fd_read_poll should be suppressed, but an API change is
> > - necessary in the character devices to suppress fd_can_read(). */
> > -int qemu_set_fd_handler2(int fd,
> > - IOCanReadHandler *fd_read_poll,
> > - IOHandler *fd_read,
> > - IOHandler *fd_write,
> > - void *opaque)
> > + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> > + int events = 0;
> > +
> > + if (ioh->deleted)
> > + continue;
> > +
> > + if (ioh->fd_read &&
> > + (!ioh->fd_read_poll ||
> > + ioh->fd_read_poll(ioh->opaque) != 0)) {
> > + events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
> > + }
> > + if (ioh->fd_write) {
> > + events |= G_IO_OUT | G_IO_ERR;
> > + }
> > + if (events) {
> > + ioh->pfd.fd = ioh->fd;
> > + ioh->pfd.events = events;
> > + if (!ioh->pfd_added) {
> > + qsourcek->add_poll(qsource, &ioh->pfd);
> > + ioh->pfd_added = true;
> > + }
> > + } else {
> > + ioh->pfd.events = 0;
> > + ioh->pfd.revents = 0;
> > + }
> > + }
> > + *timeout = 10;
> > + return false;
> > +}
> > +
> > +static bool iohandler_check(QSource *qsource)
> > {
> > + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
> > + IOHandlerState *s = sourcek->get_user_data(qsource);
> > IOHandlerRecord *ioh;
> >
> > + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> > + if (ioh->deleted) {
> > + continue;
> > + }
> > + if (ioh->pfd.revents) {
> > + return true;
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static bool iohandler_dispatch(QSource *qsource)
> > +{
> > + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
> > + IOHandlerState *s = sourcek->get_user_data(qsource);
> > + IOHandlerRecord *pioh, *ioh;
> > +
> > + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
> > + int revents = ioh->pfd.revents;
> > + if (!ioh->deleted && ioh->fd_read &&
> > + (revents && (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
> > + ioh->fd_read(ioh->opaque);
> > + }
> > +
> > + if (!ioh->deleted && ioh->fd_write &&
> > + (revents & (G_IO_OUT | G_IO_ERR))) {
> > + ioh->fd_write(ioh->opaque);
> > + }
> > +
> > + /* Do this last in case read/write handlers marked it for deletion */
> > + if (ioh->deleted) {
> > + if (ioh->pfd_added) {
> > + sourcek->remove_poll(qsource, &ioh->pfd);
> > + }
> > + QLIST_REMOVE(ioh, next);
> > + g_free(ioh);
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static void iohandler_finalize(QSource *qsource)
> > +{
> > + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
> > + IOHandlerState *s = sourcek->get_user_data(qsource);
> > + IOHandlerRecord *pioh, *ioh;
> > +
> > + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
> > + if (ioh->pfd_added) {
> > + sourcek->remove_poll(qsource, &ioh->pfd);
> > + }
> > + QLIST_REMOVE(ioh, next);
> > + g_free(ioh);
> > + }
> > +
> > + g_free(s);
> > +}
> > +
> > +static const QSourceFuncs iohandler_funcs = {
> > + iohandler_prepare,
> > + iohandler_check,
> > + iohandler_dispatch,
> > + iohandler_finalize,
> > +};
> > +
> > +void iohandler_attach(QContext *ctx)
> > +{
> > + IOHandlerState *s;
> > + QSource *qsource;
> > +
> > + s = g_new0(IOHandlerState, 1);
> > + QLIST_INIT(&s->io_handlers);
> > +
> > + qsource = qsource_new(iohandler_funcs, NULL, QSOURCE_IOHANDLER, s);
> > + qcontext_attach(ctx, qsource, NULL);
> > +}
> > +
> > +int set_fd_handler2(QContext *ctx,
> > + int fd,
> > + IOCanReadHandler *fd_read_poll,
> > + IOHandler *fd_read,
> > + IOHandler *fd_write,
> > + void *opaque)
> > +{
> > + QSourceClass *sourcek;
> > + QSource *source;
> > + IOHandlerState *s;
> > + IOHandlerRecord *ioh;
> > +
> > + source = qcontext_find_source_by_name(ctx, QSOURCE_IOHANDLER);
> > + if (!source) {
> > + assert(0);
> > + }
> > + sourcek = QSOURCE_GET_CLASS(source);
> > + s = sourcek->get_user_data(source);
> > +
> > assert(fd >= 0);
> >
> > if (!fd_read && !fd_write) {
> > - QLIST_FOREACH(ioh, &io_handlers, next) {
> > + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> > if (ioh->fd == fd) {
> > ioh->deleted = 1;
> > break;
> > }
> > }
> > } else {
> > - QLIST_FOREACH(ioh, &io_handlers, next) {
> > + QLIST_FOREACH(ioh, &s->io_handlers, next) {
> > if (ioh->fd == fd)
> > goto found;
> > }
> > ioh = g_malloc0(sizeof(IOHandlerRecord));
> > - QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> > + QLIST_INSERT_HEAD(&s->io_handlers, ioh, next);
> > found:
> > ioh->fd = fd;
> > ioh->fd_read_poll = fd_read_poll;
> > @@ -86,74 +218,34 @@ int qemu_set_fd_handler2(int fd,
> > return 0;
> > }
> >
> > -int qemu_set_fd_handler(int fd,
> > - IOHandler *fd_read,
> > - IOHandler *fd_write,
> > - void *opaque)
> > +int set_fd_handler(QContext *ctx,
> > + int fd,
> > + IOHandler *fd_read,
> > + IOHandler *fd_write,
> > + void *opaque)
> > {
> > - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> > + return set_fd_handler2(ctx, fd, NULL, fd_read, fd_write, opaque);
> > }
> >
> > -void qemu_iohandler_fill(GArray *pollfds)
> > +/* XXX: fd_read_poll should be suppressed, but an API change is
> > + necessary in the character devices to suppress fd_can_read(). */
> > +int qemu_set_fd_handler2(int fd,
> > + IOCanReadHandler *fd_read_poll,
> > + IOHandler *fd_read,
> > + IOHandler *fd_write,
> > + void *opaque)
> > {
> > - IOHandlerRecord *ioh;
> > -
> > - QLIST_FOREACH(ioh, &io_handlers, next) {
> > - int events = 0;
> > -
> > - if (ioh->deleted)
> > - continue;
> > - if (ioh->fd_read &&
> > - (!ioh->fd_read_poll ||
> > - ioh->fd_read_poll(ioh->opaque) != 0)) {
> > - events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
> > - }
> > - if (ioh->fd_write) {
> > - events |= G_IO_OUT | G_IO_ERR;
> > - }
> > - if (events) {
> > - GPollFD pfd = {
> > - .fd = ioh->fd,
> > - .events = events,
> > - };
> > - ioh->pollfds_idx = pollfds->len;
> > - g_array_append_val(pollfds, pfd);
> > - } else {
> > - ioh->pollfds_idx = -1;
> > - }
> > - }
> > + return set_fd_handler2(qemu_get_qcontext(), fd,
> > + fd_read_poll, fd_read, fd_write,
> > + opaque);
> > }
> >
> > -void qemu_iohandler_poll(GArray *pollfds, int ret)
> > +int qemu_set_fd_handler(int fd,
> > + IOHandler *fd_read,
> > + IOHandler *fd_write,
> > + void *opaque)
> > {
> > - if (ret > 0) {
> > - IOHandlerRecord *pioh, *ioh;
> > -
> > - QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> > - int revents = 0;
> > -
> > - if (!ioh->deleted && ioh->pollfds_idx != -1) {
> > - GPollFD *pfd = &g_array_index(pollfds, GPollFD,
> > - ioh->pollfds_idx);
> > - revents = pfd->revents;
> > - }
> > -
> > - if (!ioh->deleted && ioh->fd_read &&
> > - (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
> > - ioh->fd_read(ioh->opaque);
> > - }
> > - if (!ioh->deleted && ioh->fd_write &&
> > - (revents & (G_IO_OUT | G_IO_ERR))) {
> > - ioh->fd_write(ioh->opaque);
> > - }
> > -
> > - /* Do this last in case read/write handlers marked it for deletion */
> > - if (ioh->deleted) {
> > - QLIST_REMOVE(ioh, next);
> > - g_free(ioh);
> > - }
> > - }
> > - }
> > + return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> > }
> >
> > /* reaping of zombies. right now we're not passing the status to
> > diff --git a/main-loop.c b/main-loop.c
> > index f46aece..ae284a6 100644
> > --- a/main-loop.c
> > +++ b/main-loop.c
> > @@ -27,6 +27,8 @@
> > #include "slirp/slirp.h"
> > #include "qemu/main-loop.h"
> > #include "block/aio.h"
> > +#include "qcontext/qcontext.h"
> > +#include "qcontext/glib-qcontext.h"
> >
> > #ifndef _WIN32
> >
> > @@ -107,6 +109,13 @@ static int qemu_signal_init(void)
> > }
> > #endif
> >
> > +static QContext *qemu_qcontext;
> > +
> > +QContext *qemu_get_qcontext(void)
> > +{
> > + return qemu_qcontext;
> > +}
> > +
> > static AioContext *qemu_aio_context;
> >
> > AioContext *qemu_get_aio_context(void)
> > @@ -128,6 +137,7 @@ int qemu_init_main_loop(void)
> > {
> > int ret;
> > GSource *src;
> > + Error *err = NULL;
> >
> > init_clocks();
> > if (init_timer_alarm() < 0) {
> > @@ -135,6 +145,15 @@ int qemu_init_main_loop(void)
> > exit(1);
> > }
> >
> > + qemu_qcontext = QCONTEXT(glib_qcontext_new("main", false, &err));
> > + if (err) {
> > + g_warning("error initializing main qcontext");
> > + error_free(err);
> > + return -1;
> > + }
> > +
> > + iohandler_attach(qemu_qcontext);
> > +
> > ret = qemu_signal_init();
> > if (ret) {
> > return ret;
> > @@ -464,9 +483,7 @@ int main_loop_wait(int nonblocking)
> > slirp_update_timeout(&timeout);
> > slirp_pollfds_fill(gpollfds);
> > #endif
> > - qemu_iohandler_fill(gpollfds);
> > ret = os_host_main_loop_wait(timeout);
> > - qemu_iohandler_poll(gpollfds, ret);
> > #ifdef CONFIG_SLIRP
> > slirp_pollfds_poll(gpollfds, (ret < 0));
> > #endif
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource
2013-05-06 7:53 ` Paolo Bonzini
2013-05-06 19:03 ` mdroth
@ 2013-08-15 6:07 ` Wenchao Xia
1 sibling, 0 replies; 28+ messages in thread
From: Wenchao Xia @ 2013-08-15 6:07 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemulist, aliguori, Michael Roth, stefanha, qemu-devel
于 2013-5-6 15:53, Paolo Bonzini 写道:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
>> This introduces a GlibQContext wrapper around the main GMainContext
>> event loop, and associates iohandlers with it via a QSource (which
>> GlibQContext creates a GSource from so that it can be driven via
>> GLib. A subsequent patch will drive the GlibQContext directly)
>>
>> We also add "QContext-aware" functionality to iohandler interfaces
>> so that they can be bound to other QContext event loops, and add
>> non-global set_fd_handler() interfaces to facilitate this. This is made
>> possible by simply searching a given QContext for a QSource by the name
>> of "iohandler" so that we can attach event handlers to the associated
>> IOHandlerState.
>>
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
> This patch is why I think that this is a bit overengineered. The main
> loop is always glib-based, there should be no need to go through the
> QSource abstraction.
>
I thought to use glib-based eventloop before, but found that
AioContext's pending request need to be flushed in release
operation, which can't be done in g_main_context_release. This
brings difficult to do every thing based on glib's API. Do
you think we should stick to QContext to wrap or hide GMainContext?
> BTW, this is broken for Win32. The right thing to do here is to first
> convert iohandler to a GSource in such a way that it works for both
> POSIX and Win32, and then (if needed) we can later convert GSource to
> QSource.
>
> Paolo
>
>> ---
>> include/qemu/main-loop.h | 31 +++++-
>> iohandler.c | 238 ++++++++++++++++++++++++++++++++--------------
>> main-loop.c | 21 +++-
>> 3 files changed, 213 insertions(+), 77 deletions(-)
>>
>> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
>> index 6f0200a..dbadf9f 100644
>> --- a/include/qemu/main-loop.h
>> +++ b/include/qemu/main-loop.h
>> @@ -26,6 +26,7 @@
>> #define QEMU_MAIN_LOOP_H 1
>>
>> #include "block/aio.h"
>> +#include "qcontext/qcontext.h"
>>
>> #define SIG_IPI SIGUSR1
>>
>> @@ -168,9 +169,24 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque);
>>
>> /* async I/O support */
>>
>> +#define QSOURCE_IOHANDLER "iohandler"
>> +
>> typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
>> typedef int IOCanReadHandler(void *opaque);
>>
>> +QContext *qemu_get_qcontext(void);
>> +/**
>> + * iohandler_attach: Attach a QSource to a QContext
>> + *
>> + * This enables the use of IOHandler interfaces such as
>> + * set_fd_handler() on the given QContext. IOHandler lists will be
>> + * tracked/handled/dispatched based on a named QSource that is added to
>> + * the QContext
>> + *
>> + * @ctx: A QContext to add an IOHandler QSource to
>> + */
>> +void iohandler_attach(QContext *ctx);
>> +
>> /**
>> * qemu_set_fd_handler2: Register a file descriptor with the main loop
>> *
>> @@ -217,6 +233,13 @@ int qemu_set_fd_handler2(int fd,
>> IOHandler *fd_write,
>> void *opaque);
>>
>> +int set_fd_handler2(QContext *ctx,
>> + int fd,
>> + IOCanReadHandler *fd_read_poll,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque);
>> +
>> /**
>> * qemu_set_fd_handler: Register a file descriptor with the main loop
>> *
>> @@ -250,6 +273,12 @@ int qemu_set_fd_handler(int fd,
>> IOHandler *fd_write,
>> void *opaque);
>>
>> +int set_fd_handler(QContext *ctx,
>> + int fd,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque);
>> +
>> #ifdef CONFIG_POSIX
>> /**
>> * qemu_add_child_watch: Register a child process for reaping.
>> @@ -302,8 +331,6 @@ void qemu_mutex_unlock_iothread(void);
>> /* internal interfaces */
>>
>> void qemu_fd_register(int fd);
>> -void qemu_iohandler_fill(GArray *pollfds);
>> -void qemu_iohandler_poll(GArray *pollfds, int rc);
>>
>> QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
>> void qemu_bh_schedule_idle(QEMUBH *bh);
>> diff --git a/iohandler.c b/iohandler.c
>> index ae2ef8f..8625272 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -41,38 +41,170 @@ typedef struct IOHandlerRecord {
>> int fd;
>> int pollfds_idx;
>> bool deleted;
>> + GPollFD pfd;
>> + bool pfd_added;
>> } IOHandlerRecord;
>>
>> -static QLIST_HEAD(, IOHandlerRecord) io_handlers =
>> - QLIST_HEAD_INITIALIZER(io_handlers);
>> +typedef struct IOHandlerState {
>> + QLIST_HEAD(, IOHandlerRecord) io_handlers;
>> +} IOHandlerState;
>>
>> +static bool iohandler_prepare(QSource *qsource, int *timeout)
>> +{
>> + QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource);
>> + IOHandlerState *s = qsourcek->get_user_data(qsource);
>> + IOHandlerRecord *ioh;
>>
>> -/* XXX: fd_read_poll should be suppressed, but an API change is
>> - necessary in the character devices to suppress fd_can_read(). */
>> -int qemu_set_fd_handler2(int fd,
>> - IOCanReadHandler *fd_read_poll,
>> - IOHandler *fd_read,
>> - IOHandler *fd_write,
>> - void *opaque)
>> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
>> + int events = 0;
>> +
>> + if (ioh->deleted)
>> + continue;
>> +
>> + if (ioh->fd_read &&
>> + (!ioh->fd_read_poll ||
>> + ioh->fd_read_poll(ioh->opaque) != 0)) {
>> + events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
>> + }
>> + if (ioh->fd_write) {
>> + events |= G_IO_OUT | G_IO_ERR;
>> + }
>> + if (events) {
>> + ioh->pfd.fd = ioh->fd;
>> + ioh->pfd.events = events;
>> + if (!ioh->pfd_added) {
>> + qsourcek->add_poll(qsource, &ioh->pfd);
>> + ioh->pfd_added = true;
>> + }
>> + } else {
>> + ioh->pfd.events = 0;
>> + ioh->pfd.revents = 0;
>> + }
>> + }
>> + *timeout = 10;
>> + return false;
>> +}
>> +
>> +static bool iohandler_check(QSource *qsource)
>> {
>> + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
>> + IOHandlerState *s = sourcek->get_user_data(qsource);
>> IOHandlerRecord *ioh;
>>
>> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
>> + if (ioh->deleted) {
>> + continue;
>> + }
>> + if (ioh->pfd.revents) {
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static bool iohandler_dispatch(QSource *qsource)
>> +{
>> + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
>> + IOHandlerState *s = sourcek->get_user_data(qsource);
>> + IOHandlerRecord *pioh, *ioh;
>> +
>> + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
>> + int revents = ioh->pfd.revents;
>> + if (!ioh->deleted && ioh->fd_read &&
>> + (revents && (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
>> + ioh->fd_read(ioh->opaque);
>> + }
>> +
>> + if (!ioh->deleted && ioh->fd_write &&
>> + (revents & (G_IO_OUT | G_IO_ERR))) {
>> + ioh->fd_write(ioh->opaque);
>> + }
>> +
>> + /* Do this last in case read/write handlers marked it for deletion */
>> + if (ioh->deleted) {
>> + if (ioh->pfd_added) {
>> + sourcek->remove_poll(qsource, &ioh->pfd);
>> + }
>> + QLIST_REMOVE(ioh, next);
>> + g_free(ioh);
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static void iohandler_finalize(QSource *qsource)
>> +{
>> + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource);
>> + IOHandlerState *s = sourcek->get_user_data(qsource);
>> + IOHandlerRecord *pioh, *ioh;
>> +
>> + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) {
>> + if (ioh->pfd_added) {
>> + sourcek->remove_poll(qsource, &ioh->pfd);
>> + }
>> + QLIST_REMOVE(ioh, next);
>> + g_free(ioh);
>> + }
>> +
>> + g_free(s);
>> +}
>> +
>> +static const QSourceFuncs iohandler_funcs = {
>> + iohandler_prepare,
>> + iohandler_check,
>> + iohandler_dispatch,
>> + iohandler_finalize,
>> +};
>> +
>> +void iohandler_attach(QContext *ctx)
>> +{
>> + IOHandlerState *s;
>> + QSource *qsource;
>> +
>> + s = g_new0(IOHandlerState, 1);
>> + QLIST_INIT(&s->io_handlers);
>> +
>> + qsource = qsource_new(iohandler_funcs, NULL, QSOURCE_IOHANDLER, s);
>> + qcontext_attach(ctx, qsource, NULL);
>> +}
>> +
>> +int set_fd_handler2(QContext *ctx,
>> + int fd,
>> + IOCanReadHandler *fd_read_poll,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque)
>> +{
>> + QSourceClass *sourcek;
>> + QSource *source;
>> + IOHandlerState *s;
>> + IOHandlerRecord *ioh;
>> +
>> + source = qcontext_find_source_by_name(ctx, QSOURCE_IOHANDLER);
>> + if (!source) {
>> + assert(0);
>> + }
>> + sourcek = QSOURCE_GET_CLASS(source);
>> + s = sourcek->get_user_data(source);
>> +
>> assert(fd >= 0);
>>
>> if (!fd_read && !fd_write) {
>> - QLIST_FOREACH(ioh, &io_handlers, next) {
>> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
>> if (ioh->fd == fd) {
>> ioh->deleted = 1;
>> break;
>> }
>> }
>> } else {
>> - QLIST_FOREACH(ioh, &io_handlers, next) {
>> + QLIST_FOREACH(ioh, &s->io_handlers, next) {
>> if (ioh->fd == fd)
>> goto found;
>> }
>> ioh = g_malloc0(sizeof(IOHandlerRecord));
>> - QLIST_INSERT_HEAD(&io_handlers, ioh, next);
>> + QLIST_INSERT_HEAD(&s->io_handlers, ioh, next);
>> found:
>> ioh->fd = fd;
>> ioh->fd_read_poll = fd_read_poll;
>> @@ -86,74 +218,34 @@ int qemu_set_fd_handler2(int fd,
>> return 0;
>> }
>>
>> -int qemu_set_fd_handler(int fd,
>> - IOHandler *fd_read,
>> - IOHandler *fd_write,
>> - void *opaque)
>> +int set_fd_handler(QContext *ctx,
>> + int fd,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque)
>> {
>> - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
>> + return set_fd_handler2(ctx, fd, NULL, fd_read, fd_write, opaque);
>> }
>>
>> -void qemu_iohandler_fill(GArray *pollfds)
>> +/* XXX: fd_read_poll should be suppressed, but an API change is
>> + necessary in the character devices to suppress fd_can_read(). */
>> +int qemu_set_fd_handler2(int fd,
>> + IOCanReadHandler *fd_read_poll,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque)
>> {
>> - IOHandlerRecord *ioh;
>> -
>> - QLIST_FOREACH(ioh, &io_handlers, next) {
>> - int events = 0;
>> -
>> - if (ioh->deleted)
>> - continue;
>> - if (ioh->fd_read &&
>> - (!ioh->fd_read_poll ||
>> - ioh->fd_read_poll(ioh->opaque) != 0)) {
>> - events |= G_IO_IN | G_IO_HUP | G_IO_ERR;
>> - }
>> - if (ioh->fd_write) {
>> - events |= G_IO_OUT | G_IO_ERR;
>> - }
>> - if (events) {
>> - GPollFD pfd = {
>> - .fd = ioh->fd,
>> - .events = events,
>> - };
>> - ioh->pollfds_idx = pollfds->len;
>> - g_array_append_val(pollfds, pfd);
>> - } else {
>> - ioh->pollfds_idx = -1;
>> - }
>> - }
>> + return set_fd_handler2(qemu_get_qcontext(), fd,
>> + fd_read_poll, fd_read, fd_write,
>> + opaque);
>> }
>>
>> -void qemu_iohandler_poll(GArray *pollfds, int ret)
>> +int qemu_set_fd_handler(int fd,
>> + IOHandler *fd_read,
>> + IOHandler *fd_write,
>> + void *opaque)
>> {
>> - if (ret > 0) {
>> - IOHandlerRecord *pioh, *ioh;
>> -
>> - QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
>> - int revents = 0;
>> -
>> - if (!ioh->deleted && ioh->pollfds_idx != -1) {
>> - GPollFD *pfd = &g_array_index(pollfds, GPollFD,
>> - ioh->pollfds_idx);
>> - revents = pfd->revents;
>> - }
>> -
>> - if (!ioh->deleted && ioh->fd_read &&
>> - (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
>> - ioh->fd_read(ioh->opaque);
>> - }
>> - if (!ioh->deleted && ioh->fd_write &&
>> - (revents & (G_IO_OUT | G_IO_ERR))) {
>> - ioh->fd_write(ioh->opaque);
>> - }
>> -
>> - /* Do this last in case read/write handlers marked it for deletion */
>> - if (ioh->deleted) {
>> - QLIST_REMOVE(ioh, next);
>> - g_free(ioh);
>> - }
>> - }
>> - }
>> + return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
>> }
>>
>> /* reaping of zombies. right now we're not passing the status to
>> diff --git a/main-loop.c b/main-loop.c
>> index f46aece..ae284a6 100644
>> --- a/main-loop.c
>> +++ b/main-loop.c
>> @@ -27,6 +27,8 @@
>> #include "slirp/slirp.h"
>> #include "qemu/main-loop.h"
>> #include "block/aio.h"
>> +#include "qcontext/qcontext.h"
>> +#include "qcontext/glib-qcontext.h"
>>
>> #ifndef _WIN32
>>
>> @@ -107,6 +109,13 @@ static int qemu_signal_init(void)
>> }
>> #endif
>>
>> +static QContext *qemu_qcontext;
>> +
>> +QContext *qemu_get_qcontext(void)
>> +{
>> + return qemu_qcontext;
>> +}
>> +
>> static AioContext *qemu_aio_context;
>>
>> AioContext *qemu_get_aio_context(void)
>> @@ -128,6 +137,7 @@ int qemu_init_main_loop(void)
>> {
>> int ret;
>> GSource *src;
>> + Error *err = NULL;
>>
>> init_clocks();
>> if (init_timer_alarm() < 0) {
>> @@ -135,6 +145,15 @@ int qemu_init_main_loop(void)
>> exit(1);
>> }
>>
>> + qemu_qcontext = QCONTEXT(glib_qcontext_new("main", false, &err));
>> + if (err) {
>> + g_warning("error initializing main qcontext");
>> + error_free(err);
>> + return -1;
>> + }
>> +
>> + iohandler_attach(qemu_qcontext);
>> +
>> ret = qemu_signal_init();
>> if (ret) {
>> return ret;
>> @@ -464,9 +483,7 @@ int main_loop_wait(int nonblocking)
>> slirp_update_timeout(&timeout);
>> slirp_pollfds_fill(gpollfds);
>> #endif
>> - qemu_iohandler_fill(gpollfds);
>> ret = os_host_main_loop_wait(timeout);
>> - qemu_iohandler_poll(gpollfds, ret);
>> #ifdef CONFIG_SLIRP
>> slirp_pollfds_poll(gpollfds, (ret < 0));
>> #endif
>>
>
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 28+ messages in thread
* [Qemu-devel] [PATCH 8/9] main-loop: drive main event loop via QContext
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (6 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-03 16:03 ` [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread Michael Roth
` (2 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
This series attaches the global AioContext's GSource to the QContext
we've layered on top of the main GMainContext. This, paired with the
similar treatment given to IOHandlers earlier, allows us to drive the
main loop without using fill/poll functions to add/dispatch events that
are external to our main (Glib)QContext.
This patch breaks Slirp, which needs to be re-worked so that it can be
driven via QContext. Patches are on the list to port that to GSource,
so we assume for the purposes of this RFC that that's a solved problem.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
async.c | 16 +++++++++++
include/block/aio.h | 6 +++++
main-loop.c | 75 ++++++++++++++++++---------------------------------
3 files changed, 48 insertions(+), 49 deletions(-)
diff --git a/async.c b/async.c
index 90fe906..a36e2e1 100644
--- a/async.c
+++ b/async.c
@@ -22,10 +22,13 @@
* THE SOFTWARE.
*/
+#include <glib.h>
#include "qemu-common.h"
#include "block/aio.h"
#include "block/thread-pool.h"
#include "qemu/main-loop.h"
+#include "qcontext/qcontext.h"
+#include "qcontext/glib-qcontext.h"
/***********************************************************/
/* bottom halves (can be seen as timers which expire ASAP) */
@@ -219,6 +222,19 @@ AioContext *aio_context_new(void)
return ctx;
}
+void aio_context_attach(AioContext *ctx, QContext *qctx)
+{
+ GSource *aio_gsource = aio_get_g_source(ctx);
+ GMainContext *qctx_main_context =
+ glib_qcontext_get_context(GLIB_QCONTEXT(qctx));
+
+ /* TODO: implement AioContext as a QSource instead of a GSource
+ * so we can attach to any QContext implementation instead of
+ * acting directly on the underlying GMainContext
+ */
+ g_source_attach(aio_gsource, qctx_main_context);
+}
+
void aio_context_ref(AioContext *ctx)
{
g_source_ref(&ctx->source);
diff --git a/include/block/aio.h b/include/block/aio.h
index 1836793..400df54 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -17,6 +17,7 @@
#include "qemu-common.h"
#include "qemu/queue.h"
#include "qemu/event_notifier.h"
+#include "qcontext/qcontext.h"
typedef struct BlockDriverAIOCB BlockDriverAIOCB;
typedef void BlockDriverCompletionFunc(void *opaque, int ret);
@@ -244,4 +245,9 @@ void qemu_aio_set_fd_handler(int fd,
void *opaque);
#endif
+/* Attach an AioContext to a QContext so that it can be driven by a
+ * QContext-based event loop
+ */
+void aio_context_attach(AioContext *ctx, QContext *qctx);
+
#endif
diff --git a/main-loop.c b/main-loop.c
index ae284a6..708581d 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -136,7 +136,6 @@ static GArray *gpollfds;
int qemu_init_main_loop(void)
{
int ret;
- GSource *src;
Error *err = NULL;
init_clocks();
@@ -159,62 +158,37 @@ int qemu_init_main_loop(void)
return ret;
}
+ /* TODO: we can drop gpollfds completely once we integrate
+ * Ping Fan's slirp->glib patches. Until then slirp won't
+ * work since we're no longer driving it via fill/poll
+ * functions
+ */
gpollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD));
qemu_aio_context = aio_context_new();
- src = aio_get_g_source(qemu_aio_context);
- g_source_attach(src, NULL);
- g_source_unref(src);
+ aio_context_attach(qemu_aio_context, qemu_qcontext);
return 0;
}
-static int max_priority;
-
#ifndef _WIN32
-static int glib_pollfds_idx;
-static int glib_n_poll_fds;
-
-static void glib_pollfds_fill(uint32_t *cur_timeout)
-{
- GMainContext *context = g_main_context_default();
- int timeout = 0;
- int n;
-
- g_main_context_prepare(context, &max_priority);
-
- glib_pollfds_idx = gpollfds->len;
- n = glib_n_poll_fds;
- do {
- GPollFD *pfds;
- glib_n_poll_fds = n;
- g_array_set_size(gpollfds, glib_pollfds_idx + glib_n_poll_fds);
- pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
- n = g_main_context_query(context, max_priority, &timeout, pfds,
- glib_n_poll_fds);
- } while (n != glib_n_poll_fds);
-
- if (timeout >= 0 && timeout < *cur_timeout) {
- *cur_timeout = timeout;
- }
-}
-
-static void glib_pollfds_poll(void)
-{
- GMainContext *context = g_main_context_default();
- GPollFD *pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
-
- if (g_main_context_check(context, max_priority, pfds, glib_n_poll_fds)) {
- g_main_context_dispatch(context);
- }
-}
#define MAX_MAIN_LOOP_SPIN (1000)
static int os_host_main_loop_wait(uint32_t timeout)
{
- int ret;
+ QContext *qctx = qemu_get_qcontext();
+ gboolean ret;
+ int calculated_timeout = 0;
+ int events_dispatched = 0;
static int spin_counter;
- glib_pollfds_fill(&timeout);
+ ret = qcontext_prepare(qctx, &calculated_timeout);
+ if (ret) {
+ qcontext_dispatch(qctx);
+ events_dispatched = 1;
+ }
+ if (calculated_timeout >= 0 && calculated_timeout < timeout) {
+ timeout = calculated_timeout;
+ }
/* If the I/O thread is very busy or we are incorrectly busy waiting in
* the I/O thread, this can lead to starvation of the BQL such that the
@@ -241,15 +215,16 @@ static int os_host_main_loop_wait(uint32_t timeout)
} else {
spin_counter++;
}
-
- ret = g_poll((GPollFD *)gpollfds->data, gpollfds->len, timeout);
-
+ ret = qcontext_poll(qctx, timeout);
if (timeout > 0) {
qemu_mutex_lock_iothread();
}
+ if (ret && qcontext_check(qctx)) {
+ qcontext_dispatch(qctx);
+ events_dispatched = 1;
+ }
- glib_pollfds_poll();
- return ret;
+ return events_dispatched;
}
#else
/***********************************************************/
@@ -391,6 +366,8 @@ static void pollfds_poll(GArray *pollfds, int nfds, fd_set *rfds,
}
}
+static int max_priority;
+
static int os_host_main_loop_wait(uint32_t timeout)
{
GMainContext *context = g_main_context_default();
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (7 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 8/9] main-loop: drive main event loop via QContext Michael Roth
@ 2013-05-03 16:03 ` Michael Roth
2013-05-06 7:54 ` Paolo Bonzini
2013-05-06 3:26 ` [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops liu ping fan
2013-05-06 7:54 ` Paolo Bonzini
10 siblings, 1 reply; 28+ messages in thread
From: Michael Roth @ 2013-05-03 16:03 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, aliguori, qemulist, stefanha
virtio-blk dataplane currently creates/manages it's own thread to
offload work to a separate event loop.
This patch insteads allows us to specify a QContext-based event loop by
adding a "context" property for virtio-blk we can use like so:
qemu ... \
-object glib-qcontext,id=ctx0,threaded=yes
-drive file=file.raw,id=drive0,aio=native,cache=none \
-device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
virtio-blk dataplane then simply attachs/detaches it's AioContext to the
ctx0 event loop on start/stop.
This also makes available the option to drive a virtio-blk dataplane via
the default main loop:
qemu ... \
-drive file=file.raw,id=drive0,aio=native,cache=none \
-device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
This doesn't do much in and of itself, but helps to demonstrate how we
might model a general mechanism to offload device workloads to separate
threads.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/block/dataplane/virtio-blk.c | 46 ++++++++++++---------------------------
include/hw/virtio/virtio-blk.h | 7 ++++--
2 files changed, 19 insertions(+), 34 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..08ea10f 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -24,6 +24,8 @@
#include "virtio-blk.h"
#include "block/aio.h"
#include "hw/virtio/virtio-bus.h"
+#include "qcontext/qcontext.h"
+#include "qcontext/glib-qcontext.h"
enum {
SEG_MAX = 126, /* maximum number of I/O segments */
@@ -60,6 +62,7 @@ struct VirtIOBlockDataPlane {
* use it).
*/
AioContext *ctx;
+ QContext *qctx;
EventNotifier io_notifier; /* Linux AIO completion */
EventNotifier host_notifier; /* doorbell */
@@ -375,26 +378,6 @@ static void handle_io(EventNotifier *e)
}
}
-static void *data_plane_thread(void *opaque)
-{
- VirtIOBlockDataPlane *s = opaque;
-
- do {
- aio_poll(s->ctx, true);
- } while (!s->stopping || s->num_reqs > 0);
- return NULL;
-}
-
-static void start_data_plane_bh(void *opaque)
-{
- VirtIOBlockDataPlane *s = opaque;
-
- qemu_bh_delete(s->start_bh);
- s->start_bh = NULL;
- qemu_thread_create(&s->thread, data_plane_thread,
- s, QEMU_THREAD_JOINABLE);
-}
-
bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
VirtIOBlockDataPlane **dataplane)
{
@@ -460,6 +443,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
VirtQueue *vq;
int i;
+ Error *err = NULL;
if (s->started) {
return;
@@ -502,9 +486,16 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
/* Kick right away to begin processing requests already in vring */
event_notifier_set(virtio_queue_get_host_notifier(vq));
- /* Spawn thread in BH so it inherits iothread cpusets */
- s->start_bh = qemu_bh_new(start_data_plane_bh, s);
- qemu_bh_schedule(s->start_bh);
+ /* use QEMU main loop/context by default */
+ if (!s->blk->context) {
+ s->blk->context = g_strdup("main");
+ }
+ s->qctx = qcontext_find_by_name(s->blk->context, &err);
+ if (err) {
+ fprintf(stderr, "virtio-blk failed to start: %s", error_get_pretty(err));
+ exit(1);
+ }
+ aio_context_attach(s->ctx, s->qctx);
}
void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
@@ -517,15 +508,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
s->stopping = true;
trace_virtio_blk_data_plane_stop(s);
- /* Stop thread or cancel pending thread creation BH */
- if (s->start_bh) {
- qemu_bh_delete(s->start_bh);
- s->start_bh = NULL;
- } else {
- aio_notify(s->ctx);
- qemu_thread_join(&s->thread);
- }
-
aio_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL);
ioq_cleanup(&s->ioqueue);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index fc71853..c5514a4 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -110,6 +110,7 @@ struct VirtIOBlkConf
uint32_t scsi;
uint32_t config_wce;
uint32_t data_plane;
+ char *context;
};
struct VirtIOBlockDataPlane;
@@ -138,13 +139,15 @@ typedef struct VirtIOBlock {
DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), \
DEFINE_PROP_STRING("serial", _state, _field.serial), \
DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), \
- DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true)
+ DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true), \
+ DEFINE_PROP_STRING("context", _state, _field.context)
#else
#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field) \
DEFINE_BLOCK_PROPERTIES(_state, _field.conf), \
DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), \
DEFINE_PROP_STRING("serial", _state, _field.serial), \
- DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true)
+ DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), \
+ DEFINE_PROP_STRING("context", _state, _field.context)
#endif /* __linux__ */
void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread
2013-05-03 16:03 ` [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread Michael Roth
@ 2013-05-06 7:54 ` Paolo Bonzini
2013-05-06 19:13 ` mdroth
0 siblings, 1 reply; 28+ messages in thread
From: Paolo Bonzini @ 2013-05-06 7:54 UTC (permalink / raw)
To: Michael Roth; +Cc: aliguori, qemu-devel, stefanha, qemulist
Il 03/05/2013 18:03, Michael Roth ha scritto:
> virtio-blk dataplane currently creates/manages it's own thread to
> offload work to a separate event loop.
>
> This patch insteads allows us to specify a QContext-based event loop by
> adding a "context" property for virtio-blk we can use like so:
>
> qemu ... \
> -object glib-qcontext,id=ctx0,threaded=yes
> -drive file=file.raw,id=drive0,aio=native,cache=none \
> -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
>
> virtio-blk dataplane then simply attachs/detaches it's AioContext to the
> ctx0 event loop on start/stop.
>
> This also makes available the option to drive a virtio-blk dataplane via
> the default main loop:
>
> qemu ... \
> -drive file=file.raw,id=drive0,aio=native,cache=none \
> -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
>
> This doesn't do much in and of itself, but helps to demonstrate how we
> might model a general mechanism to offload device workloads to separate
> threads.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/block/dataplane/virtio-blk.c | 46 ++++++++++++---------------------------
> include/hw/virtio/virtio-blk.h | 7 ++++--
> 2 files changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 0356665..08ea10f 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -24,6 +24,8 @@
> #include "virtio-blk.h"
> #include "block/aio.h"
> #include "hw/virtio/virtio-bus.h"
> +#include "qcontext/qcontext.h"
> +#include "qcontext/glib-qcontext.h"
>
> enum {
> SEG_MAX = 126, /* maximum number of I/O segments */
> @@ -60,6 +62,7 @@ struct VirtIOBlockDataPlane {
> * use it).
> */
> AioContext *ctx;
> + QContext *qctx;
> EventNotifier io_notifier; /* Linux AIO completion */
> EventNotifier host_notifier; /* doorbell */
>
> @@ -375,26 +378,6 @@ static void handle_io(EventNotifier *e)
> }
> }
>
> -static void *data_plane_thread(void *opaque)
> -{
> - VirtIOBlockDataPlane *s = opaque;
> -
> - do {
> - aio_poll(s->ctx, true);
> - } while (!s->stopping || s->num_reqs > 0);
> - return NULL;
> -}
> -
> -static void start_data_plane_bh(void *opaque)
> -{
> - VirtIOBlockDataPlane *s = opaque;
> -
> - qemu_bh_delete(s->start_bh);
> - s->start_bh = NULL;
> - qemu_thread_create(&s->thread, data_plane_thread,
> - s, QEMU_THREAD_JOINABLE);
> -}
> -
> bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
> VirtIOBlockDataPlane **dataplane)
> {
> @@ -460,6 +443,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> VirtQueue *vq;
> int i;
> + Error *err = NULL;
>
> if (s->started) {
> return;
> @@ -502,9 +486,16 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> /* Kick right away to begin processing requests already in vring */
> event_notifier_set(virtio_queue_get_host_notifier(vq));
>
> - /* Spawn thread in BH so it inherits iothread cpusets */
> - s->start_bh = qemu_bh_new(start_data_plane_bh, s);
> - qemu_bh_schedule(s->start_bh);
> + /* use QEMU main loop/context by default */
> + if (!s->blk->context) {
> + s->blk->context = g_strdup("main");
> + }
Or rather create a device-specific context by default?
Paolo
> + s->qctx = qcontext_find_by_name(s->blk->context, &err);
> + if (err) {
> + fprintf(stderr, "virtio-blk failed to start: %s", error_get_pretty(err));
> + exit(1);
> + }
> + aio_context_attach(s->ctx, s->qctx);
> }
>
> void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
> @@ -517,15 +508,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
> s->stopping = true;
> trace_virtio_blk_data_plane_stop(s);
>
> - /* Stop thread or cancel pending thread creation BH */
> - if (s->start_bh) {
> - qemu_bh_delete(s->start_bh);
> - s->start_bh = NULL;
> - } else {
> - aio_notify(s->ctx);
> - qemu_thread_join(&s->thread);
> - }
> -
> aio_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL);
> ioq_cleanup(&s->ioqueue);
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index fc71853..c5514a4 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -110,6 +110,7 @@ struct VirtIOBlkConf
> uint32_t scsi;
> uint32_t config_wce;
> uint32_t data_plane;
> + char *context;
> };
>
> struct VirtIOBlockDataPlane;
> @@ -138,13 +139,15 @@ typedef struct VirtIOBlock {
> DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), \
> DEFINE_PROP_STRING("serial", _state, _field.serial), \
> DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), \
> - DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true)
> + DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true), \
> + DEFINE_PROP_STRING("context", _state, _field.context)
> #else
> #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field) \
> DEFINE_BLOCK_PROPERTIES(_state, _field.conf), \
> DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), \
> DEFINE_PROP_STRING("serial", _state, _field.serial), \
> - DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true)
> + DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), \
> + DEFINE_PROP_STRING("context", _state, _field.context)
> #endif /* __linux__ */
>
> void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread
2013-05-06 7:54 ` Paolo Bonzini
@ 2013-05-06 19:13 ` mdroth
0 siblings, 0 replies; 28+ messages in thread
From: mdroth @ 2013-05-06 19:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, stefanha, qemulist
On Mon, May 06, 2013 at 09:54:03AM +0200, Paolo Bonzini wrote:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
> > virtio-blk dataplane currently creates/manages it's own thread to
> > offload work to a separate event loop.
> >
> > This patch insteads allows us to specify a QContext-based event loop by
> > adding a "context" property for virtio-blk we can use like so:
> >
> > qemu ... \
> > -object glib-qcontext,id=ctx0,threaded=yes
> > -drive file=file.raw,id=drive0,aio=native,cache=none \
> > -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
> >
> > virtio-blk dataplane then simply attachs/detaches it's AioContext to the
> > ctx0 event loop on start/stop.
> >
> > This also makes available the option to drive a virtio-blk dataplane via
> > the default main loop:
> >
> > qemu ... \
> > -drive file=file.raw,id=drive0,aio=native,cache=none \
> > -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
> >
> > This doesn't do much in and of itself, but helps to demonstrate how we
> > might model a general mechanism to offload device workloads to separate
> > threads.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > hw/block/dataplane/virtio-blk.c | 46 ++++++++++++---------------------------
> > include/hw/virtio/virtio-blk.h | 7 ++++--
> > 2 files changed, 19 insertions(+), 34 deletions(-)
> >
> > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > index 0356665..08ea10f 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -24,6 +24,8 @@
> > #include "virtio-blk.h"
> > #include "block/aio.h"
> > #include "hw/virtio/virtio-bus.h"
> > +#include "qcontext/qcontext.h"
> > +#include "qcontext/glib-qcontext.h"
> >
> > enum {
> > SEG_MAX = 126, /* maximum number of I/O segments */
> > @@ -60,6 +62,7 @@ struct VirtIOBlockDataPlane {
> > * use it).
> > */
> > AioContext *ctx;
> > + QContext *qctx;
> > EventNotifier io_notifier; /* Linux AIO completion */
> > EventNotifier host_notifier; /* doorbell */
> >
> > @@ -375,26 +378,6 @@ static void handle_io(EventNotifier *e)
> > }
> > }
> >
> > -static void *data_plane_thread(void *opaque)
> > -{
> > - VirtIOBlockDataPlane *s = opaque;
> > -
> > - do {
> > - aio_poll(s->ctx, true);
> > - } while (!s->stopping || s->num_reqs > 0);
> > - return NULL;
> > -}
> > -
> > -static void start_data_plane_bh(void *opaque)
> > -{
> > - VirtIOBlockDataPlane *s = opaque;
> > -
> > - qemu_bh_delete(s->start_bh);
> > - s->start_bh = NULL;
> > - qemu_thread_create(&s->thread, data_plane_thread,
> > - s, QEMU_THREAD_JOINABLE);
> > -}
> > -
> > bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
> > VirtIOBlockDataPlane **dataplane)
> > {
> > @@ -460,6 +443,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> > VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > VirtQueue *vq;
> > int i;
> > + Error *err = NULL;
> >
> > if (s->started) {
> > return;
> > @@ -502,9 +486,16 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> > /* Kick right away to begin processing requests already in vring */
> > event_notifier_set(virtio_queue_get_host_notifier(vq));
> >
> > - /* Spawn thread in BH so it inherits iothread cpusets */
> > - s->start_bh = qemu_bh_new(start_data_plane_bh, s);
> > - qemu_bh_schedule(s->start_bh);
> > + /* use QEMU main loop/context by default */
> > + if (!s->blk->context) {
> > + s->blk->context = g_strdup("main");
> > + }
>
> Or rather create a device-specific context by default?
Yup, definitely.
I think I did it this way to to give an idea of how a "normal" threaded device
might look (i.e. reworked or written from the start to always be capable of
being driven by a separate event loop)
But x-data-plane=on should imply a new context be used so we don't break
things, and I'll do it that way on the next pass.
>
> Paolo
>
> > + s->qctx = qcontext_find_by_name(s->blk->context, &err);
> > + if (err) {
> > + fprintf(stderr, "virtio-blk failed to start: %s", error_get_pretty(err));
> > + exit(1);
> > + }
> > + aio_context_attach(s->ctx, s->qctx);
> > }
> >
> > void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
> > @@ -517,15 +508,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
> > s->stopping = true;
> > trace_virtio_blk_data_plane_stop(s);
> >
> > - /* Stop thread or cancel pending thread creation BH */
> > - if (s->start_bh) {
> > - qemu_bh_delete(s->start_bh);
> > - s->start_bh = NULL;
> > - } else {
> > - aio_notify(s->ctx);
> > - qemu_thread_join(&s->thread);
> > - }
> > -
> > aio_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL);
> > ioq_cleanup(&s->ioqueue);
> >
> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > index fc71853..c5514a4 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -110,6 +110,7 @@ struct VirtIOBlkConf
> > uint32_t scsi;
> > uint32_t config_wce;
> > uint32_t data_plane;
> > + char *context;
> > };
> >
> > struct VirtIOBlockDataPlane;
> > @@ -138,13 +139,15 @@ typedef struct VirtIOBlock {
> > DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), \
> > DEFINE_PROP_STRING("serial", _state, _field.serial), \
> > DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), \
> > - DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true)
> > + DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true), \
> > + DEFINE_PROP_STRING("context", _state, _field.context)
> > #else
> > #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field) \
> > DEFINE_BLOCK_PROPERTIES(_state, _field.conf), \
> > DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), \
> > DEFINE_PROP_STRING("serial", _state, _field.serial), \
> > - DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true)
> > + DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), \
> > + DEFINE_PROP_STRING("context", _state, _field.context)
> > #endif /* __linux__ */
> >
> > void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (8 preceding siblings ...)
2013-05-03 16:03 ` [Qemu-devel] [PATCH 9/9] dataplane: use a QContext event loop in place of custom thread Michael Roth
@ 2013-05-06 3:26 ` liu ping fan
2013-05-06 18:43 ` mdroth
2013-05-06 7:54 ` Paolo Bonzini
10 siblings, 1 reply; 28+ messages in thread
From: liu ping fan @ 2013-05-06 3:26 UTC (permalink / raw)
To: Michael Roth; +Cc: pbonzini, aliguori, qemu-devel, stefanha
On Sat, May 4, 2013 at 12:03 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> These patches apply on top of qemu.git master, and can also be obtained from:
> git://github.com/mdroth/qemu.git qcontext
>
> OVERVIEW
>
> This series introduces a set of QOM classes/interfaces for event
> registration/handling: QContext and QSource, which are based closely on
> their GMainContext/GSource GLib counterparts.
>
> QContexts can be created via the command-line via -object, and can also be
> intructed (via -object params/properties) to automatically start a
> thread/event-loop to handle QSources we attach to them.
>
> The reference implementation of QContext is GlibQContext, which uses
> GMainContext/GSource interfaces underneath the covers, thus we can
> also attach GSource (and as a result, AioContexts) to it.
>
> As part of this series we also port the QEMU main loop to using QContext
> and drop virtio-blk's dataplane thread in favor of a GlibQContext thread,
> which virtio-blk dataplane attaches to via it's AioContext's GSource.
>
> With these patches in place we can do virtio-blk dataplane assignment
> like so:
>
> qemu ... \
> -object glib-qcontext,id=ctx0,threaded=yes
> -drive file=file.raw,id=drive0,aio=native,cache=none \
> -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
>
> And also gain the ability to assign a virtio-blk dataplane to the default
> QContext driven by the QEMU main iothread:
>
> qemu ... \
> -drive file=file.raw,id=drive0,aio=native,cache=none \
> -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
>
> The latter likely isn't particularly desirable, and the series is in rough
> shape in general, but the goal of this RFC to demonstrate the approach and
> get some feedback on how we might handle thread assignments for things like
> virtio-blk/virtio-net dataplane, and multi-threaded device models general.
>
> Input on this would be greatly appreciated.
>
> BACKGROUND
>
> There has been an outgoing discussion on qemu-devel about what event loop
> interface to consolidate around for virtio-blk dataplane, threaded virtio-net,
> and offloading device workloads to seperate threads in general.
>
> Currently the main candidates seem to be GLib GSources and AioContext, with
> virtio-blk/virtio-net dataplane being the use-cases under consideration.
>
> virtio-blk:
>
> In the case of virtio-blk dataplane, we need to drive virtqueue and AIO events.
> Since AioContext is used extensively throughout the block layer to drive AIO,
> it makes sense to re-use it here as we look toward pushing dataplane
> functionality deeper into the block layer to benefit from things like image
> format support/snapshots/etc.
>
> virtio-net:
>
> In the case of Ping Fan's virtio-net dataplane patches
> (http://thread.gmane.org/gmane.comp.emulators.qemu/196111/focus=196111), we
> need to drive virtqueue and NetClient peer events (such as TAP devices, or
> possibly things like slirp in the future). Currently NetClient events rely on
> IOHandler interfaces such as qemu_set_fd_handler(). These interfaces are global
> ones that rely on a single IOHandler list serviced by QEMU's main loop. An
> effort is currently underway to port these to GSources so that can be more
> easilly attached to other event loops (as opposed to the hooks used for the
> virtio-net dataplane series).
>
> Theoretically, much of the latter (such as TAP devices) can also be done around
> AioContext with some minor changes, but other NetClient backends such as slirp
> lend themselves to more open-ended event loop interfaces like GSources. Other
> devices might also find themselves needing something more open-ended (a somewhat
> silly but present example being virtio-serial + GSource-driven chardev)
>
> QContext:
>
> Since the direction for the forseeable future will likely continue to be
> GSources for some things, AioContext for others, a way to reconcile these
> approaches would be the age-old approach of adding a layer of abstration on
> top of the 2 so that we can handle device/backend thread assignments using
> a general mechanism. Building around this abstration also leaves open the
> ability to deal with things like locking considerations for real-time support
> in the future.
>
> A reasonable start to modeling abstraction layer this would be the open-ended
> GMainContext/GSource approach that QEMU relies on already, which is what
> the QContext/QSource interfaces do (with some minor differences/additions
> such as QSources storing and opaque instead of the GSource-subclassing approach
> for GLib).
>
I think, custom-ed function for readable or not , ex, tap_can_send()
should be passed into QSource, but I failed to find such things in
[PATCH 3/9] QSource: QEMU event source object. Do I miss it?
Thanks and regards,
Pingfan
> TODO/KNOWN ISSUES
>
> - GTK UI causes a crash during certain window refresh events. works fine with
> VNC though, and no problems with other GSources.
> - slirp isn't working (will work with Ping Fan's slirp->GSource conversion)
> - add proper locking
> - integrate timers into a QSource to make timer-event driveable in seperate
> QContext event loops
> - consider a command-line wrapper to -object, such as:
> -context <id>,[threaded=<yes|no>],[backend=<default|glib>]
>
> Makefile.objs | 6 +-
> async.c | 16 +++
> hw/block/dataplane/virtio-blk.c | 46 ++-----
> include/block/aio.h | 6 +
> include/hw/virtio/virtio-blk.h | 7 +-
> include/qcontext/glib-qcontext.h | 59 ++++++++
> include/qcontext/qcontext.h | 76 +++++++++++
> include/qcontext/qsource.h | 63 +++++++++
> include/qemu/main-loop.h | 31 ++++-
> include/qom/object.h | 35 +++++
> iohandler.c | 238 ++++++++++++++++++++++----------
> main-loop.c | 96 ++++++-------
> qcontext/Makefile.objs | 1 +
> qcontext/glib-qcontext.c | 280 ++++++++++++++++++++++++++++++++++++++
> qcontext/qcontext.c | 252 ++++++++++++++++++++++++++++++++++
> qcontext/qsource.c | 143 +++++++++++++++++++
> qom/Makefile.objs | 6 +-
> qom/object.c | 46 +++++++
> tests/Makefile | 7 +
> tests/test-qcontext.c | 259 +++++++++++++++++++++++++++++++++++
> vl.c | 2 +
> 21 files changed, 1512 insertions(+), 163 deletions(-)
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-06 3:26 ` [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops liu ping fan
@ 2013-05-06 18:43 ` mdroth
0 siblings, 0 replies; 28+ messages in thread
From: mdroth @ 2013-05-06 18:43 UTC (permalink / raw)
To: liu ping fan; +Cc: pbonzini, aliguori, qemu-devel, stefanha
On Mon, May 06, 2013 at 11:26:06AM +0800, liu ping fan wrote:
> On Sat, May 4, 2013 at 12:03 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > These patches apply on top of qemu.git master, and can also be obtained from:
> > git://github.com/mdroth/qemu.git qcontext
> >
> > OVERVIEW
> >
> > This series introduces a set of QOM classes/interfaces for event
> > registration/handling: QContext and QSource, which are based closely on
> > their GMainContext/GSource GLib counterparts.
> >
> > QContexts can be created via the command-line via -object, and can also be
> > intructed (via -object params/properties) to automatically start a
> > thread/event-loop to handle QSources we attach to them.
> >
> > The reference implementation of QContext is GlibQContext, which uses
> > GMainContext/GSource interfaces underneath the covers, thus we can
> > also attach GSource (and as a result, AioContexts) to it.
> >
> > As part of this series we also port the QEMU main loop to using QContext
> > and drop virtio-blk's dataplane thread in favor of a GlibQContext thread,
> > which virtio-blk dataplane attaches to via it's AioContext's GSource.
> >
> > With these patches in place we can do virtio-blk dataplane assignment
> > like so:
> >
> > qemu ... \
> > -object glib-qcontext,id=ctx0,threaded=yes
> > -drive file=file.raw,id=drive0,aio=native,cache=none \
> > -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=ctx0
> >
> > And also gain the ability to assign a virtio-blk dataplane to the default
> > QContext driven by the QEMU main iothread:
> >
> > qemu ... \
> > -drive file=file.raw,id=drive0,aio=native,cache=none \
> > -device virtio-blk,drive=drive0,scsi=off,x-data-plane=on,context=main
> >
> > The latter likely isn't particularly desirable, and the series is in rough
> > shape in general, but the goal of this RFC to demonstrate the approach and
> > get some feedback on how we might handle thread assignments for things like
> > virtio-blk/virtio-net dataplane, and multi-threaded device models general.
> >
> > Input on this would be greatly appreciated.
> >
> > BACKGROUND
> >
> > There has been an outgoing discussion on qemu-devel about what event loop
> > interface to consolidate around for virtio-blk dataplane, threaded virtio-net,
> > and offloading device workloads to seperate threads in general.
> >
> > Currently the main candidates seem to be GLib GSources and AioContext, with
> > virtio-blk/virtio-net dataplane being the use-cases under consideration.
> >
> > virtio-blk:
> >
> > In the case of virtio-blk dataplane, we need to drive virtqueue and AIO events.
> > Since AioContext is used extensively throughout the block layer to drive AIO,
> > it makes sense to re-use it here as we look toward pushing dataplane
> > functionality deeper into the block layer to benefit from things like image
> > format support/snapshots/etc.
> >
> > virtio-net:
> >
> > In the case of Ping Fan's virtio-net dataplane patches
> > (http://thread.gmane.org/gmane.comp.emulators.qemu/196111/focus=196111), we
> > need to drive virtqueue and NetClient peer events (such as TAP devices, or
> > possibly things like slirp in the future). Currently NetClient events rely on
> > IOHandler interfaces such as qemu_set_fd_handler(). These interfaces are global
> > ones that rely on a single IOHandler list serviced by QEMU's main loop. An
> > effort is currently underway to port these to GSources so that can be more
> > easilly attached to other event loops (as opposed to the hooks used for the
> > virtio-net dataplane series).
> >
> > Theoretically, much of the latter (such as TAP devices) can also be done around
> > AioContext with some minor changes, but other NetClient backends such as slirp
> > lend themselves to more open-ended event loop interfaces like GSources. Other
> > devices might also find themselves needing something more open-ended (a somewhat
> > silly but present example being virtio-serial + GSource-driven chardev)
> >
> > QContext:
> >
> > Since the direction for the forseeable future will likely continue to be
> > GSources for some things, AioContext for others, a way to reconcile these
> > approaches would be the age-old approach of adding a layer of abstration on
> > top of the 2 so that we can handle device/backend thread assignments using
> > a general mechanism. Building around this abstration also leaves open the
> > ability to deal with things like locking considerations for real-time support
> > in the future.
> >
> > A reasonable start to modeling abstraction layer this would be the open-ended
> > GMainContext/GSource approach that QEMU relies on already, which is what
> > the QContext/QSource interfaces do (with some minor differences/additions
> > such as QSources storing and opaque instead of the GSource-subclassing approach
> > for GLib).
> >
> I think, custom-ed function for readable or not , ex, tap_can_send()
> should be passed into QSource, but I failed to find such things in
> [PATCH 3/9] QSource: QEMU event source object. Do I miss it?
We can actually do that via the prepare function. It gets called prior
to polling the GPollFDs for that QSource/GSource. In the case where
tap_can_send() == false, we'd simply zero out the GPollFD.events for the
tap device in prepare().
One other interesting option this series opens up with regard to
NetClient that currently use qemu_set_fd_handler():
We now have set_fd_handler(ctx, ...) interfaces, which assigns FD handlers
to a specific QContext/event loop. So we don't actually need to convert all
NetClients to GSources to offload handling to a dataplane thread. I
think we'd still want to use a GSource for slirp though.
Not saying that approach is better though, but maybe something to
consider.
>
> Thanks and regards,
> Pingfan
> > TODO/KNOWN ISSUES
> >
> > - GTK UI causes a crash during certain window refresh events. works fine with
> > VNC though, and no problems with other GSources.
> > - slirp isn't working (will work with Ping Fan's slirp->GSource conversion)
> > - add proper locking
> > - integrate timers into a QSource to make timer-event driveable in seperate
> > QContext event loops
> > - consider a command-line wrapper to -object, such as:
> > -context <id>,[threaded=<yes|no>],[backend=<default|glib>]
> >
> > Makefile.objs | 6 +-
> > async.c | 16 +++
> > hw/block/dataplane/virtio-blk.c | 46 ++-----
> > include/block/aio.h | 6 +
> > include/hw/virtio/virtio-blk.h | 7 +-
> > include/qcontext/glib-qcontext.h | 59 ++++++++
> > include/qcontext/qcontext.h | 76 +++++++++++
> > include/qcontext/qsource.h | 63 +++++++++
> > include/qemu/main-loop.h | 31 ++++-
> > include/qom/object.h | 35 +++++
> > iohandler.c | 238 ++++++++++++++++++++++----------
> > main-loop.c | 96 ++++++-------
> > qcontext/Makefile.objs | 1 +
> > qcontext/glib-qcontext.c | 280 ++++++++++++++++++++++++++++++++++++++
> > qcontext/qcontext.c | 252 ++++++++++++++++++++++++++++++++++
> > qcontext/qsource.c | 143 +++++++++++++++++++
> > qom/Makefile.objs | 6 +-
> > qom/object.c | 46 +++++++
> > tests/Makefile | 7 +
> > tests/test-qcontext.c | 259 +++++++++++++++++++++++++++++++++++
> > vl.c | 2 +
> > 21 files changed, 1512 insertions(+), 163 deletions(-)
> >
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-03 16:03 [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops Michael Roth
` (9 preceding siblings ...)
2013-05-06 3:26 ` [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops liu ping fan
@ 2013-05-06 7:54 ` Paolo Bonzini
2013-05-06 12:25 ` Anthony Liguori
2013-05-06 18:17 ` mdroth
10 siblings, 2 replies; 28+ messages in thread
From: Paolo Bonzini @ 2013-05-06 7:54 UTC (permalink / raw)
To: Michael Roth; +Cc: aliguori, qemu-devel, stefanha, qemulist
Il 03/05/2013 18:03, Michael Roth ha scritto:
> These patches apply on top of qemu.git master, and can also be obtained from:
> git://github.com/mdroth/qemu.git qcontext
>
> OVERVIEW
>
> This series introduces a set of QOM classes/interfaces for event
> registration/handling: QContext and QSource, which are based closely on
> their GMainContext/GSource GLib counterparts.
>
> QContexts can be created via the command-line via -object, and can also be
> intructed (via -object params/properties) to automatically start a
> thread/event-loop to handle QSources we attach to them.
This is an awesome idea.
However, it seems a bit overengineered. Why do we need QSource at all?
In my opinion, we should first change dataplane to use AioContext as a
GSource, and benchmark it thoroughly. If it is fast enough, we can
"just" introduce a glib-based QContext and be done with it. Hopefully
that is the case...
Paolo
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-06 7:54 ` Paolo Bonzini
@ 2013-05-06 12:25 ` Anthony Liguori
2013-05-06 18:35 ` mdroth
2013-05-06 18:17 ` mdroth
1 sibling, 1 reply; 28+ messages in thread
From: Anthony Liguori @ 2013-05-06 12:25 UTC (permalink / raw)
To: Paolo Bonzini, Michael Roth; +Cc: qemu-devel, stefanha, qemulist
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
>> These patches apply on top of qemu.git master, and can also be obtained from:
>> git://github.com/mdroth/qemu.git qcontext
>>
>> OVERVIEW
>>
>> This series introduces a set of QOM classes/interfaces for event
>> registration/handling: QContext and QSource, which are based closely on
>> their GMainContext/GSource GLib counterparts.
>>
>> QContexts can be created via the command-line via -object, and can also be
>> intructed (via -object params/properties) to automatically start a
>> thread/event-loop to handle QSources we attach to them.
>
> This is an awesome idea.
Ack.
> However, it seems a bit overengineered.
Ack.
> Why do we need QSource at all?
> In my opinion, we should first change dataplane to use AioContext as a
> GSource, and benchmark it thoroughly. If it is fast enough, we can
> "just" introduce a glib-based QContext and be done with it. Hopefully
> that is the case...
Why even bother with QContext then?
Regards,
Anthony Liguori
>
> Paolo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-06 12:25 ` Anthony Liguori
@ 2013-05-06 18:35 ` mdroth
2013-05-06 20:04 ` Paolo Bonzini
0 siblings, 1 reply; 28+ messages in thread
From: mdroth @ 2013-05-06 18:35 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Paolo Bonzini, qemu-devel, stefanha, qemulist
On Mon, May 06, 2013 at 07:25:24AM -0500, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > Il 03/05/2013 18:03, Michael Roth ha scritto:
> >> These patches apply on top of qemu.git master, and can also be obtained from:
> >> git://github.com/mdroth/qemu.git qcontext
> >>
> >> OVERVIEW
> >>
> >> This series introduces a set of QOM classes/interfaces for event
> >> registration/handling: QContext and QSource, which are based closely on
> >> their GMainContext/GSource GLib counterparts.
> >>
> >> QContexts can be created via the command-line via -object, and can also be
> >> intructed (via -object params/properties) to automatically start a
> >> thread/event-loop to handle QSources we attach to them.
> >
> > This is an awesome idea.
>
> Ack.
>
> > However, it seems a bit overengineered.
>
> Ack.
>
> > Why do we need QSource at all?
> > In my opinion, we should first change dataplane to use AioContext as a
> > GSource, and benchmark it thoroughly. If it is fast enough, we can
> > "just" introduce a glib-based QContext and be done with it. Hopefully
> > that is the case...
>
> Why even bother with QContext then?
The QContext/GlibQContext object in general, or the QContext base class?
In the case of the former, I think a wrapper around GLib that we can
instantiate from the command-line line and query properties like TIDs
from is necessary for robust control over event loops and CPU resources.
We get this essentially for free with QOM, so I think it makes sense to
use it.
In the case of the latter I'm not too sure. Without the QSource
abstraction there isn't much reason not to use the native GLib
interfaces on the underlying GSources/GMainContexts directly. In which
case GlibQContext would only need to be a container of sorts with some
minor additions like spawning an event thread for itself.
If we ever did need to switch it out in favor of a non-GLib
implementation, it should be a mostly mechanical conversion of
GSource->QSource and adding some wrappers around
g_main_context_prepare/check/etc.
Also along that line, if we're taking the approach of not adding
infrastructure/cruft until we actually have a plan to use it, it probably
makes sense to make QContext a concrete class implemented via GLib, and we
can move the GLib stuff to a sub-class later if we ever end up with another
QContext implementation.
Does this seem reasonable?
>
> Regards,
>
> Anthony Liguori
>
> >
> > Paolo
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-06 18:35 ` mdroth
@ 2013-05-06 20:04 ` Paolo Bonzini
0 siblings, 0 replies; 28+ messages in thread
From: Paolo Bonzini @ 2013-05-06 20:04 UTC (permalink / raw)
To: mdroth; +Cc: Anthony Liguori, qemu-devel, stefanha, qemulist
Il 06/05/2013 20:35, mdroth ha scritto:
> In the case of the former, I think a wrapper around GLib that we can
> instantiate from the command-line line and query properties like TIDs
> from is necessary for robust control over event loops and CPU resources.
> We get this essentially for free with QOM, so I think it makes sense to
> use it.
>
> In the case of the latter I'm not too sure. Without the QSource
> abstraction there isn't much reason not to use the native GLib
> interfaces on the underlying GSources/GMainContexts directly. In which
> case GlibQContext would only need to be a container of sorts with some
> minor additions like spawning an event thread for itself.
>
> If we ever did need to switch it out in favor of a non-GLib
> implementation, it should be a mostly mechanical conversion of
> GSource->QSource and adding some wrappers around
> g_main_context_prepare/check/etc.
I'm not sure it is that easy, but I agree entirely with everything else.
> Also along that line, if we're taking the approach of not adding
> infrastructure/cruft until we actually have a plan to use it, it probably
> makes sense to make QContext a concrete class implemented via GLib, and we
> can move the GLib stuff to a sub-class later if we ever end up with another
> QContext implementation.
>
> Does this seem reasonable?
Yes, very much.
Paolo
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-06 7:54 ` Paolo Bonzini
2013-05-06 12:25 ` Anthony Liguori
@ 2013-05-06 18:17 ` mdroth
2013-05-08 11:54 ` Stefan Hajnoczi
1 sibling, 1 reply; 28+ messages in thread
From: mdroth @ 2013-05-06 18:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: aliguori, qemu-devel, stefanha, qemulist
On Mon, May 06, 2013 at 09:54:14AM +0200, Paolo Bonzini wrote:
> Il 03/05/2013 18:03, Michael Roth ha scritto:
> > These patches apply on top of qemu.git master, and can also be obtained from:
> > git://github.com/mdroth/qemu.git qcontext
> >
> > OVERVIEW
> >
> > This series introduces a set of QOM classes/interfaces for event
> > registration/handling: QContext and QSource, which are based closely on
> > their GMainContext/GSource GLib counterparts.
> >
> > QContexts can be created via the command-line via -object, and can also be
> > intructed (via -object params/properties) to automatically start a
> > thread/event-loop to handle QSources we attach to them.
>
> This is an awesome idea.
>
> However, it seems a bit overengineered. Why do we need QSource at all?
> In my opinion, we should first change dataplane to use AioContext as a
> GSource, and benchmark it thoroughly. If it is fast enough, we can
I think it would be great to just stick with GSources. I didn't want to
rely too heavily on GLib for the RFC since there seems to be some
reservations about relying too heavily on GLib for our
OneTrueEventLoop interface (mainly, lack of PI mutexes in the context of
real-time device threads, or other performance considerations that might
pop up and cause us to rethink our use of glib).
However, knowing that we *could* do something like porting to QSources and
using a different QContext implementation if the need ever became
evident is enough for me, and I'm happy to drop QSources until we
actually need them. The GSource->QSource conversions would be mostly
mechanical.
> GSource, and benchmark it thoroughly. If it is fast enough, we can
> "just" introduce a glib-based QContext and be done with it. Hopefully
> that is the case...
Sounds good to me. I'll look into that more, and talk to some of our
performance folks who were involved with the virtio-blk dataplane
testing.
>
> Paolo
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [Qemu-devel] [RFC 0/9] QContext: QOM class to support multiple event loops
2013-05-06 18:17 ` mdroth
@ 2013-05-08 11:54 ` Stefan Hajnoczi
0 siblings, 0 replies; 28+ messages in thread
From: Stefan Hajnoczi @ 2013-05-08 11:54 UTC (permalink / raw)
To: mdroth; +Cc: Paolo Bonzini, aliguori, qemu-devel, qemulist
On Mon, May 06, 2013 at 01:17:53PM -0500, mdroth wrote:
> On Mon, May 06, 2013 at 09:54:14AM +0200, Paolo Bonzini wrote:
> > Il 03/05/2013 18:03, Michael Roth ha scritto:
> > > These patches apply on top of qemu.git master, and can also be obtained from:
> > > git://github.com/mdroth/qemu.git qcontext
> > >
> > > OVERVIEW
> > >
> > > This series introduces a set of QOM classes/interfaces for event
> > > registration/handling: QContext and QSource, which are based closely on
> > > their GMainContext/GSource GLib counterparts.
> > >
> > > QContexts can be created via the command-line via -object, and can also be
> > > intructed (via -object params/properties) to automatically start a
> > > thread/event-loop to handle QSources we attach to them.
> >
> > This is an awesome idea.
> >
> > However, it seems a bit overengineered. Why do we need QSource at all?
> > In my opinion, we should first change dataplane to use AioContext as a
> > GSource, and benchmark it thoroughly. If it is fast enough, we can
>
> I think it would be great to just stick with GSources. I didn't want to
> rely too heavily on GLib for the RFC since there seems to be some
> reservations about relying too heavily on GLib for our
> OneTrueEventLoop interface (mainly, lack of PI mutexes in the context of
> real-time device threads, or other performance considerations that might
> pop up and cause us to rethink our use of glib).
>
> However, knowing that we *could* do something like porting to QSources and
> using a different QContext implementation if the need ever became
> evident is enough for me, and I'm happy to drop QSources until we
> actually need them. The GSource->QSource conversions would be mostly
> mechanical.
>
> > GSource, and benchmark it thoroughly. If it is fast enough, we can
> > "just" introduce a glib-based QContext and be done with it. Hopefully
> > that is the case...
>
> Sounds good to me. I'll look into that more, and talk to some of our
> performance folks who were involved with the virtio-blk dataplane
> testing.
Great. I see value in QOM, it allows event loop threads to be specified
on the command-line and monitor. But it would be nice to drop QSource
as well as the QContext inheritance hierarchy.
BTW there should be a command analogous to query-cpus that lists the
QContexts and their thread IDs. This way CPU affinity can be set
similar to how we do it for vcpu threads today.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread