[vlc-devel] [PATCH] android: threads support

[Rémi Denis-Courmont]

On Thu, 04 Oct 2012 23:22:09 +0200, Rafaël Carré <funman at videolan.org>
wrote:
>>> +/**
>>> + * Initializes a semaphore.
>>> + */
>>> +void vlc_sem_init (vlc_sem_t *sem, unsigned value)
>> 
>> This is not going to interoperate with cancellation.
> 
> What do you mean?
> 
> Some of those are mandated to be cancellation points?

sem_wait() and pthread_rwlock_(rd|wr)lock().


>>> +void vlc_cancel (vlc_thread_t thread_id)
>>> +{
>>> +    bool self = thread_id == pthread_getspecific(thread_key);
>>> +
>>> +    thread_id->killed = true;
>>> +    if (!thread_id->killable)
>>> +        return;
>>> +
>>> +    vlc_mutex_t *lock = thread_id->lock;
>> 
>> I fail to grasp where the memory barriers are.
> 
> Hm i am not sure how barriers work.
> 
> On android ldr/str is one instruction so it should be atomic?

I very much doubt that LDR and STR are atomic on ARM SMP.

Also, nothing warrants that the compiler will emit a single load
instruction.

>>> +    if (lock) {
>>> +        if (!self)
>>> +            vlc_mutex_lock(lock);
>> 
>> I don't see what warrants that the mutex still exists at this point.
> 
> vlc_join(thread_id) has not been called yet.

So the thread handle exists. But my question concerns the mutex.

Also, I do not think self can be true if lock is non-NULL.

>>> +        if (thread_id->cond)
>>> +            pthread_cond_broadcast(thread_id->cond);
>>> +        if (!self)
>>> +            vlc_mutex_unlock(lock);
>>> +    }
>>> +
>>> +    if (self)
>>> +        vlc_testcancel();
>> 
>> No, vlc_cancel() is not a cancellation point.
> 
> In this case, the thread is cancelling itself (can that happen?).

vlc_cancel() is not a cancellation point in any case.

>>> +void vlc_testcancel (void)
>>> +{
>>> +    vlc_thread_t thread = pthread_getspecific(thread_key);
>>> +    if (!thread) /* not created by VLC, can't be cancelled */
>>> +        return;
>>> +    if (!thread->killable || !thread->killed)
>>> +        return;
>>> +
>>> +    for (vlc_cleanup_t *p = thread->cleaners; p != NULL; p = p->next)
>>> +        p->proc (p->data);
>> 
>> Why don't you just call pthread_exit() and let bionic run the handlers?
> 
> in vlc_thread.h, use of pthread cleanup is under
> #if defined (LIBVLC_USE_PTHREAD_CANCEL)
> 
> I guess it should be LIBVLC_USE_PTHREAD and we can remove the above
> code, I'll try this way.

You probably should have a separate block for Android anyway, semaphores
and read-write locks in addition.

-- 
Rémi Denis-Courmont
Sent from my collocated server