svn commit: r215664 - in head/sys: compat/linux kern

[Kostik Belousov]

On Mon, Nov 22, 2010 at 11:13:06AM +0100, Alexander Leidinger wrote:
> Quoting Kostik Belousov <kostikbel at gmail.com> (from Mon, 22 Nov 2010  
> 11:31:34 +0200):
> 
> >On Mon, Nov 22, 2010 at 09:07:00AM +0000, Alexander Leidinger wrote:
> >>Author: netchild
> >>Date: Mon Nov 22 09:06:59 2010
> >>New Revision: 215664
> >>URL: http://svn.freebsd.org/changeset/base/215664
> >>
> >>Log:
> >>  By using the 32-bit Linux version of Sun's Java Development Kit 1.6
> >>  on FreeBSD (amd64), invocations of "javac" (or "java") eventually
> >>  end with the output of "Killed" and exit code 137.
> 
> 
> >>@@ -196,6 +198,12 @@ linux_proc_exit(void *arg __unused, stru
> >> 	} else
> >> 		EMUL_SHARED_WUNLOCK(&emul_shared_lock);
> >>
> >>+	if ((shared_flags & EMUL_SHARED_HASXSTAT) != 0) {
> >>+		PROC_LOCK(p);
> >>+		p->p_xstat = shared_xstat;
> >>+		PROC_UNLOCK(p);
> >>+	}
> >Why is process lock taken there ? The assignment to u_short inside the
> >properly aligned structure is atomic on all supported architectures, and
> >the thread that should see side-effect of assignment is the same thread
> >that does assignment.
> 
> Change below.
> 
> >>+
> >> 	if (child_clear_tid != NULL) {
> >> 		struct linux_sys_futex_args cup;
> >> 		int null = 0;
> >>@@ -257,6 +265,9 @@ linux_proc_exec(void *arg __unused, stru
> >> 	if (__predict_false(imgp->sysent == &elf_linux_sysvec
> >> 	    && p->p_sysent != &elf_linux_sysvec))
> >> 		linux_proc_init(FIRST_THREAD_IN_PROC(p), p->p_pid, 0);
> >>+	if (__predict_false(p->p_sysent == &elf_linux_sysvec))
> >>+		/* Kill threads regardless of imgp->sysent value */
> >>+		linux_kill_threads(FIRST_THREAD_IN_PROC(p), SIGKILL);
> >This is better expressed by
> >	if ((p->p_sysent->sv_flags & SV_ABI_MASK) == SV_ABI_LINUX)
> 
> Is this OK for you?
> ---snip---
> Index: compat/linux/linux_emul.c
> ===================================================================
> --- compat/linux/linux_emul.c   (Revision 215664)
> +++ compat/linux/linux_emul.c   (Arbeitskopie)
> @@ -198,11 +198,8 @@
>         } else
>                 EMUL_SHARED_WUNLOCK(&emul_shared_lock);
> 
> -       if ((shared_flags & EMUL_SHARED_HASXSTAT) != 0) {
> -               PROC_LOCK(p);
> +       if ((shared_flags & EMUL_SHARED_HASXSTAT) != 0)
>                 p->p_xstat = shared_xstat;
> -               PROC_UNLOCK(p);
> -       }
> 
>         if (child_clear_tid != NULL) {
>                 struct linux_sys_futex_args cup;
> @@ -265,7 +262,8 @@
>         if (__predict_false(imgp->sysent == &elf_linux_sysvec
>             && p->p_sysent != &elf_linux_sysvec))
>                 linux_proc_init(FIRST_THREAD_IN_PROC(p), p->p_pid, 0);
> -       if (__predict_false(p->p_sysent == &elf_linux_sysvec))
> +       if (__predict_false((p->p_sysent->sv_flags & SV_ABI_MASK) ==
> +           SV_ABI_LINUX))
>                 /* Kill threads regardless of imgp->sysent value */
>                 linux_kill_threads(FIRST_THREAD_IN_PROC(p), SIGKILL);
>         if (__predict_false(imgp->sysent != &elf_linux_sysvec
> ---snip---
Yes.

> 
> >Regardless of this mostly cosmetic issue, this is racy. Other
> >linux thread in the same process might do an execve(3).
> >More, if execve(3) call fails, then you return into the process
> >that lacks all threads except the one that called execve(3).
> 
> How critical is this in your opinion (relative to the issue this patch  
> is fixing)? Do you prefer a backout or do you think the probability  
> that the someone wins the race is low enough?
> 
> Do you see a solution for the race?
I did not asked for backout, nor I am asking now.

Most likely, the semantic of linux thread groups cannot be implemented
by only using event handlers that linux.ko hooks now. How linux handles
single-threading when doing execve(2) from multithreaded process ?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20101122/414e8b7a/attachment.pgp