[uml-devel] sysemu

[uml-devel] sysemu

[Jeff Dike]

Call me a lamer, but I only now got around to looking at the sysemu
patch in detail.

As it stands, it can't be merged into mainline because it adds two
instructions to the system call fast path.  This is the sort of thing
that make Linus and others go ballistic.

The duplication of syscall_trace as syscall_emulate is also pretty
ugly.

Below, see a patch which tweaks things to eliminate both problems.
Now, PT_SCEMU is set in conjunction with PT_TRACESYS, rather than
one or the other, but not both, being set.

This means that the PT_TRACESYS test in entry.S covers both cases, and
the fast path is unchanged.

The other change was to make syscall_trace return zero or non-zero,
according as it should run the system call or immediately return.
This adds a line to syscall_trace and eliminates syscall_emulate.

The updated patch is 83 lines of diff rather than the original 128.

I've run it with both a little test case, and a tt mode UML.  The getpid()
benchmark there showed little improvement (~1-2%), surprisingly.  I'm going
to profile to see what's going on there.

Have a look and see if it looks reasonable.

				Jeff

diff -Naur linux-2.4.22-1.2197.nptl/arch/i386/kernel/entry.S linux-2.4.22-1.2197.nptl-sysemu/arch/i386/kernel/entry.S
--- linux-2.4.22-1.2197.nptl/arch/i386/kernel/entry.S	2004-07-01 15:04:41.000000000 -0400
+++ linux-2.4.22-1.2197.nptl-sysemu/arch/i386/kernel/entry.S	2004-08-21 12:02:25.000000000 -0400
@@ -240,7 +240,9 @@
 	ALIGN
 tracesys:
 	movl $-ENOSYS,EAX(%esp)
-	call SYMBOL_NAME(syscall_trace)
+	call SYMBOL_NAME(syscall_trace) # non-zero return from syscall_trace
+	cmpl $0, %eax			# means that the system call was
+	jne ret_from_sys_call		# nullified
 	movl ORIG_EAX(%esp),%eax
 	cmpl $(NR_syscalls),%eax
 	jae tracesys_exit
diff -Naur linux-2.4.22-1.2197.nptl/arch/i386/kernel/ptrace.c linux-2.4.22-1.2197.nptl-sysemu/arch/i386/kernel/ptrace.c
--- linux-2.4.22-1.2197.nptl/arch/i386/kernel/ptrace.c	2004-07-01 15:04:37.000000000 -0400
+++ linux-2.4.22-1.2197.nptl-sysemu/arch/i386/kernel/ptrace.c	2004-08-21 12:05:12.000000000 -0400
@@ -351,6 +351,7 @@
 		  }
 		  break;
 
+	case PTRACE_SCEMU: /* continue and replace next syscall */
 	case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall */
 	case PTRACE_CONT: { /* restart after signal. */
 		long tmp;
@@ -360,6 +361,8 @@
 			break;
 		if (request == PTRACE_SYSCALL)
 			child->ptrace |= PT_TRACESYS;
+		else if (request == PTRACE_SCEMU)
+			child->ptrace |= PT_TRACESYS | PT_SCEMU;
 		else
 			child->ptrace &= ~PT_TRACESYS;
 		child->exit_code = data;
@@ -396,7 +399,7 @@
 		ret = -EIO;
 		if ((unsigned long) data > _NSIG)
 			break;
-		child->ptrace &= ~PT_TRACESYS;
+		child->ptrace &= ~(PT_TRACESYS | PT_SCEMU);
 		if ((child->ptrace & PT_DTRACE) == 0) {
 			/* Spurious delayed TF traps may occur */
 			child->ptrace |= PT_DTRACE;
@@ -512,7 +515,7 @@
 	return ret;
 }
 
-asmlinkage void syscall_trace(void)
+asmlinkage int syscall_trace(void)
 {
 	if ((current->ptrace & (PT_PTRACED|PT_TRACESYS)) !=
 						(PT_PTRACED|PT_TRACESYS))
@@ -534,4 +537,7 @@
 		current->exit_code = 0;
 	}
 	recalc_sigpending();
+
+	/* 1 if nullifying the syscall, 0 if running it */
+	return(current->ptrace & PT_SCEMU);
 }
