[Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

vfork() is a kind of fork, not thread despite CLONE_VM

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 linux-user/syscall.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index fd4890e..2abdc83 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2787,7 +2787,7 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
     sigset_t sigmask;
 #endif
 
-    if (flags & CLONE_VM) {
+    if (!(flags & CLONE_VFORK) && flags & CLONE_VM) {
 #if defined(USE_NPTL)
         new_thread_info info;
         pthread_attr_t attr;
@@ -2856,8 +2856,8 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
 #endif
 #endif
     } else {
-        /* if no CLONE_VM, we consider it is a fork */
-        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2)) != 0)
+        /* we consider it is a fork or vfork */
+        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2 | CLONE_VFORK | CLONE_VM)) != 0)
             return -EINVAL;
         fork_start();
         ret = fork();
-- 
1.5.6.5.GIT

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Jamie Lokier]

Kirill A. Shutemov wrote:
> -    if (flags & CLONE_VM) {
> +    if (!(flags & CLONE_VFORK) && flags & CLONE_VM) {

Parentheses around (flags & CLONE_VM) would be good here.

-- Jamie

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

On Sun, Aug 24, 2008 at 03:47:33PM +0100, Jamie Lokier wrote:
> Kirill A. Shutemov wrote:
> > -    if (flags & CLONE_VM) {
> > +    if (!(flags & CLONE_VFORK) && flags & CLONE_VM) {
> 
> Parentheses around (flags & CLONE_VM) would be good here.

Agreed. I'll resend patch soon.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

vfork() is a kind of fork, not thread despite CLONE_VM

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 linux-user/syscall.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index fd4890e..8a00734 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2787,7 +2787,7 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
     sigset_t sigmask;
 #endif
 
-    if (flags & CLONE_VM) {
+    if (!(flags & CLONE_VFORK) && (flags & CLONE_VM)) {
 #if defined(USE_NPTL)
         new_thread_info info;
         pthread_attr_t attr;
@@ -2856,8 +2856,8 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
 #endif
 #endif
     } else {
-        /* if no CLONE_VM, we consider it is a fork */
-        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2)) != 0)
+        /* we consider it is a fork or vfork */
+        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2 | CLONE_VFORK | CLONE_VM)) != 0)
             return -EINVAL;
         fork_start();
         ret = fork();
-- 
1.5.6.5.GIT

[Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

vfork() is a kind of fork, not thread despite CLONE_VM

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 linux-user/syscall.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index eba2c02..ae7a5a2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2787,7 +2787,7 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
     sigset_t sigmask;
 #endif
 
-    if (flags & CLONE_VM) {
+    if (!(flags & CLONE_VFORK) && (flags & CLONE_VM)) {
 #if defined(USE_NPTL)
         new_thread_info info;
         pthread_attr_t attr;
@@ -2856,8 +2856,8 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
 #endif
 #endif
     } else {
-        /* if no CLONE_VM, we consider it is a fork */
-        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2)) != 0)
+        /* we consider it is a fork or vfork */
+        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2 | CLONE_VFORK | CLONE_VM)) != 0)
             return -EINVAL;
         fork_start();
         ret = fork();
-- 
1.5.6.5.GIT

[Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

vfork() is a kind of fork, not thread despite CLONE_VM

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 linux-user/syscall.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 56b4138..124d14e 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2788,7 +2788,7 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
     sigset_t sigmask;
 #endif
 
-    if (flags & CLONE_VM) {
+    if (!(flags & CLONE_VFORK) && (flags & CLONE_VM)) {
 #if defined(USE_NPTL)
         new_thread_info info;
         pthread_attr_t attr;
@@ -2857,8 +2857,8 @@ static int do_fork(CPUState *env, unsigned int flags, abi_ulong newsp,
 #endif
 #endif
     } else {
-        /* if no CLONE_VM, we consider it is a fork */
-        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2)) != 0)
+        /* we consider it is a fork or vfork */
+        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2 | CLONE_VFORK | CLONE_VM)) != 0)
             return -EINVAL;
         fork_start();
         ret = fork();
-- 
1.5.6.5.GIT

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[andrzej zaborowski]

2008/9/18 Kirill A. Shutemov <kirill@shutemov.name>:
> vfork() is a kind of fork, not thread despite CLONE_VM

According to clone(2) it can be either, the only difference is that
vfork() suspends the parent process.  So if CLONE_VM is set, I think
still the pthread / clone way should be used and the child thread
should be waited on.

On the other hand the patch makes fork() and vfork() be treated identically?

