Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2010-01-11 Thread CAI Qian 
Thanks for pointing out. Sorry for the false alarm.

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-14 Thread Jan Kratochvil 
On Wed, 09 Dec 2009 19:12:41 +0100, Oleg Nesterov wrote:
   while the '.func_name' is the text address.
 
  tried to change the code to
 
  REGS_ACCESS (regs, nip) = (unsigned long) .raise_sigusr2
 
  but gcc doesn't like this ;)
...
 Yes, I verified the patch below fixes step-jump-cont.c on
 ibm-js20-02.lab.bos.redhat.com.

Checked-in a similar patch but same as used now in other testcases, sorry for
not using the patch of yours.


Regards,
Jan


--- step-jump-cont.c8 Dec 2008 18:23:41 -   1.12
+++ step-jump-cont.c14 Dec 2009 11:38:37 -  1.13
@@ -213,6 +213,24 @@ int main (void)
   REGS_ACCESS (regs, eip) = (unsigned long) raise_sigusr2;
 #elif defined __x86_64__
   REGS_ACCESS (regs, rip) = (unsigned long) raise_sigusr2;
+#elif defined __powerpc64__
+  {
+/* ppc64 `raise_sigusr2' resolves to the function descriptor.  */
+union
+  {
+   void (*f) (void);
+   struct
+ {
+   void *entry;
+   void *toc;
+ }
+   *p;
+  }
+const func_u = { raise_sigusr2 };
+
+REGS_ACCESS (regs, nip) = (unsigned long) func_u.p-entry;
+REGS_ACCESS (regs, gpr[2]) = (unsigned long) func_u.p-toc;
+  }
 #elif defined __powerpc__
   REGS_ACCESS (regs, nip) = (unsigned long) raise_sigusr2;
 #else

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-09 Thread Oleg Nesterov 
On 12/08, Ananth N Mavinakayanahalli wrote:

 On Mon, Dec 07, 2009 at 07:05:40PM +0100, Oleg Nesterov wrote:
  On 12/07, Oleg Nesterov wrote:
  
   On 12/07, Jan Kratochvil wrote:
   
On Mon, 07 Dec 2009 15:24:51 +0100, Oleg Nesterov wrote:
 But. raise_sigusr2 is not equal to the actual address of 
 raise_sigusr2(),
 this value points to the thunk (I do not know the correct English 
 term)
   
ppc64 calls it function descriptor (GDB
ppc64_linux_convert_from_func_ptr_addr):
   For PPC64, a function descriptor is a TOC entry,
  
   Thanks Jan.
  
