[OS X] getcmdargs() memory fragmentation

[giampaolo]

From [email protected] on May 11, 2009 22:28:25

Here are 7 issues with the psutil/arch/osx/process_info.c file:

line 134 -- val is unused.

line 135 -- val_start is unused.

line 167 -- pid is long while mib is int.

line 171 -- should the constant 22 not be EINVAL?

line 173 -- should -2 not be ARGS_ACCESS_DENIED?

line 242 -- np is never assigned any value.

line 254 -- ERROR_B falls thru to ERROR_C.


For your consideration, attached is a modified version of this file.

Attachment: OSX_process_info.c

Original issue: http://code.google.com/p/psutil/issues/detail?id=50

[giampaolo]

From [email protected] on May 11, 2009 17:43:21

Thanks for the patch. This and the BSD process_info.c files will eventually need to
be properly cleaned up and have unused artifacts from the PSI code removed and the
code restructured. I'll check this in for now as r393 and hopefully I'll have a
chance to go through and really clean things up soon.

Thanks again for your input on this and the other issues you've identified. Code
review is always welcome, especially on the C code.

Status: Fixed
Labels: Milestone-0.1.3

[giampaolo]

From [email protected] on May 11, 2009 22:21:17

Attached is another version for the same psutil/arch/osx/process_info.c file with modifications
in the GetBSDProcessList and getcmdargs functions.

The GetBSDProcessList functions is simpler and uses an increased buffer size to improve the chances for 
success on the second sysctl call.

Function getcmdargs uses the same double sysctl call method as GetBSDProcessList and only get the 
KERN_ARGMAX value as a last resort.  In addition, getcmdargs avoids fragmenting memory for smaller 
argument lists by using the space on the stack.

Attachment: OSX2_process_info.c

[giampaolo]

From [email protected] on May 11, 2009 22:28:56

Reopening.
In the meantime I ask you: every time you apply such modifications do you make sure
that both test_psutil.py and test_memory_leak.py test scripts run successfully?

Status: Started

[giampaolo]

From [email protected] on May 12, 2009 09:16:00

getcmdargs() does not work correctly as defined in OSX2_process_info.c but I've
checked in the GetBSDProcessList() changes for now.

[giampaolo]

From [email protected] on May 12, 2009 09:19:04

Both test cases work the same with the modified and original 0.1.2 code.  There is one failure, the same in both

ERROR: test_types (__main__.TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/test_psutil.py", line 279, in test_types
    self.assert_(isinstance(p.get_cpu_times(), tuple))
  File 
"/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/site-packages/psutil/_psutil.py", line 281, in get_cpu_times
    return _platform_impl.get_cpu_times(self.pid)
  File 
"/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/site-packages/psutil/_psosx.py", line 104, in get_cpu_times
    return _psutil_osx.get_process_cpu_times(pid)
AccessDenied: task_for_pid() failed for pid 348 with error 5

[giampaolo]

From [email protected] on May 12, 2009 09:21:06

The tests mentioned before were run only with Python 2.6.2 on MacOS X 10.4.11 Intel.

[giampaolo]

From [email protected] on May 12, 2009 09:27:11

Test suite should be run as root/sudo otherwise task_for_pid() will fail and you'll
get the above errors. I'm running the same, 10.4.11 with python 2.6 so I'm not sure
why it would have worked for you and not for me but in any case I'm going through now
and cleaning up a little of the getcmdargs() code since I'm in here.

[giampaolo]

From [email protected] on May 12, 2009 09:45:20

With sudo python test ... all tests pass with the modified files.  With Python 2.5.2, the unmodified code also 
passes but the modified version shows two failures "SystemError: getcmdargs(): args parsing".  I'll look into 
those.

[giampaolo]

From [email protected] on May 12, 2009 09:56:12

I've checked in a new process_info.c in r397 with an updated getcmdargs(), so don't
worry about it. I will have to go back in later to add in support for environment
variables anyway. Thanks.

[giampaolo]

From [email protected] on May 12, 2009 10:15:31

This is quite bizarre.  But getcmdarg works fine for all tests if the size of args[2048] is increased to 
args[2049].  Also, if args[1024] is used, the tests fail since argsize is 2048.  That must be a special  value.  
here is what the top of getcmdargs looks like now

    ....
    char     *ap, *cp, *ep, *sp, *procargs, args[2050];  /* avoid 2048 */

    /* Get the size of the process arguments. */
    mib[0] = CTL_KERN;
    mib[1] = KERN_PROCARGS2;
    mib[2] = (int)pid;

    argsize = ~(size_t)0;
    if (sysctl(mib, 3, NULL, &argsize, NULL, 0) == -1) {
        /* Try to get the maximum size. */
        mib[0] = CTL_KERN;
        mib[1] = KERN_ARGMAX;

        size = sizeof(argsize);
        if (sysctl(mib, 2, &argsize, &size, NULL, 0) == -1) {
            PyErr_SetFromErrno(NULL);
            return 1;
        }
    } else {
        argsize += 2;  /* some margin */
    }
    ....

[giampaolo]

From [email protected] on May 12, 2009 10:28:36

OK.  But keep in mind argmax is typically 256+K.  Each getcmdargs call could fragment memory by another 
256+K.   Hopefully, the MacOS X memory manager avoids that.

[giampaolo]

From [email protected] on May 12, 2009 13:12:32

Thanks, I'll convert this issue over to a more specific reminder to take a look at
the getcmdargs() memory profiling more in depth later.

Summary: [OS X] getcmdargs() memory fragmentation
Owner: jloden
Labels: OpSys-OSX

[giampaolo]

From [email protected] on May 12, 2009 14:26:33

One final observation.  It seems that there is often a problem if the size value returned by the last sysctl call is 
the same as the argsize value passed in.   Perhaps, the argsize value is indeed too small to hold the results, 
but sysctl is supposed to return -1 and ser errno to ENOMEM in that case.  It looks like that is not happening.

Therefore, it probably a good idea to slightly increase the size returned by a NULL sysctl call, anywhere.  Here 
is the top of my getcmdargs function:

    ....
    char     *ap, *cp, *ep, *sp, *procargs, args[2048];

    /* Get the size of the process arguments. */
    mib[0] = CTL_KERN;
    mib[1] = KERN_PROCARGS2;
    mib[2] = (int)pid;

    argsize = ~(size_t)0;
    if (sysctl(mib, 3, NULL, &argsize, NULL, 0) == -1) {
        /* Try to get the maximum size. */
        mib[0] = CTL_KERN;
        mib[1] = KERN_ARGMAX;

        size = sizeof(argsize);
        if (sysctl(mib, 2, &argsize, &size, NULL, 0) == -1) {
            PyErr_SetFromErrno(NULL);
            return 1;
        }
    } else {  /* add small margin */
        size = argsize + 2 + (argsize >> 4);
        if (argsize < size) {
            argsize = size;
        }
    }
    ...

That is similar to the GetBSDProcessList above it.

[giampaolo]

From [email protected] on September 17, 2009 11:32:19

Status: PostPoned

[giampaolo]

From g.rodola on March 02, 2013 03:49:10

Updated csets after the SVN -> Mercurial migration: r393 == revision 3afcd3d9c60f r397 == revision 80a3b7473daa

[giampaolo]

From g.rodola on April 30, 2014 05:50:41

I guess this can be closed as outdated.

Status: WontFix