| Message ID | 20170606181948.16238-13-rkagan@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 06, 2017 at 09:19:37PM +0300, Roman Kagan wrote: > Multiple entities (e.g. VMBus devices) can use the same SINT route. To > make their lives easier in maintaining SINT route ownership, make it > reference-counted. Adjust the respective API names accordingly. > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> Isn't it easier to reuse existing refcounting infrastructure here? Is it overkill to make it a QOM object? (struct Object has 40 bytes)
On Wed, Jun 14, 2017 at 10:53:25AM -0300, Eduardo Habkost wrote: > On Tue, Jun 06, 2017 at 09:19:37PM +0300, Roman Kagan wrote: > > Multiple entities (e.g. VMBus devices) can use the same SINT route. To > > make their lives easier in maintaining SINT route ownership, make it > > reference-counted. Adjust the respective API names accordingly. > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > Isn't it easier to reuse existing refcounting infrastructure > here? Is it overkill to make it a QOM object? (struct Object has > 40 bytes) Normally the guests use a sint route per cpu or less. So no, the space overhead is not an issue. I also wanted to reuse regular QOM refcounting so I QOM-ified it at first. However, while hammering out the design, I found no appropriate place in the QOM hierachy where to stick these objects so we dropped the idea. If I get your proposal right you suggest to leave it unattached instead. That should probably work; however, looking at all the boilerplate code this would entail, including OBJECT casts, I'm not sure it would save anything. Do you think it's worth reworking into QOM? Thanks, Roman.
On Wed, Jun 14, 2017 at 07:23:56PM +0300, Roman Kagan wrote: > On Wed, Jun 14, 2017 at 10:53:25AM -0300, Eduardo Habkost wrote: > > On Tue, Jun 06, 2017 at 09:19:37PM +0300, Roman Kagan wrote: > > > Multiple entities (e.g. VMBus devices) can use the same SINT route. To > > > make their lives easier in maintaining SINT route ownership, make it > > > reference-counted. Adjust the respective API names accordingly. > > > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > > > Isn't it easier to reuse existing refcounting infrastructure > > here? Is it overkill to make it a QOM object? (struct Object has > > 40 bytes) > > Normally the guests use a sint route per cpu or less. So no, the space > overhead is not an issue. > > I also wanted to reuse regular QOM refcounting so I QOM-ified it at > first. However, while hammering out the design, I found no appropriate > place in the QOM hierachy where to stick these objects so we dropped the > idea. > > If I get your proposal right you suggest to leave it unattached instead. > That should probably work; however, looking at all the boilerplate code > this would entail, including OBJECT casts, I'm not sure it would save > anything. Do you think it's worth reworking into QOM? I just noticed I didn't reply to this, sorry. I'm also not sure it is worth reworking into QOM, so I guess it's up to you. About placing the object in the QOM hierarchy, I don't think that would be a problem. Objects without a parent are safer, as long as reference counting is properly tracked. About the OBJECT casts, I guess that's a common issue. I'm considering proposing making object_ref()/object_unref() macros that use OBJECT() automatically.
diff --git a/target/i386/hyperv.h b/target/i386/hyperv.h index bc071da..ca5a32d 100644 --- a/target/i386/hyperv.h +++ b/target/i386/hyperv.h @@ -23,11 +23,11 @@ typedef void (*HvSintAckClb)(void *data); int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit); -HvSintRoute *kvm_hv_sint_route_create(X86CPU *cpu, uint32_t sint, - HvSintAckClb sint_ack_clb, - void *sint_ack_clb_data); - -void kvm_hv_sint_route_destroy(HvSintRoute *sint_route); +HvSintRoute *hyperv_sint_route_new(X86CPU *cpu, uint32_t sint, + HvSintAckClb sint_ack_clb, + void *sint_ack_clb_data); +void hyperv_sint_route_ref(HvSintRoute *sint_route); +void hyperv_sint_route_unref(HvSintRoute *sint_route); int kvm_hv_sint_route_set_sint(HvSintRoute *sint_route); diff --git a/hw/misc/hyperv_testdev.c b/hw/misc/hyperv_testdev.c index 1b12295..929bf4f 100644 --- a/hw/misc/hyperv_testdev.c +++ b/hw/misc/hyperv_testdev.c @@ -55,7 +55,7 @@ static void sint_route_create(HypervTestDev *dev, X86CPU *cpu, uint8_t sint) sint_route->cpu = cpu; sint_route->sint = sint; - sint_route->sint_route = kvm_hv_sint_route_create(cpu, sint, NULL, NULL); + sint_route->sint_route = hyperv_sint_route_new(cpu, sint, NULL, NULL); assert(sint_route->sint_route); QLIST_INSERT_HEAD(&dev->sint_routes, sint_route, le); @@ -81,7 +81,7 @@ static void sint_route_destroy(HypervTestDev *dev, X86CPU *cpu, uint8_t sint) sint_route = sint_route_find(dev, cpu, sint); QLIST_REMOVE(sint_route, le); - kvm_hv_sint_route_destroy(sint_route->sint_route); + hyperv_sint_route_unref(sint_route->sint_route); g_free(sint_route); } diff --git a/target/i386/hyperv.c b/target/i386/hyperv.c index a150401..ae67f82 100644 --- a/target/i386/hyperv.c +++ b/target/i386/hyperv.c @@ -24,6 +24,7 @@ struct HvSintRoute { EventNotifier sint_ack_notifier; HvSintAckClb sint_ack_clb; void *sint_ack_clb_data; + unsigned refcount; }; uint32_t hyperv_vp_index(X86CPU *cpu) @@ -90,9 +91,9 @@ static void kvm_hv_sint_ack_handler(EventNotifier *notifier) sint_route->sint_ack_clb(sint_route->sint_ack_clb_data); } -HvSintRoute *kvm_hv_sint_route_create(X86CPU *cpu, uint32_t sint, - HvSintAckClb sint_ack_clb, - void *sint_ack_clb_data) +HvSintRoute *hyperv_sint_route_new(X86CPU *cpu, uint32_t sint, + HvSintAckClb sint_ack_clb, + void *sint_ack_clb_data) { HvSintRoute *sint_route; EventNotifier *ack_notifier; @@ -130,6 +131,7 @@ HvSintRoute *kvm_hv_sint_route_create(X86CPU *cpu, uint32_t sint, sint_route->sint_ack_clb_data = sint_ack_clb_data; sint_route->cpu = cpu; sint_route->sint = sint; + sint_route->refcount = 1; return sint_route; @@ -148,8 +150,23 @@ err: return NULL; } -void kvm_hv_sint_route_destroy(HvSintRoute *sint_route) +void hyperv_sint_route_ref(HvSintRoute *sint_route) { + sint_route->refcount++; +} + +void hyperv_sint_route_unref(HvSintRoute *sint_route) +{ + if (!sint_route) { + return; + } + + assert(sint_route->refcount > 0); + + if (--sint_route->refcount) { + return; + } + kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, &sint_route->sint_set_notifier, sint_route->gsi);
Multiple entities (e.g. VMBus devices) can use the same SINT route. To make their lives easier in maintaining SINT route ownership, make it reference-counted. Adjust the respective API names accordingly. Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- target/i386/hyperv.h | 10 +++++----- hw/misc/hyperv_testdev.c | 4 ++-- target/i386/hyperv.c | 25 +++++++++++++++++++++---- 3 files changed, 28 insertions(+), 11 deletions(-)