diff mbox

Fix: RHEL 7.2 kvm instrumentation

Message ID CABp_NWB7SQ9zz8F9UhKJjD_a274OBDjOvQ78Yvm-Hs1N_7zxLw@mail.gmail.com
State Rejected, archived
Headers show

Commit Message

Mohamad Gebai June 27, 2016, 3:26 p.m. UTC
Signed-off-by: Mohamad Gebai <mohamad.gebai at gmail.com>
---
 instrumentation/events/lttng-module/arch/x86/kvm/trace.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

                ctf_integer(__u32, csbase,
kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS))
                ctf_integer(__u8, len, vcpu->arch.emulate_ctxt._eip

Comments

Mathieu Desnoyers June 27, 2016, 4:13 p.m. UTC | #1
----- On Jun 27, 2016, at 11:26 AM, mo geb <mohamad.gebai at gmail.com> wrote: 

Please use [PATCH lttng-modules] in the subject. 

> Signed-off-by: Mohamad Gebai < mohamad.gebai at gmail.com >
> ---
> instrumentation/events/lttng-module/arch/x86/kvm/trace.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

> diff --git a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> index 1282bea..ef3d8aa 100644
> --- a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> +++ b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> @@ -429,7 +429,8 @@ LTTNG_TRACEPOINT_EVENT(kvm_emulate_insn,
> ctf_integer(__u8, len, vcpu->arch.emulate_ctxt.decode.eip
> - vcpu->arch.emulate_ctxt.decode.fetch.start)
> ctf_array(__u8, insn, vcpu->arch.emulate_ctxt.decode.fetch.data, 15)
> -#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0))
> +#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0) \
> + && !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))

We don't use negation "!" in kernel version range tests. We also don't use 
"&&" typically, to make the code easier to maintain. 

Instead, we might want to swap the order in which ifdefs are done, and put 
newer versions at the top, e.g.: 

#if (LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)) \ 
|| LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0)) 
... 
#elif (LINUX_VERSION_CODE >= KERNEL_VERSION(3,1,0)) 
... 
#else 
... 
#endif 

Thanks, 

Mathieu 

> ctf_integer(__u64, rip, vcpu->arch.emulate_ctxt.fetch.start)
> ctf_integer(__u32, csbase, kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS))
> ctf_integer(__u8, len, vcpu->arch.emulate_ctxt._eip
> --
> 2.1.1

> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Mohamad Gebai June 27, 2016, 5:40 p.m. UTC | #2
On 06/27/2016 12:13 PM, Mathieu Desnoyers wrote:
>
> ----- On Jun 27, 2016, at 11:26 AM, mo geb <mohamad.gebai at gmail.com> 
> wrote:
>
> Please use [PATCH lttng-modules] in the subject.
>
Got it.
>
>     Signed-off-by: Mohamad Gebai <mohamad.gebai at gmail.com>
>     ---
>      instrumentation/events/lttng-module/arch/x86/kvm/trace.h | 3 ++-
>      1 file changed, 2 insertions(+), 1 deletion(-)
>
>     diff --git
>     a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>     b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>     index 1282bea..ef3d8aa 100644
>     --- a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>     +++ b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>     @@ -429,7 +429,8 @@ LTTNG_TRACEPOINT_EVENT(kvm_emulate_insn,
>                     ctf_integer(__u8, len,
>     vcpu->arch.emulate_ctxt.decode.eip
>                                     -
>     vcpu->arch.emulate_ctxt.decode.fetch.start)
>                     ctf_array(__u8, insn,
>     vcpu->arch.emulate_ctxt.decode.fetch.data, 15)
>     -#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0))
>     +#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0) \
>     +    && !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))
>
>
> We don't use negation "!" in kernel version range tests. We also don't use
> "&&" typically, to make the code easier to maintain.
>
> Instead, we might want to swap the order in which ifdefs are done, and put
> newer versions at the top, e.g.:
>
> #if (LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)) \
>   || LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))
> ...
> #elif (LINUX_VERSION_CODE >= KERNEL_VERSION(3,1,0))
> ...
> #else
> ...
> #endif
>
> Thanks,
>
> Mathieu
>

