Re: [PATCH 3/8] posix: Consolidate fork implementation

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

On 09/03/2021 07:58, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/sysdeps/nptl/_Fork.c b/sysdeps/nptl/_Fork.c
>> new file mode 100644
>> index 0000000000..9c701bc3d7
>> --- /dev/null
>> +++ b/sysdeps/nptl/_Fork.c
> 
>> +pid_t
>> +_Fork (void)
>> +{
>> +  pid_t pid = arch_fork (&THREAD_SELF->tid);
>> +  if (pid == 0)
>> +    {
>> +      struct pthread *self = THREAD_SELF;
>> +
>> +      /* See __pthread_once.  */
>> +      if (__fork_generation_pointer != NULL)
>> +	*__fork_generation_pointer += __PTHREAD_ONCE_FORK_GEN_INCR;
> 
> This should not be part of _Fork, I think.  There is no need to do that
> for a new interface.

I think you are right, this is used solely for pthread_once and it is not
marked as async-signal-safe (so there is no need to keep it on Linux
_Fork implementation).

> 
> I think this means that there has to be _Fork, the generic fork
> implementation, and something between _Fork and fork that performs the
> architecture-specific bits.

It should be simpler to add a inline hook on fork instead, so the 
sequence would be:

posix/fork.c
   [...]
   pid_t pid = _Fork ();
   if (pid == 0)
     {
       __fork_system_setup ();
       [...]
     }

And __fork_system_setup () is implemented on Linux to setup the required
fork generation for pthread_once (with an empty generic one).

> 
> Hurd may also need this for its internal fork handlers if they are
> supposed to run for fork, but not for _Fork.

I did not dig into Hurd implementation, but I tried to make its _Fork
to no run pthread_atfork as well.  It does use the old callback 
mechanism:

sysdeps/mach/hurd/_Fork.c

 41 /* Things that want to be called before we fork, to prepare the parent for
 42    task_create, when the new child task will inherit our address space.  */
 43 DEFINE_HOOK (_hurd_fork_prepare_hook, (void));
 44 
 45 /* Things that want to be called when we are forking, with the above all
 46    locked.  They are passed the task port of the child.  The child process
 47    is all set up except for doing proc_child, and has no threads yet.  */
 48 DEFINE_HOOK (_hurd_fork_setup_hook, (void));
 49 
 50 /* Things to be run in the child fork.  */
 51 DEFINE_HOOK (_hurd_fork_child_hook, (void));
 52 
 53 /* Things to be run in the parent fork.  */
 54 DEFINE_HOOK (_hurd_fork_parent_hook, (void));

sysdeps/mach/hurd/profil.c:213:text_set_element (_hurd_fork_prepare_hook, fork_profil_prepare);

sysdeps/mach/hurd/profil.c:255:text_set_element (_hurd_fork_child_hook, fork_profil_child);
sysdeps/mach/hurd/setitimer.c:377:text_set_element (_hurd_fork_child_hook, fork_itimer);

sysdeps/mach/hurd/profil.c:221:text_set_element (_hurd_fork_parent_hook, fork_profil_parent);

The hooks seems to be self-contained and there are only a handful of them,
so maybe Hurd developers could cleanup this up and run in directly
instead.

The __run_fork_handlers is only called from fork.c now.

> 
> Given that this affects the way functionality is split in the
> consolidation, I have not reviewed this version of the patch further.

Alright, I will update the patch.