On 5/1/07, Neal Norwitz <nnorwitz@gmail.com> wrote:

In rev 54982 (the first time this crash was seen), I see something which might create a problem. In python/trunk/Modules/posixmodule.c (near line 6300):

+ PyMem_FREE(mode); Py_END_ALLOW_THREADS

The PyMem_MALLOC call that creates 'mode' is also called without explicitly holding the GIL. Can you call PyMem_FREE() without the GIL held? I couldn't find it

documented either way.

I believe the GIL does not need to be held, but obviously Tim or someone with more memory experience should step in to say definitively. If you look at Include/pymem.h, PyMem_FREE gets defined as PyObject_FREE in a debug build. PyObject_Free is defined at _PyObject_DebugFree. That function checks that the memory has not been written with the debug bit pattern and then calls PyObject_Free. That call just sticks the memory back into pymalloc's memory pool which is implemented without using any Python objects. In other words no Python objects are used in pymalloc (to my knowledge) and thus is safe to use without the GIL. -Brett

but I got to a different conclusion. If it really goes through the pymalloc pool (obmalloc), then it must hold the GIL while doing so. obmalloc itself is not thread-safe, and relies on the GIL for thread-safety.

In release mode, PyMEM_FREE goes directly to free, which is thread- safe.

Yes. It is quite unfortunate how PyMem_* gets redirected to the PyObject_* functions in debug builds. Even worse is how PyObject_Malloc gets #defined to PyObject_DebugMalloc for debug builds, changing linkage of modules. But that is a different matter. One thing I'd like to point out however, is that it is quite unnecessary for the PyObject_DebugMalloc() functions to lie on top of PyObject_Malloc() They can just call malloc() etc. directly, since in debug builds the performance benefit of the block allocator is moot. I'd suggest to keep the debug functions as a thin layer on top of malloc to do basic testing. I'd even suggest that we reverse things, and move the debug library to pymem.c. This would keep the debug functionalty threadsafe on top of regular malloc, rather than wrapping it in there with the non-threadsafe object allocator. We would then have void *PyMem_DebugMalloc() /* layers malloc /* void *PyMem_Malloc() /* calls PyMem_MALLOC */ #ifndef _DEBUG #define PyMem_MALLOC malloc #else #define PyMem_MALLOC PyMem_DebugMalloc #endif PyObject_Malloc() would then just call PyMem_DebugMalloc in DEBUG builds. The reason I have opinions on this is that at CCP we have spent considerable effort on squeezing our own veneer functions into the memory allocators, both for the PyMem ones and PyObject. And the structure of the macros and their interconnectivity really doesn't make it easy. We ended up creating a set of macros like PyMem_MALLOC_INNER() and ease our functions between the MALLOC and the INNER. I'll try to show you the patch one day which is a reasonable attempt at a slight reform in the structure of these memory APIs. Perhaps something for Py3K. Kristjan

-----Original Message----- From: skip@pobox.com [mailto:skip@pobox.com] Sent: Tuesday, May 01, 2007 20:46 But then you would substantially change the memory access behavior of the program in a debug build, that is, more than it is already changed by the fact that you have changed the memory layout of Python objects.

Well, as we say in Iceland, that is a piece difference, not the whole sheep. In fact, most of the memory is already managed by the Object allocator, so there is only slight additional change. Further, at least one platform (windows) already employs a different memory allocator implementation for malloc in debug builds, namely a debug allocator. In addition, many python structures grow extra members in debug builds, most notably the PyObject _head. So you probably never have the exactly same blocksize pattern anyway. At any rate, the debug memory only changes memory access patterns by growing every block by a fixed amount. And why do we want to keep the same memory pattern? Isn't the memory allocator supposed to be a black box? The only reason I can see for maintaining the exact same pattern in a debug build is to reproduce some sort of memory access error, but that is precisely what the debug routines are for. Admittedly, I have never used the debug routines much. generally disable the object allocator for debug builds, and rely on the windows debug malloc implementation to spot errors for me, or failing that, I use Rational Purify, which costs money.

On 5/1/07, Tim Peters <tim.peters@gmail.com> wrote:

[Neal Norwitz]

...

...

In rev 54982 (the first time this crash was seen), I see something which might create a problem. In python/trunk/Modules/posixmodule.c (near line 6300):

+ PyMem_FREE(mode); Py_END_ALLOW_THREADS

Shouldn't do that.

[Brett Cannon]

...

The PyMem_MALLOC call that creates 'mode' is also called without explicitly holding the GIL.

Or that ;-)

Luckily I misread the code so it doesn't do that boo-boo.

...

Can you call PyMem_FREE() without the GIL held? I couldn't find it

...

documented either way.

...

I believe the GIL does not need to be held, but obviously Tim or someone with more memory experience should step in to say definitively.

The GIL should be held. The relevant docs are in the Python/C API manual, section "8.1 Thread State and the Global Interpreter Lock":

Therefore, the rule exists that only the thread that has acquired the global interpreter lock may operate on Python objects or call Python/C API functions.

PyMem_XYZ is certainly a "Python/C API function". There are functions you can call without holding the GIL, and section 8.1 intends to give an exhaustive list of those. These are functions that can't rely on the GIL, like PyEval_InitThreads() (which /creates/ the GIL), and various functions that create and destroy thread and interpreter state.

...

If you look at Include/pymem.h, PyMem_FREE gets defined as PyObject_FREE in a debug build. PyObject_Free is defined at _PyObject_DebugFree. That function checks that the memory has not been written with the debug bit pattern and then calls PyObject_Free. That call just sticks the memory back into pymalloc's memory pool which is implemented without using any Python objects.

But pymalloc's pools have a complex internal structure of their own, and cannot be mucked with safely by multiple threads simultaneously.

Ah, OK. That makes sense. Glad I pointed out my ignorance then. =) -Brett