Thanks for the comments, I'll make those changes and resubmit. I just 
want to point out that I used a check similar to what's in 
wrapper/trace-clock.h:

#if ((LTTNG_KERNEL_RANGE(3,10,0, 3,10,14) && 
!LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,0, 3,10,14,0,0)) \
     || LTTNG_KERNEL_RANGE(3,11,0, 3,11,3))
#error "Linux kernels 3.10 and 3.11 introduce a deadlock in the 
timekeeping subsystem. Fixed by commit 
7bd36014460f793c19e7d6c94dab67b0afcfcb7f \"timekeeping: Fix HRTICK 
related deadlock from ntp lock changes\" in Linux."
#endif

Mohamad

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.lttng.org/pipermail/lttng-dev/attachments/20160627/d9755b12/attachment-0001.html>
Mathieu Desnoyers June 27, 2016, 5:59 p.m. UTC | #3
---- On Jun 27, 2016, at 1:40 PM, Mohamad <mohamad.gebai at gmail.com> wrote: 

> On 06/27/2016 12:13 PM, Mathieu Desnoyers wrote:

>> ----- On Jun 27, 2016, at 11:26 AM, mo geb <mohamad.gebai at gmail.com> wrote:

>> Please use [PATCH lttng-modules] in the subject.

> Got it.

>>> Signed-off-by: Mohamad Gebai < mohamad.gebai at gmail.com >
>>> ---
>>> instrumentation/events/lttng-module/arch/x86/kvm/trace.h | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)

>>> diff --git a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>>> b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>>> index 1282bea..ef3d8aa 100644
>>> --- a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>>> +++ b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
>>> @@ -429,7 +429,8 @@ LTTNG_TRACEPOINT_EVENT(kvm_emulate_insn,
>>> ctf_integer(__u8, len, vcpu->arch.emulate_ctxt.decode.eip
>>> - vcpu->arch.emulate_ctxt.decode.fetch.start)
>>> ctf_array(__u8, insn, vcpu->arch.emulate_ctxt.decode.fetch.data, 15)
>>> -#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0))
>>> +#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0) \
>>> + && !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))

>> We don't use negation "!" in kernel version range tests. We also don't use
>> "&&" typically, to make the code easier to maintain.

>> Instead, we might want to swap the order in which ifdefs are done, and put
>> newer versions at the top, e.g.:

>> #if (LINUX_VERSION_CODE >= KERNEL_VERSION(3,17,0)) \
>> || LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))
>> ...
>> #elif (LINUX_VERSION_CODE >= KERNEL_VERSION(3,1,0))
>> ...
>> #else
>> ...
>> #endif

>> Thanks,

>> Mathieu

> Thanks for the comments, I'll make those changes and resubmit. I just want to
> point out that I used a check similar to what's in wrapper/trace-clock.h:

> #if ((LTTNG_KERNEL_RANGE(3,10,0, 3,10,14) &&
> !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,0, 3,10,14,0,0)) \
> || LTTNG_KERNEL_RANGE(3,11,0, 3,11,3))
> #error "Linux kernels 3.10 and 3.11 introduce a deadlock in the timekeeping
> subsystem. Fixed by commit 7bd36014460f793c19e7d6c94dab67b0afcfcb7f
> \"timekeeping: Fix HRTICK related deadlock from ntp lock changes\" in Linux."
> #endif
This is OK for a single #if/endif that surrounds an error condition. For the instrumentation, 
we tend to end up having multiple #if #elif #elif #elif #endif that accumulate over time. 
In order to keep this maintainable, I try to avoid having && and ! there. 

Thanks, 

Mathieu 

> Mohamad

> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Michael Jeanson June 27, 2016, 6:01 p.m. UTC | #4
Hi,

This patch does not seem to target master or even 2.8 since the 
LTTNG_RHEL_KERNEL_RANGE macro was modified to take 6 arguments per
RHEL kernel version.

We usually only accept patches targeted at master that are then
backported to supported versions.

Regards,

Michael

