Re: [Qemu-devel] Re: [PATCH 3/3] vnc: threaded VNC server

[Paolo Bonzini]

> >> +void vnc_job_push(VncJob *job)
> >> +{
> >> +    vnc_lock_queue(queue);
> >> +    if (QLIST_EMPTY(&job->rectangles)) {
> >> +        qemu_free(job);
> >
> > No need to lock if you get into the "then" block.
> 
> I locked it because the main thread can try to push a job while a
> consumer is removing one, so I can't call QLIST_EMPTY() without
> locking the queue.

You're obviously right.

>>> +    qemu_mutex_unlock(&job->vs->output_mutex);
>>> +
>>> +    if (job->vs->csock != -1 && job->vs->abording != true) {
>>> +        vnc_flush(job->vs);
>>> +    }
>>> +
>>
>> You're accessing the abort flag outside the mutex here.  Also, you are not
>> using vnc_{,un}lock_output.
>
> I assumed that bool (int) where atomic .. but you're right I should lock that.

They are, however: 1) if you access them outside a mutex you have to think about
whether you need memory barriers and whether there are races; 2) since you 
already
own the mutex and you're just keeping it a bit longer, it costs basically 
nothing.
BTW, with the same reasoning you could avoid taking the mutex altogether in
vnc_abort_display_jobs (but I think it's better to keep it).

Also, I took a look at the code again and I noticed this:

>>> +        if (job->vs->csock == -1) {
>>> +            goto disconnected;
>>> +        }

The "goto" is jumping over the unlocking of &job->vs->mutex.  You only want
a "break;" I think.

>>  static void vnc_disconnect_finish(VncState *vs)
>>  {
>> +    vnc_jobs_join(vs); /* Wait encoding jobs */
>> +    vnc_lock(vs);
>
> Possibly racy?  Maybe you have to set the aforementioned new flag
> queue->exit at the beginning of vnc_jobs_join, and refuse new jobs if it is
> set.
>
> vnc_disconnect_finish can only be called by the main thread, I don't
> see how this could be racy, any hint ?

I was thinking of someone queuing jobs between the end of vnc_jobs_join
and the time the vnc_lock is taken.  But indeed jobs cannot be queued
at this time because vnc_refresh can only be called from the same
thread.  So this is correct.

>> Also, if anything waits on the same vs in vnc_refresh while you own it in
>> vnc_disconnect_finish, as soon as you unlock they'll have a dangling
>> pointer.  (After you unlock the mutex the OS wakes the thread, but then
>> pthread_mutex_lock has to check again that no one got the lock in the
>> meanwhile; so QTAILQ_FOREACH_SAFE is not protecting you).  Probably it's
>> better to use a single lock on vd->clients instead of one lock per VncState.

Same here.  No race because everything happens in the main thread.

Paolo