Re: [PATCH v4] Fix clone (CLONE_VM) pid/tid reset (BZ#19957)

[Chris Metcalf <cmetcalf@mellanox.com>]

On 4/26/2016 11:43 AM, Adhemerval Zanella wrote:
> --- a/sysdeps/unix/sysv/linux/tile/clone.S
> +++ b/sysdeps/unix/sysv/linux/tile/clone.S
> @@ -164,43 +164,31 @@ ENTRY (__clone)
>   	cfi_def_cfa_offset (FRAME_SIZE)
>   	cfi_undefined (lr)
>   	/* Check and see if we need to reset the PID, which we do if
> -	   CLONE_THREAD isn't set, i.e. we're not staying in the thread group.
> -	   If CLONE_VM is set, we're doing some kind of thread-like clone,
> -	   so we set the tid/pid to -1 to disable using the cached values
> -	   in getpid().  Otherwise (if CLONE_VM isn't set), it's a
> +	   CLONE_VM isn't set, i.e. we're do not share the same pthread_t
> +	   with parent.   Otherwise (if CLONE_VM isn't set), it's a
>   	   fork-like clone, and we go ahead and write the cached values
>   	   from the true system pid (retrieved via __NR_getpid syscall).  */

I'd change this to:

  	/* Check and see if we need to reset the PID, which we do if
	   CLONE_VM isn't set, i.e. it's a fork-like clone with a new
	   address space.  In that case we update the cached values
  	   from the true system pid (retrieved via __NR_getpid syscall).  */


>   #ifdef __tilegx__
>   	{
>   	 moveli r0, hw1_last(CLONE_VM)
> -	 moveli r1, hw1_last(CLONE_THREAD)
>   	}
>   	{
>   	 shl16insli r0, r0, hw0(CLONE_VM)
> -	 shl16insli r1, r1, hw0(CLONE_THREAD)
>   	}
>   #else
>   	{
>   	 moveli r0, lo16(CLONE_VM)
> -	 moveli r1, lo16(CLONE_THREAD)
>   	}
>   	{
>   	 auli r0, r0, ha16(CLONE_VM)
> -	 auli r1, r1, ha16(CLONE_THREAD)
>   	}
>   #endif
>   	{
>   	 and r0, r30, r0
> -	 and r1, r30, r1
> -	}
> -	BNEZ r1, .Lno_reset_pid   /* CLONE_THREAD is set */
> -	{
> -	 movei r0, -1
> -	 BNEZ r0, .Lgotpid         /* CLONE_VM is set */
>   	}

Here you have left the "{ }" around some single instructions. Stylistically,
we typically don't do that.  Also, we only used the two-instruction format
for loading up r0/r1 because CLONE_THREAD needed it; CLONE_VM doesn't,
so we can collapse all the quoted code above down to just:

	moveli r0, CLONE_VM
	and r0, r30, r0


We could do this in one instruction if we had the bit position (i.e. log-2) of
CLONE_VM, but the obfuscation is not worth it on this not very hot path.

> +	BNEZ r0, .Lno_reset_pid   /* CLONE_VM is set */
>   	moveli TREG_SYSCALL_NR_NAME, __NR_getpid
>   	swint1
> -.Lgotpid:
>   	ADDLI_PTR r2, tp, PID_OFFSET
>   	{
>   	 ST4 r2, r0

With those changes, looks good to me.

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com