This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 3/3] Refactor atfork handlers
On 20/02/2018 08:29, Florian Weimer wrote:
> On 02/08/2018 01:50 PM, Adhemerval Zanella wrote:
>> +static struct fork_handler *
>> +fork_handler_list_find_if (struct fork_handler_list *fork_handlers,
>> + void *dso_handle)
>
> Should be called _find, not find_if (no callback is involved).
Fixed.
>
>> + struct fork_handler *first = fork_handler_list_find_if (&fork_handlers,
>> + dso_handle);
>> + /* Removing is done by shifting the elements in the way the elements
>> + that are not to be removed appear in the beginning in dynarray.
>> + This avoid the quadradic run-time if a naive strategy to remove and
>> + shift one element at time. */
>> + if (first != NULL)
>> + {
>> + struct fork_handler *result = first;
>
> result should probably be called new_end or something like that.
I changed to new_end.
>
>> + first++;
>> + for (; first != fork_handler_list_end (&fork_handlers); ++first)
>> + {
>> + if (first->dso_handle != dso_handle)
>> + {
>> + memcpy (result, first, sizeof (struct fork_handler));
>
> Wouldn't a simple struct assignment work here?
I think so, I changed it to struct assignment.
>
> I think this patch is a step in the right direction, so it should go in with these changes.
Thanks for the review.
>
> However, I think we should make a few improvements in follow-up fixes:
>
> Reduce RSS usage for the common case that no atfork handlers are ever registered. This will be the case once we remove the bogus __reclaim_stacks function.
>
> Make a temporary copy of the handler array during fork. This has two benefits: We can run the handlers without acquiring the handler lock (to avoid application deadlocks). We also make sure that a handler does not run in a child process which did not run in the parent process. I think the old implementation had both properties.
The temporary copy is problematic because we either need to allocate on the stack using
vla/alloca (current practice and prone of stack overflow) or by malloc (which requires
locking anyway). Also, to temporary copy we will need pretty much the same lock-free
algorithm which adds code complexity.
My understanding is current algorithm tries hard to remove any locking on fork generation
mainly because back then posix_spawn was no specified and suboptimal. Now that we have
a faster way to spawn process in multithread environment I think there is no much gain
in trying to optimizing locking in atfork handlers.
Regarding the handler running in child process the proposed implementation does implement
it.