Re: [Qemu-trivial] [PATCH v2] linux-user/main.c: Remove redundant end_exclusive() in arm_kernel_cmpxchg64_helper()

[Michael Tokarev]

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 <address@hidden>
> ---
>  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.  */
>