linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()

PATCH In review Chen Gang S 3 participants 4 messages Jan 25, 2015 – Jan 28, 2015

Share & tools

Chen Gang S

author

2015-01-25 10:56 UTC #0 lore raw

PATCH v2 1/1 linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()

start/end_exclusive() need be pairs, except the start_exclusive() in stop_all_tasks() which is only used by force_sig(), which will be abort. So at present, start_exclusive() in stop_all_task() need not be paired.

queue_signal() may call force_sig(), or return after kill pid (or queue signal). If could return from queue_signal(), stop_all_task() would not be called in time, the next end_exclusive() would be issue.

So in arm_kernel_cmpxchg64_helper() for ARM, need remove end_exclusive() after queue_signal(). The related commit: "97cc756 linux-user: Implement new ARM 64 bit cmpxchg kernel helper".

Signed-off-by Chen Gang <[email protected]>

---
 linux-user/main.c | 2 --
 1 file changed, 2 deletions(-)

linux-user/main.c +0 -2

index 8c70be4..2d52c1f 100644
@@ -523,8 +523,6 @@ segv:
     info.si_code = TARGET_SEGV_MAPERR;
     info._sifields._sigfault._addr = env->exception.vaddress;
     queue_signal(env, info.si_signo, &info);
-
-    end_exclusive();
 }
 
 /* Handle a jump to the kernel code page.  */

Signature · 1 line

1.9.3

Peter Maydell

Reviewed-by on v2 1/1

2015-01-25 12:41 UTC #1 lore raw

Quoted 23 lines · Chen Gang S

start/end_exclusive() need be pairs, except the start_exclusive() in stop_all_tasks() which is only used by force_sig(), which will be abort. So at present, start_exclusive() in stop_all_task() need not be paired.
queue_signal() may call force_sig(), or return after kill pid (or queue signal). If could return from queue_signal(), stop_all_task() would not be called in time, the next end_exclusive() would be issue.
So in arm_kernel_cmpxchg64_helper() for ARM, need remove end_exclusive() after queue_signal(). The related commit: "97cc756 linux-user: Implement new ARM 64 bit cmpxchg kernel helper".
Signed-off-by Chen Gang <[email protected]>
---
 linux-user/main.c | 2 --
 1 file changed, 2 deletions(-)
linux-user/main.c +0 -0
index 8c70be4..2d52c1f 100644

linux-user/main.c segv: · line 528

Quoted 8 lines

linux-user/main.c +0 -2

@@ -523,8 +523,6 @@ segv:
     info.si_code = TARGET_SEGV_MAPERR;
     info._sifields._sigfault._addr = env->exception.vaddress;
     queue_signal(env, info.si_signo, &info);
-
-    end_exclusive();
 }
 
 /* Handle a jump to the kernel code page.  */

Reviewed-by Peter Maydell <[email protected]>

thanks
-- PMM

Michael Tokarev

2015-01-27 16:12 UTC #2 lore raw

In reply to Chen Gang S

Quoted 1 line · Chen Gang S

start/end_exclusive() need be pairs, except the start_exclusive() in

"need TO be pairs", or "should be pairs" or "should be called in pairs".

Quoted 1 line

stop_all_tasks() which is only used by force_sig(), which will be abort.

"which will abort" or "which will call abort()" or "which calls abort()".

Quoted 4 lines

So at present, start_exclusive() in stop_all_task() need not be paired.
queue_signal() may call force_sig(), or return after kill pid (or queue signal).

"or return after killing pid (or queuing signal)".

Quoted 2 lines

If could return from queue_signal(), stop_all_task() would not
be called in time,

"if queue_signal() returns

Quoted 1 line

the next end_exclusive() would be issue.

"would be AN issue".