diff -Naur linux-2.4.22-1.2197.nptl/include/linux/ptrace.h linux-2.4.22-1.2197.nptl-sysemu/include/linux/ptrace.h
--- linux-2.4.22-1.2197.nptl/include/linux/ptrace.h	2004-07-01 15:05:23.000000000 -0400
+++ linux-2.4.22-1.2197.nptl-sysemu/include/linux/ptrace.h	2004-08-21 12:05:16.000000000 -0400
@@ -22,6 +22,7 @@
 #define PTRACE_DETACH		0x11
 
 #define PTRACE_SYSCALL		  24
+#define PTRACE_SCEMU		  31
 
 /* 0x4200-0x4300 are reserved for architecture-independent additions.  */
 #define PTRACE_SETOPTIONS	0x4200
diff -Naur linux-2.4.22-1.2197.nptl/include/linux/sched.h linux-2.4.22-1.2197.nptl-sysemu/include/linux/sched.h
--- linux-2.4.22-1.2197.nptl/include/linux/sched.h	2004-07-01 15:05:23.000000000 -0400
+++ linux-2.4.22-1.2197.nptl-sysemu/include/linux/sched.h	2004-08-21 11:59:18.000000000 -0400
@@ -573,6 +573,8 @@
 #define PT_TRACE_VFORK_DONE 0x00000100
 #define PT_TRACE_EXIT   0x00000200
 #define PT_DTRACE	0x00000400      /* delayed trace (used on m68k, i386) */
+#define PT_SCEMU	0x00000800	/* syscall emulation for UML */
+
 
 #define is_dumpable(tsk)    ((tsk)->task_dumpable && (tsk)->mm && (tsk)->mm->dumpable)
 



-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] sysemu

[Alexander Bochmann]

...on Mon, Aug 23, 2004 at 12:54:50AM -0400, Jeff Dike wrote:

 > The updated patch is 83 lines of diff rather than the original 128.

Thanks. The sysemu incremental patch for 2.4.26-3um  
also fixes a reoccuring crash I had while using the 
patch from sysemu.sf.net...

Alex.



-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] sysemu

[BlaisorBlade]

On Monday 23 August 2004 06:54, Jeff Dike wrote:
> Call me a lamer, but I only now got around to looking at the sysemu
> patch in detail.

> As it stands, it can't be merged into mainline because it adds two
> instructions to the system call fast path.  This is the sort of thing
> that make Linus and others go ballistic.

> The duplication of syscall_trace as syscall_emulate is also pretty
> ugly.

> Below, see a patch which tweaks things to eliminate both problems.
> Now, PT_SCEMU is set in conjunction with PT_TRACESYS, rather than
> one or the other, but not both, being set.

This is not needed. You can still set just one of them (which is cleaner) and 
replace, in the fast path, 
        testb $0x02,tsk_ptrace(%ebx)    # PT_TRACESYS
with
        testb $0x22,tsk_ptrace(%ebx)    # PT_TRACESYS | PT_SYSEMU

> This means that the PT_TRACESYS test in entry.S covers both cases, and
> the fast path is unchanged.

> The other change was to make syscall_trace return zero or non-zero,
> according as it should run the system call or immediately return.
> This adds a line to syscall_trace and eliminates syscall_emulate.


> I've run it with both a little test case, and a tt mode UML.  The getpid()
> benchmark there showed little improvement (~1-2%), surprisingly.  I'm going
> to profile to see what's going on there.
What is the surprising thing? Did you expect more or less improvement?