----- On Jun 27, 2016, at 11:26 AM, mo geb mohamad.gebai at gmail.com wrote:

> Signed-off-by: Mohamad Gebai < mohamad.gebai at gmail.com >
> ---
> instrumentation/events/lttng-module/arch/x86/kvm/trace.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> index 1282bea..ef3d8aa 100644
> --- a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> +++ b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> @@ -429,7 +429,8 @@ LTTNG_TRACEPOINT_EVENT(kvm_emulate_insn,
> ctf_integer(__u8, len, vcpu->arch.emulate_ctxt.decode.eip
> - vcpu->arch.emulate_ctxt.decode.fetch.start)
> ctf_array(__u8, insn, vcpu->arch.emulate_ctxt.decode.fetch.data, 15)
> -#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0))
> +#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0) \
> + && !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))
> ctf_integer(__u64, rip, vcpu->arch.emulate_ctxt.fetch.start)
> ctf_integer(__u32, csbase, kvm_x86_ops->get_segment_base(vcpu, VCPU_SREG_CS))
> ctf_integer(__u8, len, vcpu->arch.emulate_ctxt._eip
> --
> 2.1.1
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Mohamad Gebai June 28, 2016, 7:28 p.m. UTC | #5
You're right, sorry about that. I was on an experimental branch of ours
(addons) and didn't think that macro had changed. I'll resubmit based on
master.

On Mon, Jun 27, 2016 at 2:01 PM, Michael Jeanson <mjeanson at efficios.com>
wrote:

> Hi,
>
> This patch does not seem to target master or even 2.8 since the
> LTTNG_RHEL_KERNEL_RANGE macro was modified to take 6 arguments per
> RHEL kernel version.
>
> We usually only accept patches targeted at master that are then
> backported to supported versions.
>
> Regards,
>
> Michael
>
> ----- On Jun 27, 2016, at 11:26 AM, mo geb mohamad.gebai at gmail.com wrote:
>
> > Signed-off-by: Mohamad Gebai < mohamad.gebai at gmail.com >
> > ---
> > instrumentation/events/lttng-module/arch/x86/kvm/trace.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> > b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> > index 1282bea..ef3d8aa 100644
> > --- a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> > +++ b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
> > @@ -429,7 +429,8 @@ LTTNG_TRACEPOINT_EVENT(kvm_emulate_insn,
> > ctf_integer(__u8, len, vcpu->arch.emulate_ctxt.decode.eip
> > - vcpu->arch.emulate_ctxt.decode.fetch.start)
> > ctf_array(__u8, insn, vcpu->arch.emulate_ctxt.decode.fetch.data, 15)
> > -#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0))
> > +#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0) \
> > + && !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))
> > ctf_integer(__u64, rip, vcpu->arch.emulate_ctxt.fetch.start)
> > ctf_integer(__u32, csbase, kvm_x86_ops->get_segment_base(vcpu,
> VCPU_SREG_CS))
> > ctf_integer(__u8, len, vcpu->arch.emulate_ctxt._eip
> > --
> > 2.1.1
> >
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev at lists.lttng.org
> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.lttng.org/pipermail/lttng-dev/attachments/20160628/66e4973f/attachment.html>
diff mbox

Patch

diff --git a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
index 1282bea..ef3d8aa 100644
--- a/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
+++ b/instrumentation/events/lttng-module/arch/x86/kvm/trace.h
@@ -429,7 +429,8 @@  LTTNG_TRACEPOINT_EVENT(kvm_emulate_insn,
                ctf_integer(__u8, len, vcpu->arch.emulate_ctxt.decode.eip
                                -
vcpu->arch.emulate_ctxt.decode.fetch.start)
                ctf_array(__u8, insn,
vcpu->arch.emulate_ctxt.decode.fetch.data, 15)
-#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0))
+#elif (LINUX_VERSION_CODE < KERNEL_VERSION(3,17,0) \
+    && !LTTNG_RHEL_KERNEL_RANGE(3,10,0,7,2, 3,11,0,0,0))
                ctf_integer(__u64, rip, vcpu->arch.emulate_ctxt.fetch.start)