But actually we're interested to know answer to a slightly different
question: whenever queue_signal() returns or not (it doesn't return in force_sig case). So whole this part becomes something like:

queue_signal() may either call force_sig() and die, or return. In the latter case, stop_all_task() would not be called in time, so next end_exclusive() will be an issue.

And even more, when you look at this function (arm_kernel_cmpxchg64_helper), you'll notice it has two calls to end_exclusive() in sigsegv case, without a call to start_exclusive(). _That_ is, I think, the key point here -- the rest of the information here is not really very relevant, because the actual problem is this double call to end_exclusive() which should be removed. It is not really that interesting to know that it's not _necessary_ to call end_exclusive() in some cases which leads to abort(), because this is not one of them anyway (since queue_signal() might return just fine), and because while it is not necessary, it is not an error either. With all this extra info, thie commit message becomes just too confusing.

Quoted 2 lines

So in arm_kernel_cmpxchg64_helper() for ARM, need remove end_exclusive() after queue_signal().

"need TO remove", and again the missing subject. "We need to remove", or "we should remove", or, yet another variant, "extra end_exclusive() call should be removed".

Quoted 2 lines

The related commit: "97cc756 linux-user: Implement
new ARM 64 bit cmpxchg kernel helper".

So, how about this (the subject is fine):

start/end_exclusive() should be paired to each other. However, in arm_kernel_cmpxchg64_helper() function, end_exclusive() is called twice in a row. Remove the second, redundrand, call.

Commit which introduced this problem is"97cc756 linux-user: Implement new ARM 64 bit cmpxchg kernel helper".

Did I understand the problem correctly?

Thanks,

/mjt

Quoted 19 lines

Signed-off-by Chen Gang <[email protected]>

---
 linux-user/main.c | 2 --
 1 file changed, 2 deletions(-)

linux-user/main.c +0 -2

index 8c70be4..2d52c1f 100644
@@ -523,8 +523,6 @@ segv:
     info.si_code = TARGET_SEGV_MAPERR;
     info._sifields._sigfault._addr = env->exception.vaddress;
     queue_signal(env, info.si_signo, &info);
-
-    end_exclusive();
 }
 
 /* Handle a jump to the kernel code page.  */

Chen Gang S

author

2015-01-28 05:34 UTC #3 lore raw

Quoted 16 lines · Michael Tokarev

Quoted 1 line · Chen Gang S
start/end_exclusive() need be pairs, except the start_exclusive() in
"need TO be pairs", or "should be pairs" or "should be called in pairs".
Quoted 1 line
stop_all_tasks() which is only used by force_sig(), which will be abort.
"which will abort" or "which will call abort()" or "which calls abort()".
Quoted 4 lines
So at present, start_exclusive() in stop_all_task() need not be paired.
queue_signal() may call force_sig(), or return after kill pid (or queue signal).
"or return after killing pid (or queuing signal)".

Quoted 9 lines

Quoted 2 lines
If could return from queue_signal(), stop_all_task() would not
be called in time,
"if queue_signal() returns
Quoted 1 line
the next end_exclusive() would be issue.
"would be AN issue".

OK, thanks, I shall notice about them, next time.

Quoted 8 lines

But actually we're interested to know answer to a slightly different
question: whenever queue_signal() returns or not (it doesn't return in force_sig case). So whole this part becomes something like:
queue_signal() may either call force_sig() and die, or return. In the latter case, stop_all_task() would not be called in time, so next end_exclusive() will be an issue.

OK, it sounds good to me.

Quoted 12 lines

And even more, when you look at this function (arm_kernel_cmpxchg64_helper), you'll notice it has two calls to end_exclusive() in sigsegv case, without a call to start_exclusive(). _That_ is, I think, the key point here -- the rest of the information here is not really very relevant, because the actual problem is this double call to end_exclusive() which should be removed. It is not really that interesting to know that it's not _necessary_ to call end_exclusive() in some cases which leads to abort(), because this is not one of them anyway (since queue_signal() might return just fine), and because while it is not necessary, it is not an error either. With all this extra info, thie commit message becomes just too confusing.

For me, when process paired functions, need consider a little more.

- Are there any recurse code between lock/unlock?

- After lock, do any code call unlock indirectly? Or before unlock(), do any code call lock() indirectly?

- Between 2 unlocks (or 2 locks), does any code call lock (or unlock) indirectly?

In our case, queue_signal() may call lock indirectly between 2 unlocks, So for me, the patch is necessary to mention about queue_signal() in commit comments.

Quoted 7 lines

Quoted 2 lines
So in arm_kernel_cmpxchg64_helper() for ARM, need remove end_exclusive() after queue_signal().
"need TO remove", and again the missing subject. "We need to remove", or "we should remove", or, yet another variant, "extra end_exclusive() call should be removed".

OK.

Quoted 10 lines

Quoted 2 lines
The related commit: "97cc756 linux-user: Implement
new ARM 64 bit cmpxchg kernel helper".
So, how about this (the subject is fine):
start/end_exclusive() should be paired to each other. However, in arm_kernel_cmpxchg64_helper() function, end_exclusive() is called twice in a row. Remove the second, redundrand, call.

Quoted 7 lines

Commit which introduced this problem is"97cc756 linux-user: Implement new ARM 64 bit cmpxchg kernel helper".
?
Did I understand the problem correctly?

For me, I still suggest to give some descriptions for queue_signal().

Thanks.

Signature · 3 lines

Chen Gang

Open, share, and attitude like air, water, and life which God blessed