Menu
▾
▴
[PATCH v2] Change location to store intermediate JIT dump files
|
From: Daniel H. <dan...@li...> - 2013-01-23 10:45:20
|
On 22.01.2013 20:01, Maynard Johnson wrote:
> On 01/22/2013 02:32 AM, Daniel Hansel wrote:
>> Hi Maynard,
>>
>> I have added the exec_args size increment and created a new patch (using "git format-patch -s HEAD^..") attached to this mail.
>>
>> Sorry for that. :-)
>
> Hi, Daniel,
> The patch works well, but I have a couple of suggestions for changes:
>
> 1) In opcontrol:prep_jitdump: The description above this routine says "create jitdump directory . . . blah". Since the jitdump dir is now created by libopagent, we should remove that part of the description to match your changes to the code.
Hi Maynard,
thanks for that. I forgot to remove that part. Done now. :-)
>
> 2) You bump the OP_MINOR_VERSION of libopagent library in opagent.h, but when the library is built, it's still libopagent.so.1.0.0. I believe that's because the versioning process (as documented in opagent.c, just under the Copyright notice) was not followed. On the other hand, when doing 'java -agentlib:jvmti_oprofile=version', it does print out "1.1" for a version.
>
> But I don't think a minor version bump is necessary anyway. From my readings on the subject of library versioning . . .
> - Major version changes are for incompatible changes.
> - A minor version change is an upward-compatible change which adds some new interfaces, but maintains compatibility for all existing interfaces.
> - A micro release change is a compatible change which does not add any new interfaces -- i.e., implementation detail that does not affect programs that link to the library.
>
> I think your change falls in the realm of a micro release change. Without further investigation right now, I'm not sure how to bump that, but I'm not sure we need to or should do so. What do you think?
As far as I could read the documentation about LD and the version script functionality you are right to bump micro release number only. I do not think to leave the version number as it is (1.0.0) for
such a change.
I have seen that the version numbering of the libopagent file is coded in Makefile.am.
So I have changed the micro version at this point and reverted the minor version change.
Kind regards,
Daniel
>
> -Maynard
>>
>> Kind regards,
>> Daniel
>>
>> On 21.01.2013 23:08, Maynard Johnson wrote:
>>> On 01/18/2013 10:49 AM, Daniel Hansel wrote:
>>>> Change location to store intermediate JIT dump files
>>>>
>>>> Since JIT support was added to Oprofile the intermediate JIT dump files
>>>> holding the sampling data collected for a Java process were stored in a
>>>> hard coded directory /var/lib/oprofile/jitdump that was world-writable.
>>>> The setting of a session specific directory (i.e. "--session-dir=...")
>>>> was not used anyway.
>>>>
>>>> Now during profiling JIT dump files are stored under /tmp/.oprofile/jitdump.
>>>> When opjitconv has finished the conversion of the JIT dump files (the result
>>>> is stored under the default location (e.g. /var/lib/oprofile, ./oprofile_data
>>>> or the specified session directory) the intermediate JIT dump file will be
>>>> deleted.
>>>>
>>>> Signed-off-by: Daniel Hansel <dan...@li...>
>>>
>>> Daniel,
>>> Thanks for the patch. Unfortunately, I can't get it to apply with either 'git apply' or 'patch -p1'. I can't see anything obvious that's causing this. Do you have any ideas? Did you generate the patch with "git format-patch -s HEAD^.." or simply with "diff"?
>>>
>>> AFAICT, the patch looks OK except for the change in daemon/init.c: you need to bump up the size of the exec_args array by one since you're adding the --delete-jitdumps arg.
>>>
>>> -Maynard
>>>
>>>
>>>> ---
>>>> daemon/init.c | 1 +
>>>> libopagent/opagent.c | 34 +++++++++++++++++++++++-----------
>>>> libopagent/opagent.h | 2 +-
>>>> opjitconv/opjitconv.c | 4 ++--
>>>> utils/opcontrol | 7 +------
>>>> 5 files changed, 28 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/daemon/init.c b/daemon/init.c
>>>> index a79e36e..7ee0702 100644
>>>> --- a/daemon/init.c
>>>> +++ b/daemon/init.c
>>>> @@ -174,6 +174,7 @@ static void opd_do_jitdumps(void)
>>>> exec_args[arg_num++] = "opjitconv";
>>>> if (vmisc)
>>>> exec_args[arg_num++] = "-d";
>>>> + exec_args[arg_num++] = "--delete-jitdumps";
>>>> exec_args[arg_num++] = session_dir;
>>>> exec_args[arg_num++] = start_time_str;
>>>> exec_args[arg_num++] = end_time_str;
>>>> diff --git a/libopagent/opagent.c b/libopagent/opagent.c
>>>> index 860413f..599a093 100644
>>>> --- a/libopagent/opagent.c
>>>> +++ b/libopagent/opagent.c
>>>> @@ -116,9 +116,10 @@ static int define_bfd_vars(void)
>>>> * Define the version of the opagent library.
>>>> */
>>>> #define OP_MAJOR_VERSION 1
>>>> -#define OP_MINOR_VERSION 0
>>>> +#define OP_MINOR_VERSION 1
>>>>
>>>> -#define AGENT_DIR OP_SESSION_DIR_DEFAULT "jitdump"
>>>> +#define TMP_OPROFILE_DIR "/tmp/.oprofile"
>>>> +#define JITDUMP_DIR TMP_OPROFILE_DIR "/jitdump"
>>>>
>>>> #define MSG_MAXLEN 20
>>>>
>>>> @@ -135,17 +136,28 @@ op_agent_t op_open_agent(void)
>>>> struct timeval tv;
>>>> FILE * dumpfile = NULL;
>>>>
>>>> - rc = stat(AGENT_DIR, &dirstat);
>>>> - if (rc || !S_ISDIR(dirstat.st_mode)) {
>>>> - if (!rc)
>>>> - errno = ENOTDIR;
>>>> - fprintf(stderr,"libopagent: Jitdump agent directory %s "
>>>> - "missing\n", AGENT_DIR);
>>>> - fprintf(stderr,"libopagent: do opcontrol --setup or "
>>>> - "opcontrol --reset, first\n");
>>>> + if (0 == stat(TMP_OPROFILE_DIR, &dirstat) && !S_ISDIR(dirstat.st_mode)) {
>>>> + fprintf(stderr, "Error: Creation of directory %s failed.\n", TMP_OPROFILE_DIR);
>>>> + errno = EEXIST;
>>>> return NULL;
>>>> + } else {
>>>> + rc = mkdir(TMP_OPROFILE_DIR, S_IRWXU | S_IRWXG | S_IRWXO);
>>>> + if (rc && (errno != EEXIST)) {
>>>> + fprintf(stderr, "Error trying to create %s dir.\n", TMP_OPROFILE_DIR);
>>>> + return NULL;
>>>> + }
>>>> }
>>>> - snprintf(dump_path, PATH_MAX, "%s/%i.dump", AGENT_DIR, getpid());
>>>> + if (0 == stat(JITDUMP_DIR, &dirstat) && !S_ISDIR(dirstat.st_mode)) {
>>>> + fprintf(stderr, "Error: Creation of directory %s failed.\n", JITDUMP_DIR);
>>>> + return NULL;
>>>> + } else {
>>>> + rc = mkdir(JITDUMP_DIR, S_IRWXU | S_IRWXG | S_IRWXO);
>>>> + if (rc && (errno != EEXIST)) {
>>>> + fprintf(stderr, "Error trying to create %s dir.\n", JITDUMP_DIR);
>>>> + return NULL;
>>>> + }
>>>> + }
>>>> + snprintf(dump_path, PATH_MAX, "%s/%i.dump", JITDUMP_DIR, getpid());
>>>> snprintf(err_msg, PATH_MAX + 16, "Error opening %s\n", dump_path);
>>>> // make the dump file only accessible for the user for security reason.
>>>> fd = creat(dump_path, S_IRUSR|S_IWUSR);
>>>> diff --git a/libopagent/opagent.h b/libopagent/opagent.h
>>>> index 920e399..9a4c860 100644
>>>> --- a/libopagent/opagent.h
>>>> +++ b/libopagent/opagent.h
>>>> @@ -45,7 +45,7 @@ typedef void * op_agent_t;
>>>>
>>>> /**
>>>> * This function must be called by agents before any other function.
>>>> - * Creates and opens a JIT dump file in /var/lib/oprofile/jitdump
>>>> + * Creates and opens a JIT dump file in /tmp/.oprofile/jitdump
>>>> * using the naming convention <process_id>.dump.
>>>> *
>>>> * Returns a valid op_agent_t handle or NULL. If NULL is returned, errno
>>>> diff --git a/opjitconv/opjitconv.c b/opjitconv/opjitconv.c
>>>> index 7a9d511..bd3394e 100644
>>>> --- a/opjitconv/opjitconv.c
>>>> +++ b/opjitconv/opjitconv.c
>>>> @@ -540,7 +540,7 @@ static int op_process_jit_dumpfiles(char const * session_dir,
>>>> int rc = OP_JIT_CONV_OK;
>>>> char jitdumpfile[PATH_MAX + 1];
>>>> char oprofile_tmp_template[PATH_MAX + 1];
>>>> - char const * jitdump_dir = "/var/lib/oprofile/jitdump/";
>>>> + char const * jitdump_dir = "/tmp/.oprofile/jitdump/";
>>>>
>>>> LIST_HEAD(jd_fnames);
>>>> char const * anon_dir_filter = "*/{dep}/{anon:anon}/[0-9]*.*";
>>>> @@ -684,7 +684,7 @@ out:
>>>> static void _cleanup_jitdumps(void)
>>>> {
>>>> struct list_head * pos1, *pos2;
>>>> - char const * jitdump_dir = "/var/lib/oprofile/jitdump/";
>>>> + char const * jitdump_dir = "/tmp/.oprofile/jitdump/";
>>>> size_t dir_len = strlen(jitdump_dir);
>>>> char dmpfile_pathname[dir_len + 20];
>>>> char proc_fd_dir[PATH_MAX];
>>>> diff --git a/utils/opcontrol b/utils/opcontrol
>>>> index 6586345..76a7a3b 100644
>>>> --- a/utils/opcontrol
>>>> +++ b/utils/opcontrol
>>>> @@ -1140,12 +1140,7 @@ do_kill_daemon()
>>>> # create jitdump directory and remove any old files from
>>>> # a previous run
>>>> prep_jitdump() {
>>>> - local dumpdir=$SESSION_DIR/jitdump
>>>> - test -d $dumpdir || {
>>>> - mkdir -p $dumpdir;
>>>> - chmod 777 $dumpdir;
>>>> - return;
>>>> - }
>>>> + local dumpdir=/tmp/.oprofile/jitdump
>>>> # VMs may already be running when profiling is started, so
>>>> # remove only dump files that are not in use
>>>> for I in $dumpdir/*; do
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Master HTML5, CSS3, ASP.NET, MVC, AJAX, Knockout.js, Web API and
>>>> much more. Get web development skills now with LearnDevNow -
>>>> 350+ hours of step-by-step video tutorials by Microsoft MVPs and experts.
>>>> SALE $99.99 this month only -- learn more at:
>>>> http://p.sf.net/sfu/learnmore_122812
>>>> _______________________________________________
>>>> oprofile-list mailing list
>>>> opr...@li...
>>>> https://lists.sourceforge.net/lists/listinfo/oprofile-list
>>>>
>>>
>
|