code review 4817054: runtime, syscall: use the vdso page on linux x86 for fa...

runtime, syscall: use the vdso page on linux x86 for faster syscalls instead of int $0x80.
8l: fix handling CALL $(constant) code generated by 8a.
8a,8l: add indirect call instruction: CALL *data(SB).

[paulzhol, 2011-07-27 14:33:58]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[paulzhol, 2011-07-27 14:34:34]

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys.s
File src/pkg/runtime/linux/386/sys.s (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys...
src/pkg/runtime/linux/386/sys.s:119: INT	$0x80
using VDSO_SYSENTER instead causes http/cgi test to fail (SIGPIPE)

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys...
src/pkg/runtime/linux/386/sys.s:180: INT	$0x80
using VDSO_SYSENTER instead causes cgo to segfault when building os/user.

[rsc, 2011-07-27 16:04:51]

this is pretty ugly.
how much faster is it?

[paulzhol, 2011-07-27 16:51:16]

On Wed, Jul 27, 2011 at 7:04 PM, Russ Cox <rsc@golang.org> wrote:
> this is pretty ugly.
> how much faster is it?
>

I'm not sure I have the right benchmark to measure this..
in pkg/http
time gotest -bench=BenchmarkClientServer

for int $0x80:
http_test.BenchmarkClientServer    10000            232124 ns/op

real    0m5.336s
user    0m3.751s
sys     0m1.666s

vs

for vdso:
http_test.BenchmarkClientServer    10000            226915 ns/op

real    0m5.267s
user    0m3.746s
sys     0m1.594s

[paulzhol, 2011-07-28 07:15:21]

ok this is pretty synthetic, but..

import "testing"
import "syscall"

func BenchmarkSyscallRate(b *testing.B) {
        for i := 0; i < b.N; i++ {
                syscall.Getpid()
        }
}

/8.out -test.bench='SyscallRate'
testing: warning: no tests to run
PASS
syscall_bench.BenchmarkSyscallRate       5000000               305 ns/op

./8.out_vdso -test.bench='SyscallRate'
testing: warning: no tests to run
PASS
syscall_bench.BenchmarkSyscallRate      20000000               114 ns/op

on my Thinkpad T61 (Core2Duo T8100 2.1Ghz openSUSE 11.4 x64)

Pavel

[rsc, 2011-08-03 20:15:03]

Is a similar change necessary on amd64?

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/rt0.s
File src/pkg/runtime/linux/386/rt0.s (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/rt0...
src/pkg/runtime/linux/386/rt0.s:11: TEXT runtime·_fallback_vdso(SB),7,$0
These should probably be in sys.s
where they get used.

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sig...
File src/pkg/runtime/linux/386/signal.c (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sig...
src/pkg/runtime/linux/386/signal.c:200: 
Add

return;

right here and make sure the tree runs.
Then take it out.  :-)

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys.s
File src/pkg/runtime/linux/386/sys.s (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys...
src/pkg/runtime/linux/386/sys.s:119: INT	$0x80
On 2011/07/27 14:34:34, paulzhol wrote:
> using VDSO_SYSENTER instead causes http/cgi test to fail (SIGPIPE)

Please put that in a comment in the file.
I can believe that you have to INT $0x80 to
sigreturn.  Sigreturn is magic.

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys...
src/pkg/runtime/linux/386/sys.s:180: INT	$0x80
On 2011/07/27 14:34:34, paulzhol wrote:
> using VDSO_SYSENTER instead causes cgo to segfault when building os/user.

Add comment here too.
Clone is even more magic, because it returns in the
child on a different stack.  If you throw away the
VDSO_SYSENTER macro and use CALL instructions this will
be clearer:

// Cannot CALL *runtime·vdso(SB) here, because
// the stack changes during the system call
INT $0x80

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/mkasmh.sh
File src/pkg/runtime/mkasmh.sh (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/mkasmh.sh#new...
src/pkg/runtime/mkasmh.sh:55: echo '#define	VDSO_SYSENTER	BYTE $0xFF; BYTE
$0x15; LONG $runtime·_vdso(SB)	// gas syntax:call *runtime·_vdso'
I think 8a supports this.  It should be

CALL *runtime·_vdso(SB)

Note the *.

http://codereview.appspot.com/4817054/diff/2001/src/pkg/syscall/asm_linux_386.s
File src/pkg/syscall/asm_linux_386.s (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/syscall/asm_linux_386...
src/pkg/syscall/asm_linux_386.s:9: #include "../runtime/386/asm.h"
Should be able to drop this once
you're using CALL instructions.

[iant2, 2011-08-03 20:32:02]

rsc@golang.org writes:

> Is a similar change necessary on amd64?

No.  amd64 already uses syscall.

Ian

[iant, 2011-08-03 20:39:58]

LGTM

[paulzhol, 2011-08-05 16:14:23]

Hi!

CALL *runtime·_vdso does not parse (CALL *0x12345 can be derived from omem->con
though). 
I've tried adding a '*' nam rule to spec3 but 8l complains about "undefined:
runtime._vdso" in pass.c:315 although running 8l -a will produce the correct
opcode:
805356d ff1500000000         | (113)    CALL    ,runtime._vdso+0(SB)

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/rt0.s
File src/pkg/runtime/linux/386/rt0.s (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/rt0...
src/pkg/runtime/linux/386/rt0.s:11: TEXT runtime·_fallback_vdso(SB),7,$0
runtime·_vdso is also called from syscall/asm_linux_386.s

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sig...
File src/pkg/runtime/linux/386/signal.c (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sig...
src/pkg/runtime/linux/386/signal.c:199: runtime·_vdso =
(uint32)&runtime·_fallback_vdso;
I've skipped the initial assignment to runtime·_vdso by making fallback_vdso the
default value in rt0.s.

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys.s
File src/pkg/runtime/linux/386/sys.s (right):

http://codereview.appspot.com/4817054/diff/2001/src/pkg/runtime/linux/386/sys...
src/pkg/runtime/linux/386/sys.s:180: INT	$0x80
Why is the stack change affecting an address in the .data segment? The child
gettid call is working.

[rsc, 2011-08-05 16:17:42]

> Why is the stack change affecting an address in the .data segment? The
> child gettid call is working.

You can't have two threads using the same stack.
If you CALL runtime.vdso then the child is still using
the parent's stack, during its RET instruction.

Russ

[paulzhol, 2011-08-12 17:23:50]

Hi! 
I got 8l to work with the CALL *data(SB) 8a is now generating but it's a little
hackish.
The tree builds and passes all test though, please take another look.

Thanks,
Pavel

[rsc, 2011-08-16 21:25:31]

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/optab.c
File src/cmd/8l/optab.c (right):

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/optab.c#newcode262
src/cmd/8l/optab.c:262: Ynone,	Yi32,	Zcallcon,	2,
This change looks wrong.  Why reorder this list?
Why change this line's footprint from 2 to 1?
It looks like this line (before) was trying to use the 0xe8
in the list; now it is getting 0xff, 0x00.

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c
File src/cmd/8l/pass.c (right):

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c#newcode315
src/cmd/8l/pass.c:315: if((s->type&~SSUB) == SBSS) {
what is this for?

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/span.c
File src/cmd/8l/span.c (right):

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/span.c#newcode1123
src/cmd/8l/span.c:1123: if(t[2] == Zcallcon) {
This is changing existing behavior.
That seems like a recipe for breaking things.
I expected a new case.

[paulzhol, 2011-08-17 05:53:29]

I though I could reuse Zcallcon, or at least fix it..
right now without my changes CALL $0x12345 will cause 8l to segfault (Zcallcon
branch sets the relocation info without setting the relocation symbol).

CALL *0x12345 produces valid code but it's not use Zcallcon.
I'll try something else.

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c
File src/cmd/8l/pass.c (right):

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c#newcode315
src/cmd/8l/pass.c:315: if((s->type&~SSUB) == SBSS) {
I'm not sure how to solve this, but the new instruction 8a produces has the
symbol type set to SBSS instead of STEXT so I had to add this to prevent the
next line from triggering.

[rsc, 2011-08-17 20:52:15]

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c
File src/cmd/8l/pass.c (right):

http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c#newcode315
src/cmd/8l/pass.c:315: if((s->type&~SSUB) == SBSS) {
On 2011/08/17 05:53:29, paulzhol wrote:
> I'm not sure how to solve this, but the new instruction 8a produces has the
> symbol type set to SBSS instead of STEXT so I had to add this to prevent the
> next line from triggering.

Probably the p->as == ACALL above should say

p->as == ACALL && p->to.type == D_XXX

but I don't know off the top of my head what the XXX is.
Whatever it currently is.

[paulzhol, 2011-08-17 21:17:25]

Thanks for the pointer!
What about CALL $0x12345, is it even valid ? It probably should not
cause 8l to segfault on a null dereference though.

Pavel

On Wed, Aug 17, 2011 at 11:52 PM,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c
> File src/cmd/8l/pass.c (right):
>
> http://codereview.appspot.com/4817054/diff/25001/src/cmd/8l/pass.c#newcode315
> src/cmd/8l/pass.c:315: if((s->type&~SSUB) == SBSS) {
> On 2011/08/17 05:53:29, paulzhol wrote:
>>
>> I'm not sure how to solve this, but the new instruction 8a produces
>
> has the
>>
>> symbol type set to SBSS instead of STEXT so I had to add this to
>
> prevent the
>>
>> next line from triggering.
>
> Probably the p->as == ACALL above should say
>
> p->as == ACALL && p->to.type == D_XXX
>
> but I don't know off the top of my head what the XXX is.
> Whatever it currently is.
>
> http://codereview.appspot.com/4817054/
>

[rsc, 2011-08-17 22:00:21]

> What about CALL $0x12345, is it even valid ?

I don't believe it is.  CALL has an implicit 'address of' in it.
You say CALL main(SB), not CALL $main(SB).  The $ is
implicit.  So CALL 0x12345 should be a call to the constant
address 0x12345.  To call the value stored at that address
you'd do CALL *0x12345.  CALL $0x12345 is an error.

Russ

[paulzhol, 2011-08-21 19:48:32]

Hi!

Changeset 5520e99adddd was "accept CALL $(constant) to call absolute PC" way
back in 2009, and it added the Zcallcon, and f8009f24467f Zjmpcon.
Changeset 749a9400cd2a "8l: function at a time code layout" added the relocation
code and convert to relative jump. I think it introduces the 8l segfault on CALL
$constant (the change to data.c fixes it).

PTAL

Pavel

http://codereview.appspot.com/4817054/diff/40001/src/cmd/8l/span.c
File src/cmd/8l/span.c (right):

http://codereview.appspot.com/4817054/diff/40001/src/cmd/8l/span.c#newcode286
src/cmd/8l/span.c:286: //	print("*Ycol\n");
Should we have a new Y*** type for this ?

http://codereview.appspot.com/4817054/diff/40001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/4817054/diff/40001/src/cmd/ld/data.c#newcode186
src/cmd/ld/data.c:186: o = (r->sym ? symaddr(r->sym) : 0) + r->add - (s->value +
r->off + r->siz);
maybe the r->sym nil test should be moved into symaddr instead ?

[paulzhol, 2011-08-29 05:30:26]

Ping ?

[rsc, 2011-08-29 13:38:09]

LGTM

please fix + hg mail

http://codereview.appspot.com/4817054/diff/43001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/4817054/diff/43001/src/cmd/ld/data.c#newcode186
src/cmd/ld/data.c:186: o = (r->sym ? symaddr(r->sym) : 0) + r->add - (s->value +
r->off + r->siz);
Please write this without ?: for clarity.

o = 0;
if(r->sym)
    o += symaddr(r->sym);
o += r->add - (s->value + r->off + r->siz);

http://codereview.appspot.com/4817054/diff/43001/src/pkg/runtime/linux/386/sys.s
File src/pkg/runtime/linux/386/sys.s (right):

http://codereview.appspot.com/4817054/diff/43001/src/pkg/runtime/linux/386/sy...
src/pkg/runtime/linux/386/sys.s:141: // cannot use CALL *runtime·_vdso(SB) here,
it causes http/cgi test to fail(SIGPIPE)
// Sigreturn expects same SP as signal handler,
// so cannot CALL *runtime._vsdo(SB) here.

[paulzhol, 2011-08-29 14:07:02]

Hello rsc@golang.org, iant@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-08-29 14:36:12]

*** Submitted as http://code.google.com/p/go/source/detail?r=6faab0699280 ***

runtime, syscall: use the vdso page on linux x86 for faster syscalls instead of
int $0x80.
8l: fix handling CALL $(constant) code generated by 8a.
8a,8l: add indirect call instruction: CALL *data(SB).

R=rsc, iant
CC=golang-dev
http://codereview.appspot.com/4817054

Committer: Russ Cox <rsc@golang.org>