[PATCH] malloc: Run fork handler as late as possible [BZ #19431]

[Florian Weimer]

On 04/13/2016 02:55 PM, Torvald Riegel wrote:
> On Tue, 2016-04-12 at 21:15 +0200, Florian Weimer wrote:
>> On 04/12/2016 08:16 PM, Torvald Riegel wrote:
>>> On Thu, 2016-02-18 at 17:15 +0100, Florian Weimer wrote:
>>>> On 02/15/2016 07:10 PM, Torvald Riegel wrote:
>>>>
>>>>>> It explicitly encodes the lock order in the implementations of fork,
>>>>>> and does not rely on the registration order, thus avoiding the deadlock.
>>>>>>
>>>>>> I couldn't test the Hurd bits, but the changes look straightforward enough.
>>>>>
>>>>> Are those changes, and thus the new scheme, documented anywhere?
>>>>
>>>> Fork handlers used by the implementation should be invisible to
>>>> applications.  The old one wasn't, and this was a bug.
>>>
>>> But have you documented this and your understanding of the
>>> synchronization scheme anywhere?  Your explanation in the email with the
>>> patch seems more detailed than the comments in the code.
>>
>> That's because I'm talking mainly about removed code and explaining a
>> bug.  I'm not sure how this information will be useful to future
>> developers once the bug is gone.  We have a regression test, which
>> should avoid reintroducing precisely the same bug.  For similar issues,
>> we need to rely on code review and collective memory.
>
> But you do have a new scheme, right, or are doing things in such a way
> that what was formerly a bug now doesn't matter?  You arrived through
> some information on the conclusion that the new scheme works correctly;
> isn't this worth documenting?  It doesn't seem to be obvious.  For
> example, what about saying that fork handlers should be invisible to the
> application or otherwise you'll get problem X?

I have expanded the comments somewhat.  The key point is to make the 
malloc subsystem available to fork handlers, so it's not just about 
synchronization.

> If you can think about those, why not put a comment into a TODO in the
> code?  Or create a bug.  At least I tend to page out things, so if we
> all do that our collective memory will page out things too :)

I'll send a separate patch.

> I agree that we don't want to document why doing something arbitrary was
> a bad idea.  But if it may be something that might look sensible to do
> at first sight, documenting why that doesn't work helps, I think.

I think the history here is that ptmalloc was out-of-tree initially, so 
they had to use the fork handler mechanism to implement 
malloc-after-fork support.

>> See my reply to Roland.  I was under the impression you objected to the
>> name.
>
> I did object to the name, but simply because so far I had only seen
> cases of -internal.h.  I hadn't looked at math, crypt, and login, which
> use _private.h or -private.h.

I switched to malloc-internal.h because the existing [-_]private.h 
headers seem to be mostly subsystem-internal.

Based on previous feedback, I renamed the test to something more 
descriptive than just a bug number.

Florian

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-malloc-Run-fork-handler-as-late-as-possible-BZ-19431.patch
Type: text/x-patch
Size: 17380 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20160413/51a30e4c/attachment.bin>