in a data section,
  
   Yes!
  
   Now I can't understand how this test-case could ever work on ppc.
   step-jump-cont does:
  
 regs-nip = raise_sigusr2;  --- points to data section
 ptrace(PTRACE_CONT);
  
   of course, the tracee gets SIGSEGV, this section is not executable.
 
  Hmm. Looks like, powerpc means a lot of different hardware, and
  _PAGE_EXEC may be 0. I didn't notice this when I quickly grepped
  arch/powerpc/
 
  IOW, perhaps on some machines r implies x ?
 
  Is yes, this can explain why the results differ on different
  machines.

 Well, powerpc 32-bit adheres to the SVR4 ABI, while powerpc 64-bit uses
 the PPC64-ELF ABI (http://refspecs.linuxfoundation.org/ELF/ppc64/). The
 64bit ABI uses function descriptors and the 'func_name' is the data
 address,

Cai, Ananth, thank you.

So. I think we can forget about the possible kernel problems (and
in any case we can rule out utrace).

The test-case just wrong and should be fixed. The tracee can't execute
the function descriptor in data section, that is why it gets SIGSEGV.

 while the '.func_name' is the text address.

tried to change the code to

REGS_ACCESS (regs, nip) = (unsigned long) .raise_sigusr2

but gcc doesn't like this ;)

 (See
 handle_rt_signal64 in arch/powerpc/kernel/signal_64.c and
 kprobe_lookup_name in arch/powerpc/include/asm/kprobes.h.

Thanks... looking at handle_rt_signal64(), looks like we should
also set regs-gpr[2] = funct_desc_ptr-toc if we change regs-nip


I hope someone who understand powerpc could fix the test-case ;)

Oleg.

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-08 Thread Ananth N Mavinakayanahalli 
On Mon, Dec 07, 2009 at 01:43:27PM +0100, Oleg Nesterov wrote:
 On 12/06, CAI Qian wrote:
 
 Ananth, could you please confirm once again that step-jump-cont (from
 ptrace-tests testsuite) not fail on your machine? If yes, please tell
 me the version of glibc/gcc. Is PTRACE_GETREGS defined on your machine?

Hi Oleg,

It works for me on a Fedora 12 machine.

[ana...@mjs22lp1 ptrace-tests]$ gcc --version
gcc (GCC) 4.4.2 20091027 (Red Hat 4.4.2-7)

[ana...@mjs22lp1 ptrace-tests]$ rpm -qa |grep glibc
glibc-common-2.11-2.ppc
glibc-2.11-2.ppc64
glibc-devel-2.11-2.ppc
glibc-static-2.11-2.ppc
glibc-2.11-2.ppc
glibc-devel-2.11-2.ppc64
glibc-headers-2.11-2.ppc

And yes, PTRACE_GETREGS is defined in /usr/include/asm/ptrace.h

Ananth

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-08 Thread Ananth N Mavinakayanahalli 
On Mon, Dec 07, 2009 at 07:05:40PM +0100, Oleg Nesterov wrote:
 On 12/07, Oleg Nesterov wrote:
 
  On 12/07, Jan Kratochvil wrote:
  
   On Mon, 07 Dec 2009 15:24:51 +0100, Oleg Nesterov wrote:
But. raise_sigusr2 is not equal to the actual address of 
raise_sigusr2(),
this value points to the thunk (I do not know the correct English 
term)
  
   ppc64 calls it function descriptor (GDB
   ppc64_linux_convert_from_func_ptr_addr):
  For PPC64, a function descriptor is a TOC entry,
 
  Thanks Jan.
 
   in a data section,
 
  Yes!
 
  Now I can't understand how this test-case could ever work on ppc.
  step-jump-cont does:
 
  regs-nip = raise_sigusr2;  --- points to data section
  ptrace(PTRACE_CONT);
 
  of course, the tracee gets SIGSEGV, this section is not executable.
 
 Hmm. Looks like, powerpc means a lot of different hardware, and
 _PAGE_EXEC may be 0. I didn't notice this when I quickly grepped
 arch/powerpc/
 
 IOW, perhaps on some machines r implies x ?
 
 Is yes, this can explain why the results differ on different
 machines.

Well, powerpc 32-bit adheres to the SVR4 ABI, while powerpc 64-bit uses
the PPC64-ELF ABI (http://refspecs.linuxfoundation.org/ELF/ppc64/). The
64bit ABI uses function descriptors and the 'func_name' is the data
address, while the '.func_name' is the text address. (See
handle_rt_signal64 in arch/powerpc/kernel/signal_64.c and
kprobe_lookup_name in arch/powerpc/include/asm/kprobes.h.

Ananth

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-07 Thread caiqian 

 I'll try to investigate, but currently I am all confused, and I
 suspect we have some user-space issues. If only I knew something about ppc...

Sorry for the confusing.

 
 Ananth, could you please confirm once again that step-jump-cont (from
 ptrace-tests testsuite) not fail on your machine? If yes, please tell
 me the version of glibc/gcc. Is PTRACE_GETREGS defined on your
 machine?

Funny enough. The above failure only seen on that particular system so far. In 
fact, different PPC64 systems have different results there (roland's git tree + 
your lockless patch).

ibm-js20-02.lab.bos.redhat.com
FAIL: watchpoint
ppc-dabr-race: ./../tests/ppc-dabr-race.c:141: handler_fail: Assertion `0' 
failed.
/bin/sh: line 5: 16928 Aborted ${dir}$tst
FAIL: ppc-dabr-race
syscall-reset: ./../tests/syscall-reset.c:95: main: Assertion 
`(*__errno_location ()) == 38' failed.
errno 14 (Bad address)
unexpected child status 67f
FAIL: syscall-reset
step-fork: ./../tests/step-fork.c:56: handler_fail: Assertion `0' failed.
/bin/sh: line 5: 31144 Aborted ${dir}$tst
FAIL: step-fork

ibm-js22-02.rhts.bos.redhat.com
ibm-js12-04.rhts.bos.redhat.com
ibm-js12-05.rhts.bos.redhat.com
Looks like failed only for syscall-reset and step-fork, as we have discussed 
before. I'll be reserving ibm-js20-02.lab.bos.redhat.com at the moment.

Thanks,
CAI Qian

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-07 Thread Oleg Nesterov 
On 12/07, caiq...@redhat.com wrote:

  Ananth, could you please confirm once again that step-jump-cont (from
  ptrace-tests testsuite) not fail on your machine? If yes, please tell
  me the version of glibc/gcc. Is PTRACE_GETREGS defined on your
  machine?

 Funny enough. The above failure only seen on that particular system so far.
 In fact, different PPC64 systems have different results there (roland's git
 tree + your lockless patch).

Great! thanks.

OK, I seem to understand what happens, but I can not explain WHY does
this happen on that machine.

Once again. The tracer changes the tracee's instruction pointer to
the adrress of raise_sigusr2(), and resumes the tracee. The tracee
gets SIGSEGV right after that.

But. raise_sigusr2 is not equal to the actual address of raise_sigusr2(),
this value points to the thunk (I do not know the correct English term)
which contains the actual address:

(gdb) disassemble 0x100118c0
Dump of assembler code for function raise_sigusr2:
0x100118c0 raise_sigusr2+0:   .long 0x0    
SIGSEGV
0x100118c4 raise_sigusr2+4:   .long 0x1ab0 
aof raise_sigusr2()
0x100118c8 raise_sigusr2+8:   .long 0x0

And!!! this thunk does NOT live in .text, and vma does NOT have
VM_EXEC bit!

# cat /proc/30494/maps
0010-0012 r-xp  00:00 0 
 [vdso]
1000-1001 r-xp  fd:00 59262 
 /root/TST/sjc
1001-1002 rw-p  fd:00 59262 
 /root/TST/sjc

That is why the tracee gets SIGSEGV, and this is correct.


Cai, perhaps you could give me access to another ppc machine where
this test does not fail?

Or, could you please run the trivial program below on that machine?

Oleg.

#include stdio.h
#include stdlib.h
#include unistd.h

void my_func(void)
{
}

int main(void)
{
char cmd[128];

printf(ptr: %p\n, my_func);

sprintf(cmd, cat /proc/%d/maps, getpid());
system(cmd);

return 0;
}

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-07 Thread Jan Kratochvil 
On Mon, 07 Dec 2009 15:24:51 +0100, Oleg Nesterov wrote:
 But. raise_sigusr2 is not equal to the actual address of raise_sigusr2(),
 this value points to the thunk (I do not know the correct English term)

ppc64 calls it function descriptor (GDB
ppc64_linux_convert_from_func_ptr_addr):
   For PPC64, a function descriptor is a TOC entry, in a data section,
   which contains three words: the first word is the address of the
   function, the second word is the TOC pointer (r2), and the third word
   is the static chain value.

(gdb) x/8gx 0x805b6f6258
0x805b6f6258 open:0x00805b65cf68  0x00805b702ac0
0x805b6f6268 open64:  0x00805b65d010  0x00805b702ac0

(gdb) x/20i 0x00805b65cf68
0x805b65cf68 .__GI___open:lwz r10,-30432(r13)
0x805b65cf6c .__GI___open+4:  cmpwi   r10,0
0x805b65cf70 .__GI___open+8:  bne-0x805b65cf84 .__GI___open+28

(gdb) info sym 0x00805b702ac0
last_nip in section .bss

I was not aware there is any third word before and I do not see it there.


Regards,
Jan

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-07 Thread Oleg Nesterov 
On 12/07, Jan Kratochvil wrote:

 On Mon, 07 Dec 2009 15:24:51 +0100, Oleg Nesterov wrote:
  But. raise_sigusr2 is not equal to the actual address of 
  raise_sigusr2(),
  this value points to the thunk (I do not know the correct English term)

 ppc64 calls it function descriptor (GDB
 ppc64_linux_convert_from_func_ptr_addr):
For PPC64, a function descriptor is a TOC entry,

Thanks Jan.

 in a data section,

Yes!

Now I can't understand how this test-case could ever work on ppc.
step-jump-cont does:

regs-nip = raise_sigusr2;  --- points to data section
ptrace(PTRACE_CONT);

of course, the tracee gets SIGSEGV, this section is not executable.

Oleg.

Re: powerpc: step-jump-cont failure (Was: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless)

2009-12-07 Thread Oleg Nesterov 
On 12/07, Oleg Nesterov wrote:

 On 12/07, Jan Kratochvil wrote:
 
  On Mon, 07 Dec 2009 15:24:51 +0100, Oleg Nesterov wrote:
   But. raise_sigusr2 is not equal to the actual address of 
   raise_sigusr2(),
   this value points to the thunk (I do not know the correct English term)
 
  ppc64 calls it function descriptor (GDB
  ppc64_linux_convert_from_func_ptr_addr):
 For PPC64, a function descriptor is a TOC entry,

 Thanks Jan.

  in a data section,

 Yes!

 Now I can't understand how this test-case could ever work on ppc.
 step-jump-cont does:

   regs-nip = raise_sigusr2;  --- points to data section
   ptrace(PTRACE_CONT);

 of course, the tracee gets SIGSEGV, this section is not executable.

Hmm. Looks like, powerpc means a lot of different hardware, and
_PAGE_EXEC may be 0. I didn't notice this when I quickly grepped
arch/powerpc/

IOW, perhaps on some machines r implies x ?

Is yes, this can explain why the results differ on different
machines.

Oleg.

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-06 Thread Oleg Nesterov 
On 12/05, caiq...@redhat.com wrote:

  Yes, it looks good so far. Ptrace tests also does not show any
  regression.

 I said this too early. Looks like step-jump-count started to fail now.

 step-jump-cont: step-jump-cont.c:244: main: Assertion `0' failed.
 /bin/sh: line 5: 28212 Aborted ${dir}$tst
 FAIL: step-jump-cont

Could you please confirm that if you revert this

[PATCH] utrace: don't set -ops = utrace_detached_ops lockless

change the kernel passes the test? This is just impossible that this
patch could make any difference for this test-case.

 You can login the same machine.

Yes, thanks.

Unfortunately, I can't extend the reservation.

# extendtesttime.sh
...
Extending reservation time 48
Unable to connect to server, sleeping 5 seconds...

and so on


I give up for today. Yes, the test-case fails. It gets SIGSEGV right
after we change the instruction pointer to raise_sigusr2.

I added printf(%p\n, raise_sigusr2) to print this address, and then
disassemled the code under this address:

(gdb) disassemble 0x100118c0
Dump of assembler code for function raise_sigusr2:
0x100118c0 raise_sigusr2+0:   .long 0x0    
SIGSEGV
0x100118c4 raise_sigusr2+4:   .long 0x1ab0
0x100118c8 raise_sigusr2+8:   .long 0x0
0x100118cc raise_sigusr2+12:  vmulosh v0,v1,v19
0x100118d0 main+0:.long 0x0
0x100118d4 main+4:vmhaddshs v0,v0,v1,v12
0x100118d8 main+8:.long 0x0
...

Hmm... this doesn't look like raise_sigusr2(), so I did

(gdb) disassemble raise_sigusr2
Dump of assembler code for function raise_sigusr2:
0x1ab0 raise_sigusr2+0:   mflrr0
0x1ab4 raise_sigusr2+4:   li  r3,12
0x1ab8 raise_sigusr2+8:   std r0,16(r1)
0x1abc raise_sigusr2+12:  stdur1,-112(r1)
0x1ac0 raise_sigusr2+16:  bl  0x1780
0x1ac4 raise_sigusr2+20:  ld  r2,40(r1)
0x1ac8 raise_sigusr2+24:  cmpdi   cr7,r3,0

looks like ppc uses some form of thunks for function pointers...

Another oddity, the comment says

/* PTRACE_GETREGS / PTRACE_SETREGS are not available on ppc.

#ifdef PTRACE_GETREGS

However, PTRACE_GETREGS is defined, and the testcase does use PTRACE_GETREGS.

Perhaps we have some issues with libc or compiler...

Did you change anything else (except kernel) on this machine?

Oleg.

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-06 Thread CAI Qian 

 Could you please confirm that if you revert this
 
   [PATCH] utrace: don't set -ops = utrace_detached_ops lockless
 
 change the kernel passes the test? This is just impossible that this
 patch could make any difference for this test-case.

No, it is the same. It is true also for kernels turned off CONFIG_UTRACE.

 Unfortunately, I can't extend the reservation.

RHTS had an outage yesterday, and now seems back to normal.

 Did you change anything else (except kernel) on this machine?

Not I am aware of. I will re-install this machine to have a probably clearer 
environment, and let you know the outcome shortly.

Thanks,
CAI Qian

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-05 Thread CAI Qian 

 I was going to try again, but noticed you already recompiled and
 booted the kernel. I see ./test is running and there is nothing bad
 in dmesg ;)

Yes, it looks good so far. Ptrace tests also does not show any regression. I 
kicked off a few tests on other platform, so hopefully have more information 
shortly.

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-05 Thread caiqian 

 Yes, it looks good so far. Ptrace tests also does not show any
 regression.

I said this too early. Looks like step-jump-count started to fail now.

step-jump-cont: step-jump-cont.c:244: main: Assertion `0' failed.
/bin/sh: line 5: 28212 Aborted ${dir}$tst
FAIL: step-jump-cont

You can login the same machine.

# cd /mnt/tests/kernel/misc/ptrace-testsuite/ptrace-tests/tests
# make check

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-05 Thread Oleg Nesterov 
On 12/05, caiq...@redhat.com wrote:

  Yes, it looks good so far. Ptrace tests also does not show any
  regression.

 I said this too early. Looks like step-jump-count started to fail now.

 step-jump-cont: step-jump-cont.c:244: main: Assertion `0' failed.
 /bin/sh: line 5: 28212 Aborted ${dir}$tst
 FAIL: step-jump-cont

Hmm. This is very, very strange. I don't know what this test-case
actuallly does, but I can't see how this patch can make any difference.

 You can login the same machine.

Yes, will do tomorrow.

Thanks.

Oleg.

[PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-04 Thread Oleg Nesterov 
On 12/04, Roland McGrath wrote:

  I think the problem is clear now.

 Ok.  We should probably move this discussion to utrace-devel.

Yes, I didn't notice we discuss this offlist...

  I forgot that there is another issue (iirc a bit discussed too).
  finish_callback_report() sets -ops = utrace_detached_ops lockless!

 You'll have to remind me why this is a problem.

Re: [PATCH 85] ptrace_attach_task: rely on utrace_barrier(), don't 
check -ops
https://www.redhat.com/archives/utrace-devel/2009-October/msg00180.html

We already discussed this, but forgot to finish.

Do you agree with the patch?

--
[PATCH] utrace: don't set -ops = utrace_detached_ops lockless

finish_callback_report() changes -ops lockless. Imho this is not
right in general, the state of !EXIT_DEAD tracee must be stable
under utrace-lock.

And this can confuse ptrace_reuse_engine()-utrace_barrier() logic.
utrace_barrier() can race with reporting loop and return 0 while
engine was already detached or in the middle of detach.

See also https://www.redhat.com/archives/utrace-devel/2009-October/msg00180.html

Signed-off-by: Oleg Nesterov o...@redhat.com
---

 kernel/utrace.c |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--- UTRACE-PTRACE/kernel/utrace.c~DONT_CHANGE_OPS_LOCKLESS  2009-11-24 
17:20:33.0 +0100
+++ UTRACE-PTRACE/kernel/utrace.c   2009-12-04 17:10:37.0 +0100
@@ -1390,11 +1390,15 @@ static inline void finish_callback_repor
  struct utrace_engine *engine,
  enum utrace_resume_action action)
 {
+   if (action == UTRACE_DETACH) {
+   spin_lock(utrace-lock);
+   engine-ops = utrace_detached_ops;
+   spin_unlock(utrace-lock);
+   }
/*
 * If utrace_control() was used, treat that like UTRACE_DETACH here.
 */
-   if (action == UTRACE_DETACH || engine-ops == utrace_detached_ops) {
-   engine-ops = utrace_detached_ops;
+   if (engine-ops == utrace_detached_ops) {
report-detaches = true;
return;
}

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-04 Thread Oleg Nesterov 
Forgot to mention, I did a lot of testing on ppc machine and the
patch helps, finally I was able to reproduce the problem.

But I failed to install the kernel on Cai's machine, perhaps he
could test the patch too ;)

fighting with rhts machines is very tiresome and time consuming...

I'll send the promised patch which kills supress_sigtrap() tomorrow,
it needs testing/checking.

On 12/04, Oleg Nesterov wrote:

 On 12/04, Roland McGrath wrote:
 
   I think the problem is clear now.
 
  Ok.  We should probably move this discussion to utrace-devel.
 
 Yes, I didn't notice we discuss this offlist...
 
   I forgot that there is another issue (iirc a bit discussed too).
   finish_callback_report() sets -ops = utrace_detached_ops lockless!
 
  You'll have to remind me why this is a problem.
 
   Re: [PATCH 85] ptrace_attach_task: rely on utrace_barrier(), don't 
 check -ops
   https://www.redhat.com/archives/utrace-devel/2009-October/msg00180.html
 
 We already discussed this, but forgot to finish.
 
 Do you agree with the patch?
 
 --
 [PATCH] utrace: don't set -ops = utrace_detached_ops lockless
 
 finish_callback_report() changes -ops lockless. Imho this is not
 right in general, the state of !EXIT_DEAD tracee must be stable
 under utrace-lock.
 
 And this can confuse ptrace_reuse_engine()-utrace_barrier() logic.
 utrace_barrier() can race with reporting loop and return 0 while
 engine was already detached or in the middle of detach.
 
 See also 
 https://www.redhat.com/archives/utrace-devel/2009-October/msg00180.html
 
 Signed-off-by: Oleg Nesterov o...@redhat.com
 ---
 
  kernel/utrace.c |8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)
 
 --- UTRACE-PTRACE/kernel/utrace.c~DONT_CHANGE_OPS_LOCKLESS2009-11-24 
 17:20:33.0 +0100
 +++ UTRACE-PTRACE/kernel/utrace.c 2009-12-04 17:10:37.0 +0100
 @@ -1390,11 +1390,15 @@ static inline void finish_callback_repor
 struct utrace_engine *engine,
 enum utrace_resume_action action)
  {
 + if (action == UTRACE_DETACH) {
 + spin_lock(utrace-lock);
 + engine-ops = utrace_detached_ops;
 + spin_unlock(utrace-lock);
 + }
   /*
* If utrace_control() was used, treat that like UTRACE_DETACH here.
*/
 - if (action == UTRACE_DETACH || engine-ops == utrace_detached_ops) {
 - engine-ops = utrace_detached_ops;
 + if (engine-ops == utrace_detached_ops) {
   report-detaches = true;
   return;
   }

Re: [PATCH] utrace: don't set -ops = utrace_detached_ops lockless

2009-12-04 Thread Roland McGrath 
   I forgot that there is another issue (iirc a bit discussed too).
   finish_callback_report() sets -ops = utrace_detached_ops lockless!
 
  You'll have to remind me why this is a problem.
 
   Re: [PATCH 85] ptrace_attach_task: rely on utrace_barrier(), don't 
 check -ops
   https://www.redhat.com/archives/utrace-devel/2009-October/msg00180.html
 
 We already discussed this, but forgot to finish.

Ah, yes.  I had that message still sitting in my folder to think about
again and reply.

 Do you agree with the patch?

I think so, yes.  It could use some more comments about the importance of
the lock.  I added a comment and merged it in.


Thanks,
Roland