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

[Chen Gang S]

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