Re: [PATCH 3/3] Refactor atfork handlers

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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.