[vdr] Re: vdr-1.3.17: thread issues with vdr-xine andvdr-osdteletext

[Rainer Zocholl <UseNet-Posting-Nospam-74308-@zocki.toppoint.de>]

Klaus.Schmidinger@cadsoft.de(Klaus Schmidinger)  19.12.04 11:56


>Reinhard Nissl wrote:
>> [ ... fixing thread handling ... ]

>Somehow this is getting a little too complicated for my taste.
>There's way too much locking and condvar stuff being introduced.

>I suggest the attached patch against thread.[hc] of VDR 1.3.17.
>The main changes here are that the child thread no longer
>accesses childTid at all (it calls pthread_self() directly),

>and the running state is stored in a simple boolean variable,
>which requires no extensive locking since it is read/written
>atomically. 

You can't "read/write" anything "atomically" 
with simple C-Code in a mutlitthreaded environment.
There are CPU specific assembly commands required like
"lock: CMPXCHG AX,memptr" which are really "atomic",
but gcc does not allow direct access AFAIK (see atomic_store_rel etc.)
Too it would be reinventing the wheel, as the thread library
already offers all required stuff, tested in deep.

Some examples of real "atomic operations" you can find for example
here
http://lists.ximian.com/archives/public/mono-patches/2004-March/032172.html

Something like:

__ASM_GLOBAL_FUNC(interlocked_cmpxchg,
                  "movl 12(%esp),%eax\n\t"
                  "movl 8(%esp),%ecx\n\t"
                  "movl 4(%esp),%edx\n\t"
                  "lock; cmpxchgl %ecx,(%edx)\n\t"
                  "ret");


is "atomic", but:

If you do an

if (running)
  running=TRUE

*nothing* will inhibit a taskswitch in the virtual "line" 
between the "if" and the assignment.
So the second thread thread see too: "running not set"
and you have the (false) reentrance you wanted to avoid.
So the first task will overwrite the second
tasks values when she's back on CPU...
That might work, or not. In this case the thread
might be started twice...

But: Why bothering with the "running" flag at all if you 
accept that it might sometimes do not work or
assume(!) that the function is never ever called from different threads?


Depending how robust the remaining function is written
that code might work and will "only" fail very rarely.
The fails are not reproducible but annoying and
avoidable and only because the code should look easier???


An

if (running++)

is too absolutely "no atomic operation" tho it might look so.


Programming multithreaded *is* complicate.
That's why it's avoided so often or fails.

There is not sense in trying to "optimize" "shortcut" 
the locking calls.


It's "thread lottery" not protecting *every*
"critical/atomic resource accesses" by semaphores or mutex etc..

It may work today flawlessly. But tomorrow there is a new
plugin or a faster CPU and it may fail.
BTST, beleave me.
It is not worth the time you saved in not using a mutex.
It's begging for long term trouble.


>parentTid has been removed, since it was never actually used.


>Please take a look at this and let me know if you can find a
>possible loophole there.

That does not solve the other reentrancy problems too.
"But here it works" is no anwser in an multithreaded environment 
with more than one installation and more than one programmer
(who will act as semaphore...until he forgot the details...).

BTW:
Maybe you should describe more what the functions are expected to
to do and what is assuemd arround and why it is done the way it is done.
That would be of great help for all who are trying to help
and would help to avoid to keep "cargo cult code" ;-)