[PATCH 0 of 3] Add support for dtrace compatible sdt probes on GNU/Linux
Keith McGuigan
keith.mcguigan at oracle.com
Wed May 23 06:14:24 PDT 2012
Just a couple of quick comments/questions on the code:
Why did we get rid of a couple of the declarations in jni.cpp? Why
aren't they needed?
dtrace.hpp should be refactored (if possible) into the os-specific
subdirectories, instead of using #ifdef <OS> macros in the header.
I.e., you might have to make a src/solaris/vm/dtrace_solaris.hpp and
src/linux/vm/dtrace_linux.hpp file and include it from here.
Has this been built on solaris, and is the dtrace functionality unchanged?
--
- Keith
On 5/23/2012 6:19 AM, serguei.spitsyn at oracle.com wrote:
>
> I've generated the webrev (sorry it is not on the public domain yet):
> http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/DTrace-Probes-Linux-HS.0/
>
> Below is the patch related to the webrev above.
>
> Thanks,
> Serguei
>
>
> diff -r 4b37c0dafe3a make/linux/makefiles/dtrace.make
> --- a/make/linux/makefiles/dtrace.make Fri May 18 09:15:15 2012 -0700
> +++ b/make/linux/makefiles/dtrace.make Wed May 23 03:03:58 2012 -0700
> @@ -1,5 +1,6 @@
> #
> # Copyright (c) 2005, 2008, Oracle and/or its affiliates. All rights
> reserved.
> +# Copyright (c) 2012 Red Hat, Inc.
> # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> #
> # This code is free software; you can redistribute it and/or modify it
> @@ -25,3 +26,26 @@
> # Linux does not build jvm_db
> LIBJVM_DB =
>
> +# But it does have a SystemTap dtrace compatible sys/sdt.h
> +ifneq ($(ALT_SDT_H),)
> + SDT_H_FILE = $(ALT_SDT_H)
> +else
> + SDT_H_FILE = /usr/include/sys/sdt.h
> +endif
> +DTRACE_ENABLED = $(shell test -f $(SDT_H_FILE)&& echo $(SDT_H_FILE))
> +
> +ifneq ($(DTRACE_ENABLED),)
> + CFLAGS += -DDTRACE_ENABLED
> +endif
> +
> +# Phone target used in vm.make build target to check whether enabled.
> +.PHONY: dtraceCheck
> +ifeq ($(DTRACE_ENABLED),)
> +dtraceCheck:
> + $(QUIETLY) echo "**NOTICE** Dtrace support disabled $(SDT_H_FILE)
> not found"
> +else
> +dtraceCheck:
> +endif
> +
> +# It doesn't support HAVE_DTRACE_H though.
> +
> diff -r 4b37c0dafe3a make/linux/makefiles/vm.make
> --- a/make/linux/makefiles/vm.make Fri May 18 09:15:15 2012 -0700
> +++ b/make/linux/makefiles/vm.make Wed May 23 03:03:58 2012 -0700
> @@ -383,7 +383,7 @@
>
> #----------------------------------------------------------------------
>
> -build: $(LIBJVM) $(LAUNCHER) $(LIBJSIG) $(LIBJVM_DB) $(BUILDLIBSAPROC)
> $(WB_JAR)
> +build: $(LIBJVM) $(LAUNCHER) $(LIBJSIG) $(LIBJVM_DB) $(BUILDLIBSAPROC)
> dtraceCheck $(WB_JAR)
>
> install: install_jvm install_jsig install_saproc
>
> diff -r 4b37c0dafe3a src/share/vm/prims/jni.cpp
> --- a/src/share/vm/prims/jni.cpp Fri May 18 09:15:15 2012 -0700
> +++ b/src/share/vm/prims/jni.cpp Wed May 23 03:03:58 2012 -0700
> @@ -1,5 +1,6 @@
> /*
> * Copyright (c) 1997, 2011, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2012 Red Hat, Inc.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -2818,10 +2819,8 @@
> JNI_QUICK_ENTRY(void, jni_Set##Result##Field(JNIEnv *env, jobject obj,
> jfieldID fieldID, Argument value)) \
> JNIWrapper("Set" XSTR(Result) "Field"); \
> \
> - HS_DTRACE_PROBE_CDECL_N(hotspot_jni, Set##Result##Field__entry, \
> - ( JNIEnv*, jobject, jfieldID FP_SELECT_##Result(COMMA
> Argument,/*empty*/) ) ); \
> - HS_DTRACE_PROBE_N(hotspot_jni, Set##Result##Field__entry, \
> - ( env, obj, fieldID FP_SELECT_##Result(COMMA value,/*empty*/) ) ); \
> + FP_SELECT_##Result(DTRACE_PROBE4(hotspot_jni,
> Set##Result##Field__entry, env, obj, fieldID, value), \
> + DTRACE_PROBE3(hotspot_jni,
> Set##Result##Field__entry, env, obj, fieldID)); \
> \
> oop o = JNIHandles::resolve_non_null(obj); \
> klassOop k = o->klass(); \
> @@ -3128,10 +3127,8 @@
> \
> JNI_ENTRY(void, jni_SetStatic##Result##Field(JNIEnv *env, jclass
> clazz, jfieldID fieldID, Argument value)) \
> JNIWrapper("SetStatic" XSTR(Result) "Field"); \
> - HS_DTRACE_PROBE_CDECL_N(hotspot_jni, SetStatic##Result##Field__entry,\
> - ( JNIEnv*, jclass, jfieldID FP_SELECT_##Result(COMMA
> Argument,/*empty*/) ) ); \
> - HS_DTRACE_PROBE_N(hotspot_jni, SetStatic##Result##Field__entry, \
> - ( env, clazz, fieldID FP_SELECT_##Result(COMMA value,/*empty*/) ) ); \
> + FP_SELECT_##Result(DTRACE_PROBE4(hotspot_jni,
> SetStatic##Result##Field__entry, env, clazz, fieldID, value), \
> + DTRACE_PROBE3(hotspot_jni,
> SetStatic##Result##Field__entry, env, clazz, fieldID)); \
> \
> JNIid* id = jfieldIDWorkaround::from_static_jfieldID(fieldID); \
> assert(id->is_static_field_id(), "invalid static field id"); \
> diff -r 4b37c0dafe3a src/share/vm/utilities/dtrace.hpp
> --- a/src/share/vm/utilities/dtrace.hpp Fri May 18 09:15:15 2012 -0700
> +++ b/src/share/vm/utilities/dtrace.hpp Wed May 23 03:03:58 2012 -0700
> @@ -1,5 +1,6 @@
> /*
> * Copyright (c) 2005, 2010, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2009, 2012 Red Hat, Inc.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -32,12 +33,15 @@
> #define DTRACE_ONLY(x) x
> #define NOT_DTRACE(x)
>
> +#if defined(SOLARIS)
> // Work around dtrace tail call bug 6672627 until it is fixed in
> solaris 10.
> #define HS_DTRACE_WORKAROUND_TAIL_CALL_BUG() \
> do { volatile size_t dtrace_workaround_tail_call_bug = 1; } while (0)
>
> -#if defined(SOLARIS)
> #define USDT1 1
> +#elif defined(LINUX)
> +#define USDT1 1
> +#define HS_DTRACE_WORKAROUND_TAIL_CALL_BUG()
> #elif defined(__APPLE__)
> #define USDT2 1
> #include <sys/types.h>
> @@ -63,6 +67,11 @@
> #define DTRACE_PROBE3(a,b,c,d,e) {;}
> #define DTRACE_PROBE4(a,b,c,d,e,f) {;}
> #define DTRACE_PROBE5(a,b,c,d,e,f,g) {;}
> +#define DTRACE_PROBE6(a,b,c,d,e,f,g,h) {;}
> +#define DTRACE_PROBE7(a,b,c,d,e,f,g,h,i) {;}
> +#define DTRACE_PROBE8(a,b,c,d,e,f,g,h,i,j) {;}
> +#define DTRACE_PROBE9(a,b,c,d,e,f,g,h,i,j,k) {;}
> +#define DTRACE_PROBE10(a,b,c,d,e,f,g,h,i,j,k,l) {;}
>
> #else /* USDT2 */
>
> @@ -76,10 +85,19 @@
> #define HS_DTRACE_PROBE_FN(provider,name)\
> __dtrace_##provider##___##name
>
> +#ifdef SOLARIS
> +// Solaris dtrace needs actual extern function decls.
> #define HS_DTRACE_PROBE_DECL_N(provider,name,args) \
> DTRACE_ONLY(extern "C" void HS_DTRACE_PROBE_FN(provider,name) args)
> #define HS_DTRACE_PROBE_CDECL_N(provider,name,args) \
> DTRACE_ONLY(extern void HS_DTRACE_PROBE_FN(provider,name) args)
> +#elif defined(LINUX)
> +// Systemtap dtrace compatible probes on GNU/Linux don't.
> +#define HS_DTRACE_PROBE_DECL_N(provider,name,args)
> +#define HS_DTRACE_PROBE_CDECL_N(provider,name,args)
> +#else
> +#error "USDT1 enabled for unknown os"
> +#endif
>
> /* Dtrace probe declarations */
> #define HS_DTRACE_PROBE_DECL(provider,name) \
> @@ -118,6 +136,8 @@
> uintptr_t,uintptr_t,uintptr_t))
>
> /* Dtrace probe definitions */
> +#if defined(SOLARIS)
> +// Solaris dtrace uses actual function calls.
> #define HS_DTRACE_PROBE_N(provider,name, args) \
> DTRACE_ONLY(HS_DTRACE_PROBE_FN(provider,name) args)
>
> @@ -153,6 +173,34 @@
>
> HS_DTRACE_PROBE_N(provider,name,((uintptr_t)a0,(uintptr_t)a1,(uintptr_t)a2,\
>
> (uintptr_t)a3,(uintptr_t)a4,(uintptr_t)a5,(uintptr_t)a6,(uintptr_t)a7,\
> (uintptr_t)a8,(uintptr_t)a9))
> +#elif defined(LINUX)
> +// Systemtap dtrace compatible probes on GNU/Linux use direct macros.
> +#define HS_DTRACE_PROBE(provider,name) HS_DTRACE_PROBE0(provider,name)
> +#define HS_DTRACE_PROBE0(provider,name)\
> + DTRACE_PROBE(provider,name)
> +#define HS_DTRACE_PROBE1(provider,name,a0)\
> + DTRACE_PROBE1(provider,name,a0)
> +#define HS_DTRACE_PROBE2(provider,name,a0,a1)\
> + DTRACE_PROBE2(provider,name,a0,a1)
> +#define HS_DTRACE_PROBE3(provider,name,a0,a1,a2)\
> + DTRACE_PROBE3(provider,name,a0,a1,a2)
> +#define HS_DTRACE_PROBE4(provider,name,a0,a1,a2,a3)\
> + DTRACE_PROBE4(provider,name,a0,a1,a2,a3)
> +#define HS_DTRACE_PROBE5(provider,name,a0,a1,a2,a3,a4)\
> + DTRACE_PROBE5(provider,name,a0,a1,a2,a3,a4)
> +#define HS_DTRACE_PROBE6(provider,name,a0,a1,a2,a3,a4,a5)\
> + DTRACE_PROBE6(provider,name,a0,a1,a2,a3,a4,a5)
> +#define HS_DTRACE_PROBE7(provider,name,a0,a1,a2,a3,a4,a5,a6)\
> + DTRACE_PROBE7(provider,name,a0,a1,a2,a3,a4,a5,a6)
> +#define HS_DTRACE_PROBE8(provider,name,a0,a1,a2,a3,a4,a5,a6,a7)\
> + DTRACE_PROBE8(provider,name,a0,a1,a2,a3,a4,a5,a6,a7)
> +#define HS_DTRACE_PROBE9(provider,name,a0,a1,a2,a3,a4,a5,a6,a7,a8)\
> + DTRACE_PROBE9(provider,name,a0,a1,a2,a3,a4,a5,a6,a7,a8)
> +#define HS_DTRACE_PROBE10(provider,name,a0,a1,a2,a3,a4,a5,a6,a7,a8)\
> + DTRACE_PROBE10(provider,name,a0,a1,a2,a3,a4,a5,a6,a7,a8)
> +#else
> +#error "USDT1 enabled for unknown os"
> +#endif
>
> #endif /* !USDT2 */
>
>
>
>
>
> On 5/21/12 11:42 PM, serguei.spitsyn at oracle.com wrote:
>>
>>
>>
>> On 5/21/12 9:27 PM, David Holmes wrote:
>>> Is systemTap part of regular distros these days - in particular in
>>> our build environment?
>>>
>> I don't know yet, we have to figure it out.
>> It'd be nice to have a Keith's opinion first.
>>
>>> I'm just wondering who is going to ensure that these probe entries
>>> compile and run correctly?
>>
>> It depends on the availability or possible setup of linux machines
>> with the systemTap.
>> Some test is needed as well.
>>
>>>
>>> Can we see a complete webrev of the proposed changes?
>>
>> I'll prepare a complete webrev.
>>
>> Thanks,
>> Serguei
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 22/05/2012 1:28 PM, serguei.spitsyn at oracle.com wrote:
>>>> Mark,
>>>>
>>>> I've created new RFE:
>>>> 7170638: enable support for dtrace compatible sdt probes on GNU/Linux
>>>>
>>>> The patches look good.
>>>> But I think, it is better to be reviewed by Keith McGuigan, who
>>>> originally added DTrace probes to HotSpot.
>>>> Also, let's check if we have to submit a CCC request for this.
>>>>
>>>> Keith, do you have cycles to review the patches from Mark?
>>>> I've included them into the bug report.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 5/21/12 2:28 AM, Mark Wielaard wrote:
>>>>> On Thu, 2012-05-17 at 23:30 -0700,serguei.spitsyn at oracle.com wrote:
>>>>>> On 18/05/2012 9:47 AM,serguei.spitsyn at oracle.com wrote:
>>>>>> On 5/17/12 9:48 PM, David Holmes wrote:
>>>>>>>> Where are the patches?
>>>>>>>> I do not see them in your emails.
>>>>>>>>
>>>>>>> The patches themselves are the emails with the subjects:
>>>>>>>
>>>>>>> [PATCH 1 of 3] ...
>>>>>>> [PATCH 2 of 3] ...
>>>>>>> [PATCH 3 of 3] ...
>>>>>>>
>>>>>> I've found these emails, they somehow got to the trash folder - very
>>>>>> strange.
>>>>> Apologies. That is how hg email sends them. I have a local mercurial
>>>>> repository that contain the patches. Maybe your spam filter caught it
>>>>> because all subjects start with XXXXXX as placeholder because I don't
>>>>> have official bug numbers for them. If you could create the bugs
>>>>> and/or
>>>>> let me know if you rather see these patches split up differently
>>>>> (or not
>>>>> split up at all), please let me know and I rebase my patches and
>>>>> resend
>>>>> them.
>>>>>
>>>>>>>> On 5/17/12 12:25 PM, Mark Wielaard wrote:
>>>>>>>>> On Mon, May 14, 2012 at 07:41:50AM -0600, Daniel D. Daugherty
>>>>>>>>> wrote:
>>>>>>>>>> On 5/14/12 2:06 AM, Mark Wielaard wrote:
>>>>>>>>>>> On Thu, 2012-05-10 at 09:52 -0600, Daniel D. Daugherty
>>>>>>>>>>> wrote:
>>>>>>>>>>>> Addingserviceability-dev at openjdk.java.net to this e-mail
>>>>>>>>>>>> thread.
>>>>>>>>>>> Thanks. Please let me know if there is anything I can do to
>>>>>>>>>>> help
>>>>>>>>>>> integration of these patches, or if there are other
>>>>>>>>>>> places/people I need
>>>>>>>>>>> to add to the CC for review.
>>>>>>>>>> No problem. I was just making sure your e-mails got to the
>>>>>>>>>> right aliases.
>>>>>>>>>> As I no longer work on the Serviceability team, I'll leave it
>>>>>>>>>> to them to
>>>>>>>>>> address your e-mails.
>>>>>>>>> Feedback from the Serviceability team is highly appreciated.
>>>>>>>>> Please let
>>>>>>>>> me know how I can help getting these patches in as soon as
>>>>>>>>> possible.
>>>>>>>>>>>> On 5/10/12 7:49 AM, Mark Wielaard wrote:
>>>>>>>>>>>>> The following three patches enable support for dtrace
>>>>>>>>>>>>> compatible sdt probes
>>>>>>>>>>>>> on GNU/Linux. Most of the support was already submitted
>>>>>>>>>>>>> earlier. These are
>>>>>>>>>>>>> just the last three things left in IcedTea that actually
>>>>>>>>>>>>> enable it in
>>>>>>>>>>>>> hotspot. It has been in production use for some time
>>>>>>>>>>>>> already.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The patches could be applied all as one, or individually
>>>>>>>>>>>>> if you want to be
>>>>>>>>>>>>> able to test them independently.
>>>>>>>>>>> [... explanation of separate patches removed ...]
>>>>>>>>>>>>> Patches in separate emails.
>>>>
>>
>
More information about the serviceability-dev
mailing list