You ran the getpid() benchmark with TT or with SKAS mode? Also, please note 
that many people said that glibc caches the getpid() result, so some other 
benchmark should be used (any sys_ni_syscall should be good, IMHO, especially 
it if can exist somewhere. 137 is reserved for afs_syscall, so glibc must run 
the syscall and it's even better for the benchmark.
Also, saving two context switches for syscall may still be a little saving, 
compared to the signal delivery. But, still, I don't understand why signal 
delivering is slower than context switching.

> Have a look and see if it looks reasonable.


> diff -Naur linux-2.4.22-1.2197.nptl/arch/i386/kernel/ptrace.c
> linux-2.4.22-1.2197.nptl-sysemu/arch/i386/kernel/ptrace.c ---
> linux-2.4.22-1.2197.nptl/arch/i386/kernel/ptrace.c	2004-07-01
> 15:04:37.000000000 -0400 +++
> linux-2.4.22-1.2197.nptl-sysemu/arch/i386/kernel/ptrace.c	2004-08-21
> 12:05:12.000000000 -0400 @@ -351,6 +351,7 @@
>  		  }
>  		  break;
>
> +	case PTRACE_SCEMU: /* continue and replace next syscall */
>  	case PTRACE_SYSCALL: /* continue and stop at next (return from) syscall
> */ case PTRACE_CONT: { /* restart after signal. */
>  		long tmp;
> @@ -360,6 +361,8 @@
>  			break;
>  		if (request == PTRACE_SYSCALL)
>  			child->ptrace |= PT_TRACESYS;
> +		else if (request == PTRACE_SCEMU)
> +			child->ptrace |= PT_TRACESYS | PT_SCEMU;
>  		else
>  			child->ptrace &= ~PT_TRACESYS;
Here there needs to be another change:
 
-			child->ptrace &= ~PT_TRACESYS;
+			child->ptrace &= ~(PT_TRACESYS | PT_SCEMU);

Do you agree?

>  		child->exit_code = data;
> @@ -396,7 +399,7 @@
>  		ret = -EIO;
>  		if ((unsigned long) data > _NSIG)
>  			break;
> -		child->ptrace &= ~PT_TRACESYS;
> +		child->ptrace &= ~(PT_TRACESYS | PT_SCEMU);
>  		if ((child->ptrace & PT_DTRACE) == 0) {
>  			/* Spurious delayed TF traps may occur */
>  			child->ptrace |= PT_DTRACE;
> @@ -512,7 +515,7 @@
>  	return ret;
>  }
>
> -asmlinkage void syscall_trace(void)
> +asmlinkage int syscall_trace(void)
>  {
>  	if ((current->ptrace & (PT_PTRACED|PT_TRACESYS)) !=
>  						(PT_PTRACED|PT_TRACESYS))
> @@ -534,4 +537,7 @@
>  		current->exit_code = 0;
>  	}
>  	recalc_sigpending();
> +
> +	/* 1 if nullifying the syscall, 0 if running it */
> +	return(current->ptrace & PT_SCEMU);
>  }
Well, obviously the comment must say "!= 0", not "1". And return() is not the 
official coding style. However for 2.4 this does not matter (Marcelo will 
never merge this).

> +#define PTRACE_SCEMU		  31

> +#define PT_SCEMU	0x00000800	/* syscall emulation for UML */
As said, please rename them to PTRACE_SYSEMU and PT_SYSEMU; otherwise, I'll 
change the guest patch to use PTRACE_SCEMU as name (calling "31" in different 
ways is as good as using 31 directly).

Now I'm doing a similar work for the 2.6 patch, with some bugfixing, and then 
I'll start sending it on the LKML to review, discuss and eventually merge it.

For instance (luckily I don't see any other bug for now):

+               clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE|
TIF_SYSCALL_EMU);
will never work, because TIF_* are not bitmasks, unlike _TIF_*. I also don't 
understand why most code works with this API, but I'll follow it.
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel