Re: [Qemu-devel] [PATCH 1/2] trace: Add "cpu_init" event

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Thu, Sep 15, 2016 at 02:51:59PM +0200, Lluís Vilanova wrote:
>> Stefan Hajnoczi writes:
>> 
>> > On Wed, Sep 14, 2016 at 06:01:17PM +0200, Lluís Vilanova wrote:
>> >> Stefan Hajnoczi writes:
>> >> 
>> >> > On Tue, Sep 06, 2016 at 04:25:53PM +0200, Lluís Vilanova wrote:
>> >> >> +## vCPU
>> >> >> +
>> >> >> +# Create a new virtual (guest) CPU
>> >> >> +#
>> >> >> +# Targets: all
>> >> >> +guest_cpu_init(void *cpu) "cpu=%p"
>> >> 
>> >> > This isn't a vcpu trace event.  Please add keep it with the other
>> >> > non-vcpu trace events:
>> >> 
>> >> > # cpus.c
>> >> > guest_cpu_init(void *cpu) "cpu=%p"
>> >> 
>> >> It actually is, but as the commit message says, declaring it as such 
>> >> prevents
>> >> the event to be emitted.
>> 
>> > If it cannot be toggled on a per-vcpu basis then claiming it's a vcpu
>> > event doesn't make sense.
>> 
>> >> The culprit of this problem is that new vCPUs start with an empty 
>> >> per-vCPU trace
>> >> event set. Should we make vCPUs "inherit" the state from the global state?
>> >> (i.e., if any vcpu event is set on any vCPU, set it on the new one). The 
>> >> next
>> >> question would then be, should this inheritance only apply until tracing 
>> >> is
>> >> fully initialized of for the whole duration of QEMU?
>> 
>> > I think the underlying issue is that trace_init_vcpu_events() assumes
>> > there is a single instant where vcpus all exist and need to be
>> > initialized.
>> 
>> > A model that supports vcpu hotplug is really needed.  In that model the
>> > global dstate should be the "global" state that determines whether vcpus
>> > are initialized with the event enabled or disabled.
>> 
>> Ok, so then you're going for what I was saying. "Inheriting" the state of 
>> every
>> new vCPU from the global dynamic state. If *any* vCPU has an event enabled, 
>> all
>> new vCPUs will too.

> No, I was thinking of something slightly different:

> Right now vcpu trace events use the global dstate during intialization
> only.  After trace_init_vcpu_events() their dstate is zeroed.

> What if we leave the global dstate and use it as the template for
> enabling events in new vCPUs (hotplugged or static)?

I think we both mean the same. The global dstate is not actually zeroed after
trace_init_vcpu_events(). It is zeroed and *then*, iff it was set, set again for
that vCPU. The end result is that the global dstate (for vcpu events) counts how
many vCPUs have that event enabled.

What I mean by "inheriting if any vCPU has the event enabled", is actually check
if that counter is greater than zero. If so, then enable it for the new vCPU.

The only corner case is moving the logic in trace_init_vcpu_events() (zeroing
and setting again if it was set) into vCPU creation iff we're the first vCPU
created on the system.


>> As I said, next question is whether inheritance should apply only until full
>> initialization, or the whole time. I thing the latter is less confusing.

> I agree that it should apply the whole time.

Right. I just wrote the hotplugging path, but could not find where is the path
for hot-unplugging, if it exists.

The wiki [1] seems to point that unplugging is not yet supported, but it might
be outdated. Can anybody shed some light onto this?

[1] http://wiki.qemu.org/Features/CPUHotplug


Thanks,
  Lluis