Re: [patch 4/4] KVM-trace port to tracepoints
* Avi Kivity ([EMAIL PROTECTED]) wrote:
> Peter Zijlstra wrote:
>> On Tue, 2008-07-22 at 21:46 +0300, Avi Kivity wrote:
>>
>>> Jan Kiszka wrote:
>>>
That's true - as long as you don't have to add/remove/modify
tracepoints. I had to do this job in the past (not for KVM). Having 1
spot in 1 file (based on generic probes) would be handier in that case
than 5 spots in 3 files. But if the KVM tracepoints are considered
stable in their number and structure, that shouldn't be an issue here.
>>> Tracepoints aren't stable; they are artefacts of the implementation.
>>>
>>
>> Which IMHO makes it unsuitable for trace_mark() as that will be exported
>> to user-space, and every time you change your tracepoints you'll change
>> user visible things - not nice.
>>
>
> They are used for debugging (mostly performance related), so changes are
> expected.
>
> What uses of trace_mark() depend on a stable interface? blktrace?
>
Actually, LTTng likes to have the { marker name, field name } pairs
unchanged for the markers it looks for, but that's about it. If a
userspace analysis plugin fails to see a marker (because it is disabled
or changed), it just does not apply its particular analysis on the
trace.
Since the markers and marker types are self-described in the trace,
userspace can detect any change the the present markers, so there is no
need to rely on "version numbers" because we are able to proceed to a
complete marker list verification (names, field names, types) before
starting the trace analysis.
Mathieu
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
* Peter Zijlstra ([EMAIL PROTECTED]) wrote:
> On Wed, 2008-07-23 at 12:32 +0300, Avi Kivity wrote:
> > Peter Zijlstra wrote:
> > > There are currently no trace_mark() sites in the kernel that I'm aware
> > > of (except for the scheduler :-/, and those should be converted to
> > > tracepoints ASAP).
> > >
> > > Andrew raised the whole point about trace_mark() generating an
> > > user-visible interface and thus it should be stable, and I agree with
> > > that.
> > >
> > > What that means is that trace_mark() can only be used for really stable
> > > points.
> > >
> > > This in turn means we might as well use trace points.
> > >
> > > Which allows for the conclusion that trace_mark() is not needed and
> > > could be removed from the kernel.
> > >
> > > However - it might be handy for ad-hoc debugging purposes that never see
> > > the light of day (linus' git tree in this case). So on those grounds one
> > > could argue against removing trace_mark
> >
> > But trace_mark() is so wonderful.
>
> I guess tastes differ...
>
> > Can't we just declare the tracemarks
> > as a non-stable interface?
> >
> > Perhaps add an unstable_trace_mark() to make it clear.
>
> At the very least it would need its own output channel. But I'm afraid
> this will be KS material.
>
Hi Peter,
Currently what I have in LTTng includes this output channel. It works
for me, but if I can make it work for others that would be great.
- Tracepoints in kernel code to instrument the kernel.
- LTTng probes connect on those relatively stable tracepoints. They
format the data so it's meaningful to userspace (e.g. extracts the pid
of the prev and next process at sched_switch).
- The LTTng serializer is connected on those markers. It parses the
format string to dynamically reserve space in the relay buffer, write
a timestamp and event ID (one event ID is pre-assigned to a marker
name) and copy the arguments from the stack to the event record (which
has a variable size).
Event IDs and timestamps are added by LTTng, thus not required by
markers. However, one can think of this flow as an efficient and compact
binary data export mechanism to userspace.
Headers exports data type sizes and endianness, a special data channel
exports the mappings between { marker name, ID, format string } so
events are self-described. Therefore, one can add any event he likes and
it will be automatically understood by the tracing toolchain.
If an event is removed or filtered out or modified (by changing its
field name), the userspace trace analyser will detect it and the
specific probe which expects this event will fail to load, leading to
missing analyses, but nothing more than that.
So currently what we would have is, more or less : trace_marks located
within LTTng are kept in sync with userland, but the whole chain also
allows to add "debug-style" trace_marks directly in the kernel code
(this is really useful when trying to perform a low-impact printk-like
runtime bissection of a bug in the kernel code.
I actually see the trace_marks/LTTng combination as a printk which would
extract information in its binary form instead of using text-formatting.
The actual formatting can then be done later, in userland, if ever
needed (many analyses use the raw binary format directly). I guess KS
would be a good opportunity to discuss this interface topic.
Mathieu
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
On Wed, Jul 23, 2008 at 01:08:41PM +0300, Avi Kivity wrote: > trace_mark() is implement kvmtrace, which is propagated to userspace. > So while trace_mark() itself is not a userspace interface, one of its > users is. > > It's an unstable interface. But so is dmesg; that's the nature of tracing. Trace_mark is as stable as any other kernel interface, and the data you pass through it is as stable as you want it to. In most cases like kvmtrace or my spu scheduler tracing code the trace data is directly forwarded through a userspace interface, and that is as stable as any freeform interface, e.g. as like printk mentioned above. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Christoph Hellwig wrote: On Wed, Jul 23, 2008 at 10:55:03AM +0200, Peter Zijlstra wrote: Andrew raised the whole point about trace_mark() generating an user-visible interface and thus it should be stable, and I agree with that. I'm not sure why this comes up again and again, but it's utter bullshit. trace_mark does not generate any kind of user interface, just purely a kernel internal interface. trace_mark() is implement kvmtrace, which is propagated to userspace. So while trace_mark() itself is not a userspace interface, one of its users is. It's an unstable interface. But so is dmesg; that's the nature of tracing. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
On Wed, Jul 23, 2008 at 10:55:03AM +0200, Peter Zijlstra wrote: > Andrew raised the whole point about trace_mark() generating an > user-visible interface and thus it should be stable, and I agree with > that. I'm not sure why this comes up again and again, but it's utter bullshit. trace_mark does not generate any kind of user interface, just purely a kernel internal interface. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
On Wed, 2008-07-23 at 12:32 +0300, Avi Kivity wrote: > Peter Zijlstra wrote: > > There are currently no trace_mark() sites in the kernel that I'm aware > > of (except for the scheduler :-/, and those should be converted to > > tracepoints ASAP). > > > > Andrew raised the whole point about trace_mark() generating an > > user-visible interface and thus it should be stable, and I agree with > > that. > > > > What that means is that trace_mark() can only be used for really stable > > points. > > > > This in turn means we might as well use trace points. > > > > Which allows for the conclusion that trace_mark() is not needed and > > could be removed from the kernel. > > > > However - it might be handy for ad-hoc debugging purposes that never see > > the light of day (linus' git tree in this case). So on those grounds one > > could argue against removing trace_mark > > But trace_mark() is so wonderful. I guess tastes differ... > Can't we just declare the tracemarks > as a non-stable interface? > > Perhaps add an unstable_trace_mark() to make it clear. At the very least it would need its own output channel. But I'm afraid this will be KS material. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Peter Zijlstra wrote: There are currently no trace_mark() sites in the kernel that I'm aware of (except for the scheduler :-/, and those should be converted to tracepoints ASAP). Andrew raised the whole point about trace_mark() generating an user-visible interface and thus it should be stable, and I agree with that. What that means is that trace_mark() can only be used for really stable points. This in turn means we might as well use trace points. Which allows for the conclusion that trace_mark() is not needed and could be removed from the kernel. However - it might be handy for ad-hoc debugging purposes that never see the light of day (linus' git tree in this case). So on those grounds one could argue against removing trace_mark But trace_mark() is so wonderful. Can't we just declare the tracemarks as a non-stable interface? Perhaps add an unstable_trace_mark() to make it clear. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
On Wed, 2008-07-23 at 11:08 +0300, Avi Kivity wrote: > Peter Zijlstra wrote: > > On Tue, 2008-07-22 at 21:46 +0300, Avi Kivity wrote: > > > >> Jan Kiszka wrote: > >> > >>> That's true - as long as you don't have to add/remove/modify > >>> tracepoints. I had to do this job in the past (not for KVM). Having 1 > >>> spot in 1 file (based on generic probes) would be handier in that case > >>> than 5 spots in 3 files. But if the KVM tracepoints are considered > >>> stable in their number and structure, that shouldn't be an issue here. > >>> > >>> > >>> > >> Tracepoints aren't stable; they are artefacts of the implementation. > >> > > > > Which IMHO makes it unsuitable for trace_mark() as that will be exported > > to user-space, and every time you change your tracepoints you'll change > > user visible things - not nice. > > > > They are used for debugging (mostly performance related), so changes are > expected. > > What uses of trace_mark() depend on a stable interface? blktrace? There are currently no trace_mark() sites in the kernel that I'm aware of (except for the scheduler :-/, and those should be converted to tracepoints ASAP). Andrew raised the whole point about trace_mark() generating an user-visible interface and thus it should be stable, and I agree with that. What that means is that trace_mark() can only be used for really stable points. This in turn means we might as well use trace points. Which allows for the conclusion that trace_mark() is not needed and could be removed from the kernel. However - it might be handy for ad-hoc debugging purposes that never see the light of day (linus' git tree in this case). So on those grounds one could argue against removing trace_mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Peter Zijlstra wrote: On Tue, 2008-07-22 at 21:46 +0300, Avi Kivity wrote: Jan Kiszka wrote: That's true - as long as you don't have to add/remove/modify tracepoints. I had to do this job in the past (not for KVM). Having 1 spot in 1 file (based on generic probes) would be handier in that case than 5 spots in 3 files. But if the KVM tracepoints are considered stable in their number and structure, that shouldn't be an issue here. Tracepoints aren't stable; they are artefacts of the implementation. Which IMHO makes it unsuitable for trace_mark() as that will be exported to user-space, and every time you change your tracepoints you'll change user visible things - not nice. They are used for debugging (mostly performance related), so changes are expected. What uses of trace_mark() depend on a stable interface? blktrace? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
On Tue, 2008-07-22 at 21:46 +0300, Avi Kivity wrote: > Jan Kiszka wrote: > > That's true - as long as you don't have to add/remove/modify > > tracepoints. I had to do this job in the past (not for KVM). Having 1 > > spot in 1 file (based on generic probes) would be handier in that case > > than 5 spots in 3 files. But if the KVM tracepoints are considered > > stable in their number and structure, that shouldn't be an issue here. > > > > > > Tracepoints aren't stable; they are artefacts of the implementation. Which IMHO makes it unsuitable for trace_mark() as that will be exported to user-space, and every time you change your tracepoints you'll change user visible things - not nice. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Jan Kiszka wrote: That's true - as long as you don't have to add/remove/modify tracepoints. I had to do this job in the past (not for KVM). Having 1 spot in 1 file (based on generic probes) would be handier in that case than 5 spots in 3 files. But if the KVM tracepoints are considered stable in their number and structure, that shouldn't be an issue here. Tracepoints aren't stable; they are artefacts of the implementation. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Mathieu Desnoyers wrote:
> * Jan Kiszka ([EMAIL PROTECTED]) wrote:
>> Mathieu Desnoyers wrote:
>>> Port/cleanup of KVM-trace to tracepoints.
>>>
>>> Tracepoints allow dormat instrumentation, like the kernel markers, but also
>>> allows to describe the trace points in global headers so they can be easily
>>> managed. They also do not use format strings.
>>>
>>> Anything that would involve an action (dereference a pointer, vmcs read,
>>> ...)
>>> only required when tracing is placed in the probes created in kvm_trace.c
>>>
>>> This patch depends on the "Tracepoints" patch.
>>>
>>> Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]>
>>> CC: 'Peter Zijlstra' <[EMAIL PROTECTED]>
>>> CC: 'Feng(Eric) Liu' <[EMAIL PROTECTED]>
>>> CC: Avi Kivity <[EMAIL PROTECTED]>
>>> CC: kvm@vger.kernel.org
>>> ---
>>> arch/x86/kvm/vmx.c | 38 ++---
>>> arch/x86/kvm/x86.c | 43 ++
>>> include/trace/kvm.h | 83
>>> virt/kvm/kvm_trace.c | 336
>>> +++
>>> 4 files changed, 398 insertions(+), 102 deletions(-)
>> Is it a specific property of KVM-trace that causes this LOC blow-up? Or
>> is this a generic side-effect of tracepoints?
>>
>> [ Hmm, hope I didn't missed too much of the tracepoint discussion... ]
>>
>
> This LOC blow-up is caused by the creation of one probe per
> instrumentation site. So instead of placing the argument setup of
> everything that goes in the trace (0 to 5 u32 arguments) in the kvm
> code, it can be placed separately in a probe object, which could
> eventually be a dynamically loadable module.
>
> The primary objective of tracepoints is to make sure the kernel code
> does not become harder to read because of added instrumentation and to
> provide type-checking at compile-time without needing to put format
> strings into the kernel code, which, to some, looks like debugging code.
> The other aspect it try to address is maintainability of trace points :
> it's much easier to look at all the prototype definitions in
> include/trace/*.h and to manage them (and external tracers which would
> like to connect on those points) than to try to figure out in which C
> files tracing statements has been hidden. We can think of it as a
> standard tracing API providing a more or less stable list of kernel
> tracepoints.
>
> So, while KVMTRACE_?D() statements suits closely kvm-trace
> specificities, it's useless to impose constraints such as splitting
> unsigned longs into two u32 for tracers which can support a wider
> variety of data types.
>
> After refactoring the patch to put the probes in arch/x86/kvm, the
> result is :
>
> arch/x86/kvm/Makefile |1
> arch/x86/kvm/kvm_trace_probes.c | 251
>
> arch/x86/kvm/vmx.c | 38 ++
> arch/x86/kvm/x86.c | 43 ++
> include/asm-x86/kvm_host.h |8 +
> include/trace/kvm.h | 83 +
> virt/kvm/kvm_trace.c| 93 ++
> 7 files changed, 414 insertions(+), 103 deletions(-)
>
> So actually, is it better to have less LOC which looks like this :
>
> KVMTRACE_5D(CPUID, vcpu, function,
> (u32)kvm_register_read(vcpu, VCPU_REGS_RAX),
> (u32)kvm_register_read(vcpu, VCPU_REGS_RBX),
> (u32)kvm_register_read(vcpu, VCPU_REGS_RCX),
> (u32)kvm_register_read(vcpu, VCPU_REGS_RDX), handler);
>
>
> or more LOC looking like this :
>
> include/trace/kvm.h:
> DEFINE_TRACE(kvm_cpuid,
> TPPROTO(struct kvm_vcpu *vcpu, u32 function),
> TPARGS(vcpu, function));
>
> arch/x86/kvm/x86.c:
> trace_kvm_cpuid(vcpu, function);
>
> arch/x86/kvm/kvm_trace_probes.c:
> static void probe_kvm_cpuid(struct kvm_vcpu *vcpu, u32 function)
> {
> kvm_add_trace(KVM_TRC_CPUID, vcpu, 5,
> (u32 []){ function,
> (u32)kvm_register_read(vcpu, VCPU_REGS_RAX),
> (u32)kvm_register_read(vcpu, VCPU_REGS_RBX),
> (u32)kvm_register_read(vcpu, VCPU_REGS_RCX),
> (u32)kvm_register_read(vcpu, VCPU_REGS_RDX) });
> }
>
> int register_kvm_tracepoints(void)
> {
>...
>ret = register_trace_kvm_cpuid(probe_kvm_cpuid);
>WARN_ON(ret);
>...
> }
>
> void unregister_kvm_tracepoints(void)
> {
>...
>unregister_trace_kvm_cpuid(probe_kvm_cpuid);
>...
> }
>
> ?
>
> Notice that only a single line of code is inserted to the kernel code,
> while all the rest sits outsite in a separated probe module. So I think
> it's very important to distinguish between LOC which impair kernel code
> readability and LOC which sit in their own sandbox.
That's true - as long as you don't have to add/remove/modify
tracepoints. I had to do this job in the past (not for KVM). Having 1
spot in 1 file (based on generic probes) would be handier in that case
than 5 spots in 3 files. But if the K
Re: [patch 4/4] KVM-trace port to tracepoints
* Jan Kiszka ([EMAIL PROTECTED]) wrote:
> Mathieu Desnoyers wrote:
> > Port/cleanup of KVM-trace to tracepoints.
> >
> > Tracepoints allow dormat instrumentation, like the kernel markers, but also
> > allows to describe the trace points in global headers so they can be easily
> > managed. They also do not use format strings.
> >
> > Anything that would involve an action (dereference a pointer, vmcs read,
> > ...)
> > only required when tracing is placed in the probes created in kvm_trace.c
> >
> > This patch depends on the "Tracepoints" patch.
> >
> > Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]>
> > CC: 'Peter Zijlstra' <[EMAIL PROTECTED]>
> > CC: 'Feng(Eric) Liu' <[EMAIL PROTECTED]>
> > CC: Avi Kivity <[EMAIL PROTECTED]>
> > CC: kvm@vger.kernel.org
> > ---
> > arch/x86/kvm/vmx.c | 38 ++---
> > arch/x86/kvm/x86.c | 43 ++
> > include/trace/kvm.h | 83
> > virt/kvm/kvm_trace.c | 336
> > +++
> > 4 files changed, 398 insertions(+), 102 deletions(-)
>
> Is it a specific property of KVM-trace that causes this LOC blow-up? Or
> is this a generic side-effect of tracepoints?
>
> [ Hmm, hope I didn't missed too much of the tracepoint discussion... ]
>
This LOC blow-up is caused by the creation of one probe per
instrumentation site. So instead of placing the argument setup of
everything that goes in the trace (0 to 5 u32 arguments) in the kvm
code, it can be placed separately in a probe object, which could
eventually be a dynamically loadable module.
The primary objective of tracepoints is to make sure the kernel code
does not become harder to read because of added instrumentation and to
provide type-checking at compile-time without needing to put format
strings into the kernel code, which, to some, looks like debugging code.
The other aspect it try to address is maintainability of trace points :
it's much easier to look at all the prototype definitions in
include/trace/*.h and to manage them (and external tracers which would
like to connect on those points) than to try to figure out in which C
files tracing statements has been hidden. We can think of it as a
standard tracing API providing a more or less stable list of kernel
tracepoints.
So, while KVMTRACE_?D() statements suits closely kvm-trace
specificities, it's useless to impose constraints such as splitting
unsigned longs into two u32 for tracers which can support a wider
variety of data types.
After refactoring the patch to put the probes in arch/x86/kvm, the
result is :
arch/x86/kvm/Makefile |1
arch/x86/kvm/kvm_trace_probes.c | 251
arch/x86/kvm/vmx.c | 38 ++
arch/x86/kvm/x86.c | 43 ++
include/asm-x86/kvm_host.h |8 +
include/trace/kvm.h | 83 +
virt/kvm/kvm_trace.c| 93 ++
7 files changed, 414 insertions(+), 103 deletions(-)
So actually, is it better to have less LOC which looks like this :
KVMTRACE_5D(CPUID, vcpu, function,
(u32)kvm_register_read(vcpu, VCPU_REGS_RAX),
(u32)kvm_register_read(vcpu, VCPU_REGS_RBX),
(u32)kvm_register_read(vcpu, VCPU_REGS_RCX),
(u32)kvm_register_read(vcpu, VCPU_REGS_RDX), handler);
or more LOC looking like this :
include/trace/kvm.h:
DEFINE_TRACE(kvm_cpuid,
TPPROTO(struct kvm_vcpu *vcpu, u32 function),
TPARGS(vcpu, function));
arch/x86/kvm/x86.c:
trace_kvm_cpuid(vcpu, function);
arch/x86/kvm/kvm_trace_probes.c:
static void probe_kvm_cpuid(struct kvm_vcpu *vcpu, u32 function)
{
kvm_add_trace(KVM_TRC_CPUID, vcpu, 5,
(u32 []){ function,
(u32)kvm_register_read(vcpu, VCPU_REGS_RAX),
(u32)kvm_register_read(vcpu, VCPU_REGS_RBX),
(u32)kvm_register_read(vcpu, VCPU_REGS_RCX),
(u32)kvm_register_read(vcpu, VCPU_REGS_RDX) });
}
int register_kvm_tracepoints(void)
{
...
ret = register_trace_kvm_cpuid(probe_kvm_cpuid);
WARN_ON(ret);
...
}
void unregister_kvm_tracepoints(void)
{
...
unregister_trace_kvm_cpuid(probe_kvm_cpuid);
...
}
?
Notice that only a single line of code is inserted to the kernel code,
while all the rest sits outsite in a separated probe module. So I think
it's very important to distinguish between LOC which impair kernel code
readability and LOC which sit in their own sandbox.
Mathieu
> Jan
>
> --
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
* Anthony Liguori ([EMAIL PROTECTED]) wrote:
> Hi Mathieu,
>
Hi Anthony,
> It's difficult to review your patches because they aren't inlined.
>
> At any rate, this patches are unusable with SVM. They try to execute VT
> instructions unconditionally. For instance, you changed:
>>
>> -KVMTRACE_1D(INTR, vcpu, vmcs_read32(VM_EXIT_INTR_INFO), handler);
>> +trace_kvm_intr(vcpu);
>
> Which lived in VT-specific code (vmx.c)
>
> To:
>
>> +static void probe_kvm_intr(struct kvm_vcpu *vcpu)
>> +{
>> +kvm_add_trace(KVM_TRC_INTR, vcpu, 1,
>> +(u32 []){ vmcs_read32(VM_EXIT_INTR_INFO) });
>> +}
>> +
>
> Which lives in common code (kvm_trace.c). But vmcs_read32() is VT-specific
> and should not be used in common code so this motion is wrong. Why not
> just pass more arguments to probe_kvm_intr()? Then your first two patches
> can be dropped completely.
>
Yes, I just noticed that I made a small mistake : the probe code should
actually go in arch/x86/kvm/kvm_trace_probes.c, which is x86-specific.
The reason why I would try to move the vmcs_read32(VM_EXIT_INTR_INFO) to
the probe code is because, unlike the Markers, when a function call with
potential side-effects is put within the arguments, it's really an
argument to a static inline function. In the markers, since it was a
parameter passed to a macro, I could shuffle it into the branch and it
would not be executed when markers were disabled.
However, we don't have this with tracepoints.
kvm-move-register-read-write-to-system-headers.patch becomes
obsolete since I put the probe code in arch/x86/kvm/.
But it would still be required to move vmcs read and encodings to
headers, either to include/asm-x86 or arch/x86/kvm.
Mathieu
> Regards,
>
> Anthony Liguori
>
> Mathieu Desnoyers wrote:
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Hi Mathieu,
It's difficult to review your patches because they aren't inlined.
At any rate, this patches are unusable with SVM. They try to execute VT
instructions unconditionally. For instance, you changed:
-KVMTRACE_1D(INTR, vcpu, vmcs_read32(VM_EXIT_INTR_INFO), handler);
+trace_kvm_intr(vcpu);
Which lived in VT-specific code (vmx.c)
To:
+static void probe_kvm_intr(struct kvm_vcpu *vcpu)
+{
+kvm_add_trace(KVM_TRC_INTR, vcpu, 1,
+(u32 []){ vmcs_read32(VM_EXIT_INTR_INFO) });
+}
+
Which lives in common code (kvm_trace.c). But vmcs_read32() is
VT-specific and should not be used in common code so this motion is
wrong. Why not just pass more arguments to probe_kvm_intr()? Then your
first two patches can be dropped completely.
Regards,
Anthony Liguori
Mathieu Desnoyers wrote:
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 4/4] KVM-trace port to tracepoints
Mathieu Desnoyers wrote: > Port/cleanup of KVM-trace to tracepoints. > > Tracepoints allow dormat instrumentation, like the kernel markers, but also > allows to describe the trace points in global headers so they can be easily > managed. They also do not use format strings. > > Anything that would involve an action (dereference a pointer, vmcs read, ...) > only required when tracing is placed in the probes created in kvm_trace.c > > This patch depends on the "Tracepoints" patch. > > Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]> > CC: 'Peter Zijlstra' <[EMAIL PROTECTED]> > CC: 'Feng(Eric) Liu' <[EMAIL PROTECTED]> > CC: Avi Kivity <[EMAIL PROTECTED]> > CC: kvm@vger.kernel.org > --- > arch/x86/kvm/vmx.c | 38 ++--- > arch/x86/kvm/x86.c | 43 ++ > include/trace/kvm.h | 83 > virt/kvm/kvm_trace.c | 336 > +++ > 4 files changed, 398 insertions(+), 102 deletions(-) Is it a specific property of KVM-trace that causes this LOC blow-up? Or is this a generic side-effect of tracepoints? [ Hmm, hope I didn't missed too much of the tracepoint discussion... ] Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
