2004-06-09 20:04:26

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [PATCH] Add FUTEX_CMP_REQUEUE futex op

On Mon, 7 Jun 2004 18:03:49 +0200
Martin Schwidefsky <[email protected]> wrote:

> --- linux-2.6/arch/s390/kernel/compat_wrapper.S Mon Jun 7 16:07:24 2004
> +++ linux-2.6-s390/arch/s390/kernel/compat_wrapper.S Mon Jun 7 16:07:53 2004
> @@ -1097,6 +1097,8 @@
> lgfr %r4,%r4 # int
> llgtr %r5,%r5 # struct compat_timespec *
> llgtr %r6,%r6 # u32 *
> + lgf %r0,164(%r15) # int
> + stg %r0,160(%r15)
> jg compat_sys_futex # branch to system call
>
> .globl sys32_setxattr_wrapper

Is it just me, or this could he above stand a use of STACK_FRAME_OVERHEAD
instead of 160? I envision a time when Ulrich Weigand comes out with
a gcc -fkernel, and at that time we'll need all such references
configurable.

> diff -urN linux-2.6/include/asm-s390/ptrace.h linux-2.6-s390/include/asm-s390/ptrace.h
> --- linux-2.6/include/asm-s390/ptrace.h Mon May 10 04:32:54 2004
> +++ linux-2.6-s390/include/asm-s390/ptrace.h Mon Jun 7 16:07:53 2004
> @@ -303,6 +303,7 @@
> */
> struct pt_regs
> {
> + unsigned long args[1];
> psw_t psw;

This worries me, together with
(__u32*)((addr_t) &__KSTK_PTREGS(child)->psw

Why not to place the necessary word outside of the struct?
It just logically doesn't belong. Might be just as easy to
do that mvc to other place.

I think I'll try to scope such an implemenation.

-- Pete


2004-06-09 21:56:56

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] Add FUTEX_CMP_REQUEUE futex op

On Wednesday 09 June 2004 22:04, Pete Zaitcev wrote:

> Is it just me, or this could he above stand a use of STACK_FRAME_OVERHEAD
> instead of 160? I envision a time when Ulrich Weigand comes out with
> a gcc -fkernel, and at that time we'll need all such references
> configurable.

It wouldn't hurt, but even if we get -mkernel support in gcc, that doesn't
mean that the stack frame size has to change: You can easily have %r15
point to 160 bytes above the register save area without actually using all
that space for saving registers. The only thing that would need to change is
the location of the backchain pointer.

> Why not to place the necessary word outside of the struct?
> It just logically doesn't belong. Might be just as easy to
> do that mvc to other place.

That actually was what Martin tried in his first implementation (well, the
last one before the one he submitted). It didn't work out because some code
relied on the stack starting right after pt_regs. Martin can probably clarify
that on Friday.

Arnd <><

2004-06-11 08:35:44

by Martin Schwidefsky

[permalink] [raw]
Subject: Re: [PATCH] Add FUTEX_CMP_REQUEUE futex op





> > --- linux-2.6/arch/s390/kernel/compat_wrapper.S Mon Jun 7
16:07:24 2004
> > +++ linux-2.6-s390/arch/s390/kernel/compat_wrapper.S Mon Jun
7 16:07:53 2004
> > @@ -1097,6 +1097,8 @@
> > lgfr %r4,%r4 # int
> > llgtr %r5,%r5 #
struct compat_timespec *
> > llgtr %r6,%r6 #
u32 *
> > + lgf %r0,164(%r15) # int
> > + stg %r0,160(%r15)
> > jg compat_sys_futex # branch to system call
> >
> > .globl sys32_setxattr_wrapper
>
> Is it just me, or this could he above stand a use of STACK_FRAME_OVERHEAD
> instead of 160? I envision a time when Ulrich Weigand comes out with
> a gcc -fkernel, and at that time we'll need all such references
> configurable.
No, the offset of the arguments > 5 on the stack is 96 bytes on 31 bit and
160 bytes on 64 bit, period. This won't change because it is ABI relevant.

> > diff -urN linux-2.6/include/asm-s390/ptrace.h linux-2.6-s390/include/asm-s390/ptrace.h
> > --- linux-2.6/include/asm-s390/ptrace.h Mon May 10 04:32:54 2004
> > +++ linux-2.6-s390/include/asm-s390/ptrace.h Mon Jun 7 16:07:53 2004
> > @@ -303,6 +303,7 @@
> > */
> > struct pt_regs
> > {
> > + unsigned long args[1];
> > psw_t psw;
>
> This worries me, together with
> (__u32*)((addr_t) &__KSTK_PTREGS(child)->psw
>
> Why not to place the necessary word outside of the struct?
> It just logically doesn't belong. Might be just as easy to
> do that mvc to other place.
Well my first implementation had the arguments outside the pt_regs. But
I came to the conclusion that this is wrong. First of all pt_regs is
supposed to describe what is put on the stack during a system call and
this includes the additional parameter. The second, more important
thing is that without a structure describing exactly what's put on the
stack it is impossible for lcrash or crash to decode the system entry
stack frame. This is something we definitly want to have.
I though about the implications of the pt_regs changes for some time.
Since the pt_regs structure is solely used to put the register on the
kernel stack I don't see a problem. Every other structure is independent
of it (at least in the 2.6 kernels).

blue skies,
Martin

Linux/390 Design & Development, IBM Deutschland Entwicklung GmbH
Sch?naicherstr. 220, D-71032 B?blingen, Telefon: 49 - (0)7031 - 16-2247
E-Mail: [email protected]