Cheers

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On Sat, Sep 20, 2008 at 04:56:45AM +0200, andrzej zaborowski wrote:
> 2008/9/18 Kirill A. Shutemov <kirill@shutemov.name>:
> > vfork() is a kind of fork, not thread despite CLONE_VM
> 
> According to clone(2) it can be either, the only difference is that
> vfork() suspends the parent process.  So if CLONE_VM is set, I think
> still the pthread / clone way should be used and the child thread
> should be waited on.

vfork() suspends the parent process until a call of execve(2) or _exit(2).
If child call execnv(2) it replaces whole process, not only the thread.
If child call _exit(2) it stops while process, not only the thread.

> On the other hand the patch makes fork() and vfork() be treated identically?

$ cat usr/klibc/vfork.c
/*
 * vfork.c
 *
 * Emulate vfork() with fork() if necessary
 */

#include <unistd.h>
#include <klibc/compiler.h>
#include <klibc/sysconfig.h>

#if !_KLIBC_NO_MMU && !_KLIBC_REAL_VFORK
int vfork(void)
{
        return fork();
}
#endif

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On Sat, Sep 20, 2008 at 04:56:45AM +0200, andrzej zaborowski wrote:
> 2008/9/18 Kirill A. Shutemov <kirill@shutemov.name>:
> > vfork() is a kind of fork, not thread despite CLONE_VM
> 
> According to clone(2) it can be either, the only difference is that
> vfork() suspends the parent process.  So if CLONE_VM is set, I think
> still the pthread / clone way should be used and the child thread
> should be waited on.

Also process created with vfork(2) share stack with parent. Every thread
has its own stack.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[andrzej zaborowski]

2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> On Sat, Sep 20, 2008 at 04:56:45AM +0200, andrzej zaborowski wrote:
>> 2008/9/18 Kirill A. Shutemov <kirill@shutemov.name>:
>> > vfork() is a kind of fork, not thread despite CLONE_VM
>>
>> According to clone(2) it can be either, the only difference is that
>> vfork() suspends the parent process.  So if CLONE_VM is set, I think
>> still the pthread / clone way should be used and the child thread
>> should be waited on.
>
> vfork() suspends the parent process until a call of execve(2) or _exit(2).
> If child call execnv(2) it replaces whole process, not only the thread.
> If child call _exit(2) it stops while process, not only the thread.

Do you mean that's the current behavior in qemu?  That's not what clone(2) says.

>
>> On the other hand the patch makes fork() and vfork() be treated identically?
>
> $ cat usr/klibc/vfork.c
> /*
>  * vfork.c
>  *
>  * Emulate vfork() with fork() if necessary
>  */
>
> #include <unistd.h>
> #include <klibc/compiler.h>
> #include <klibc/sysconfig.h>
>
> #if !_KLIBC_NO_MMU && !_KLIBC_REAL_VFORK
> int vfork(void)
> {
>        return fork();
> }
> #endif

Well, that's libc. clone with CLONE_VFORK and without it are still not
the same thing.  CLONE_VM and CLONE_VFORK are ortogonal to one
another, not the opposite of.

Cheers

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

On Sat, Sep 20, 2008 at 02:45:57PM +0200, andrzej zaborowski wrote:
> 2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> > On Sat, Sep 20, 2008 at 04:56:45AM +0200, andrzej zaborowski wrote:
> >> 2008/9/18 Kirill A. Shutemov <kirill@shutemov.name>:
> >> > vfork() is a kind of fork, not thread despite CLONE_VM
> >>
> >> According to clone(2) it can be either, the only difference is that
> >> vfork() suspends the parent process.  So if CLONE_VM is set, I think
> >> still the pthread / clone way should be used and the child thread
> >> should be waited on.
> >
> > vfork() suspends the parent process until a call of execve(2) or _exit(2).
> > If child call execnv(2) it replaces whole process, not only the thread.
> > If child call _exit(2) it stops while process, not only the thread.
> 
> Do you mean that's the current behavior in qemu?  That's not what clone(2) says.

Currently, qemu with NPTL(I've tested on ARM EABI) on CLONE_VM create
thread using pthread interface. Every thread has its own stack.

vfork() is clone() with flags CLONE_VM and CLONE_VFORK. 

man vfork(2):

   Linux Description
       vfork(),  just  like  fork(2), creates a child process of the calling
       process.  For details and return value and errors, see fork(2).

       vfork() is a special case of clone(2).  It is used to create new pro-
       cesses without copying the page tables of the parent process.  It may
       be useful in performance sensitive applications where a child will be
       created which then immediately issues an execve(2).

       vfork()  differs  from  fork(2) in that the parent is suspended until
       the child makes a call to execve(2) or _exit(2).   The  child  shares
       all  memory  with its parent, including the stack, until execve(2) is
       issued by the child.  The child must  not  return  from  the  current
       function or call exit(3), but may call _exit(2).

       Signal handlers are inherited, but not shared.  Signals to the parent
       arrive after the child releases the parent's memory.

So, implementation vfork() using pthread is wrong.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[andrzej zaborowski]

2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> So, implementation vfork() using pthread is wrong.

Agreed, but implementation of vfork() using fork() is wrong, too. If
we allow a hack, it should be commented, the second thing that needs
to be commented is why the value of CLONE_VM flag is ignored if
CLONE_VFORK is set -- on Linux it's not ignored.

Cheers

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]

