On 01/21/2015, 05:40 PM, Paolo Bonzini wrote:
>
>
> On 21/01/2015 17:28, Jiri Slaby wrote:
>> + if (atomic_fetch_or(&edu->status, EDU_STATUS_COMPUTING) &
>> EDU_STATUS_COMPUTING) {
>
> Theoretically the other thread could see EDU_STATUS_COMPUTING here and
> not enter the condvar wait. So...
>
>> + break;
>> + }
>> + qemu_mutex_lock(&edu->thr_mutex);
>> + edu->fact = val;
>> + qemu_cond_signal(&edu->thr_cond);
>> + qemu_mutex_unlock(&edu->thr_mutex);
>
> ... just one change:
>
> if (atomic_read(&edu->status) & EDU_STATUS_COMPUTING) {
> break;
> }
> /* EDU_STATUS_COMPUTING cannot go 0->1 concurrently, because
> * it is only set in this function and it is under the iothread
> * mutex.
> */
> qemu_mutex_lock(&edu->thr_mutex);
> edu->fact = val;
> atomic_or(&edu->status), EDU_STATUS_COMPUTING);
> qemu_cond_signal(&edu->thr_cond);
> qemu_mutex_unlock(&edu->thr_mutex);
>
> If you are okay with this change, I'll apply the patch to my tree.
Oh, I see. I will send v5 shortly as I found a bug in documentation too.
Thanks a lot for the review.
--
js
suse labs
