* [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()
@ 2015-01-25 11:03 Chen Gang S
2015-01-25 12:41 ` Peter Maydell
2015-01-27 16:11 ` Michael Tokarev
0 siblings, 2 replies; 4+ messages in thread
From: Chen Gang S @ 2015-01-25 11:03 UTC (permalink / raw)
To: riku.voipio, david.gilbert; +Cc: QEMU Trivial, qemu-devel
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 <gang.chen.5i5j@gmail.com>
---
linux-user/main.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/linux-user/main.c b/linux-user/main.c
index 8c70be4..2d52c1f 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -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. */
--
1.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()
2015-01-25 11:03 [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper() Chen Gang S
@ 2015-01-25 12:41 ` Peter Maydell
2015-01-27 16:11 ` Michael Tokarev
1 sibling, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2015-01-25 12:41 UTC (permalink / raw)
To: Chen Gang S; +Cc: QEMU Trivial, Riku Voipio, David Gilbert, qemu-devel
On 25 January 2015 at 11:03, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
> 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 <gang.chen.5i5j@gmail.com>
> ---
> linux-user/main.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 8c70be4..2d52c1f 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -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 <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()
2015-01-25 11:03 [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper() Chen Gang S
2015-01-25 12:41 ` Peter Maydell
@ 2015-01-27 16:11 ` Michael Tokarev
2015-01-28 5:42 ` Chen Gang S
1 sibling, 1 reply; 4+ messages in thread
From: Michael Tokarev @ 2015-01-27 16:11 UTC (permalink / raw)
To: Chen Gang S, riku.voipio, david.gilbert, Peter Maydell
Cc: QEMU Trivial, qemu-devel
25.01.2015 14:03, Chen Gang S wrote:
> 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".
> 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()".
> 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)".
> If could return from queue_signal(), stop_all_task() would not
> be called in time,
"if queue_signal() returns
> 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.
> 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".
> 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
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> linux-user/main.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 8c70be4..2d52c1f 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -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. */
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()
2015-01-27 16:11 ` Michael Tokarev
@ 2015-01-28 5:42 ` Chen Gang S
0 siblings, 0 replies; 4+ messages in thread
From: Chen Gang S @ 2015-01-28 5:42 UTC (permalink / raw)
To: Michael Tokarev, riku.voipio, david.gilbert, Peter Maydell
Cc: QEMU Trivial, qemu-devel
On 1/28/15 00:11, Michael Tokarev wrote:
> 25.01.2015 14:03, Chen Gang S wrote:
>> 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".
>
>> 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()".
>
>> 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)".
>
>> If could return from queue_signal(), stop_all_task() would not
>> be called in time,
>
> "if queue_signal() returns
>
>> the next end_exclusive() would be issue.
>
> "would be AN issue".
>
OK, thanks, I shall notice about them, next time.
> 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.
> 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.
>> 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.
>> 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?
>
For me, I still suggest to give some descriptions for queue_signal().
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-01-28 5:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-25 11:03 [Qemu-devel] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper() Chen Gang S
2015-01-25 12:41 ` Peter Maydell
2015-01-27 16:11 ` Michael Tokarev
2015-01-28 5:42 ` Chen Gang S
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.