On Sat, Sep 20, 2008 at 03:52:55PM +0200, andrzej zaborowski wrote:
> 2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> > So, implementation vfork() using pthread is wrong.
> 
> Agreed, but implementation of vfork() using fork() is wrong, too.

Why? 

man 2 vfork():

BUGS
       It  is  rather  unfortunate  that Linux revived this specter from the
       past.  The BSD man page states: "This system call will be  eliminated
       when  proper system sharing mechanisms are implemented.  Users should
       not depend on the memory sharing semantics of vfork() as it will,  in
       that case, be made synonymous to fork(2)."

If any program doesn't work with vfork() implemented using fork(). it's 
program bug.


> If
> we allow a hack, it should be commented, the second thing that needs
> to be commented is why the value of CLONE_VM flag is ignored if
> CLONE_VFORK is set -- on Linux it's not ignored.

vfork() is a hack itself. It was introduced when fork() was very expensive.
Linux fork() is implemented using copy-on-write pages, so the only penalty 
incurred by fork() is the time and memory required to duplicate the parent's
page tables. It's quite cheap. So I think emulate vfork() using fork() is 
correct.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[andrzej zaborowski]

2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> On Sat, Sep 20, 2008 at 03:52:55PM +0200, andrzej zaborowski wrote:
>> 2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
>> > So, implementation vfork() using pthread is wrong.
>>
>> Agreed, but implementation of vfork() using fork() is wrong, too.
>
> Why?
>
> man 2 vfork():
>
> BUGS
>       It  is  rather  unfortunate  that Linux revived this specter from the
>       past.  The BSD man page states: "This system call will be  eliminated
>       when  proper system sharing mechanisms are implemented.  Users should
>       not depend on the memory sharing semantics of vfork() as it will,  in
>       that case, be made synonymous to fork(2)."
>
> If any program doesn't work with vfork() implemented using fork(). it's
> program bug.
>
>
>> If
>> we allow a hack, it should be commented, the second thing that needs
>> to be commented is why the value of CLONE_VM flag is ignored if
>> CLONE_VFORK is set -- on Linux it's not ignored.
>
> vfork() is a hack itself. It was introduced when fork() was very expensive.

Ok, perhaps I'm nit picking.  clone(2) specifies some semantics for
CLONE_VFORK regardless of the purpose and this implementation is
nowhere near these semantics.  I'll just add the same comment that
klibc has and push the patch.

Cheers

Re: [Qemu-devel] [PATCH] Fix vfork() syscall emulation

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

On Sat, Sep 20, 2008 at 04:35:25PM +0200, andrzej zaborowski wrote:
> 2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> > On Sat, Sep 20, 2008 at 03:52:55PM +0200, andrzej zaborowski wrote:
> >> 2008/9/20 Kirill A. Shutemov <kirill@shutemov.name>:
> >> > So, implementation vfork() using pthread is wrong.
> >>
> >> Agreed, but implementation of vfork() using fork() is wrong, too.
> >
> > Why?
> >
> > man 2 vfork():
> >
> > BUGS
> >       It  is  rather  unfortunate  that Linux revived this specter from the
> >       past.  The BSD man page states: "This system call will be  eliminated
> >       when  proper system sharing mechanisms are implemented.  Users should
> >       not depend on the memory sharing semantics of vfork() as it will,  in
> >       that case, be made synonymous to fork(2)."
> >
> > If any program doesn't work with vfork() implemented using fork(). it's
> > program bug.
> >
> >
> >> If
> >> we allow a hack, it should be commented, the second thing that needs
> >> to be commented is why the value of CLONE_VM flag is ignored if
> >> CLONE_VFORK is set -- on Linux it's not ignored.
> >
> > vfork() is a hack itself. It was introduced when fork() was very expensive.
> 
> Ok, perhaps I'm nit picking.  clone(2) specifies some semantics for
> CLONE_VFORK regardless of the purpose and this implementation is
> nowhere near these semantics.  I'll just add the same comment that
> klibc has and push the patch.

Thanks!

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]