[MPlayer-dev-eng] "lockless" cache2 looks buggy

[Ivan Kalvachev]

2006/8/25, Denis Vlasenko <vda.linux at googlemail.com>:
> Hello mplayer coders,
>
> I was looking into cache2.c. It says that it is a lockless
> cache implementation.
>
> I see s few issues with the code. First issue is the most serious,
> since it looks like real bug.
>
> (1) Non-atomic updated of metadata.
>
> Main thread reads cached data using cache_read(), which uses
> off_t variables s->min_filepos, s->max_filepos etc.
> They are 64-bit variables on most systems:
>
>     newb=s->max_filepos-s->read_filepos; // new bytes in the buffer
>     pos=s->read_filepos - s->offset;
>     if(pos<0) pos+=s->buffer_size; else
>     if(pos>=s->buffer_size) pos-=s->buffer_size;
>     if(newb>s->buffer_size-pos) newb=s->buffer_size-pos; // handle wrap...
>     if(newb>size) newb=size;
>     memcpy(buf,&s->buffer[pos],newb);
>
> However, caching thread reads new data into circular buffer
> and updates those same variables in cache_fill():
>
>   back2=s->buffer_size-(space+newb); // max back size
>   if(s->min_filepos<(read-back2)) s->min_filepos=read-back2;
>   len=stream_read(s->stream,&s->buffer[pos],space);
>   if(!len) s->eof=1;
>   s->max_filepos+=len;
>   if(pos+len>=s->buffer_size){
>       // wrap...
>       s->offset+=s->buffer_size;
>   }
>
> These updates are not atomic on 32-bit architectures (i386).
> Therefore caching thread can update lower half of a value, then it
> may get rescheduled with CPU given to main thread, which then
> will use bogus value of half-updated variable.
>
> If you don't believe me, add asm() statements as shown below:
>
> asm("#1");
>   s->max_filepos+=len;
> asm("#1");
>
> The resulting code is:
>
> #APP
>         #1
> #NO_APP
>         movl    128(%esp), %esi
>         movl    %ecx, %eax
>         cltd
>         addl    %ecx, 36(%esi)
>         adcl    %edx, 40(%esi)
> #APP
>         #1
> #NO_APP
>
>
> If tast will be resheduled in between of two add instructions
> and it will happen when you cross 32-bit border - BOOM.
>
>
> (2) fork() failure is not checked
>
> if((stream->cache_pid=fork())){
>
> The above if() thinks that non-zero return means that we are the parent.
> However, -1 is possible too (fork() may fail).
>
> (3) Suboptimal underrun handling.
>
> Cache underrun handling looks suboptimal to me:
>
>   while(size>0){
>     int pos,newb,len;
>
>     if(s->read_filepos>=s->max_filepos || s->read_filepos<s->min_filepos){
>         // eof?
>         if(s->eof) break;
>         // waiting for buffer fill...
>         usec_sleep(READ_USLEEP_TIME); // 10ms
>         continue; // try again...
>     }
>   ...
>   }
>
> If you have an underrun, it is not that good an idea to sleep.
> Isn't it better to just schedule (man sched_yield), giving
> cache thread a chance to do its work? In the above code,
> you can easily add 10 ms more latency for the already bad case
> of cache underrun.
>
> (4) Jumbledtogethercoditionalexpressions
>
> while(s->read_filepos<s->min_filepos || s->max_filepos-s->read_filepos<min){
>
> This code has readability problems. Do you hate whitespace? ;)
>
>
> I am willing to work on this code if it is acceptable to start
> with reformatting cache2.c into more readable form first.
> Would such patch be accepted? To be more precise, what are
> style guidelines to mplayer code?

There are no style guidelines to mplayer code and everybody are free
to use their style.
This of course leads to the mess you see and to endless flamewars
about indent options....

I think the original author of this code is the mplayer project founder Arpi.
He had left the project few years ago. He is great coder, but he
believed that if you need to format code in order to understand it,
then you are not real programmer. This and the total forbid of
cosmetic changes (whitespace) lead to the most "beautiful" code in the
know history of FALOSS (Free And Libre Open Source
Software)..</joking>

Now, if you want to format nicely the code you need to send it as
separate patch. It would be good if after the changes patch -l still
is able to apply patches there. (Not that there are any pending
patches in cache, AFAIK).

There is no problem if you want to work on this code.

However I would be more happy of a complete rewrite that instead of
one big circular buffer uses many chunks. The problem that this is
supposed to solve is, when non-interleaved avi is played that have
audio and video on the both ends of the file, meaning that cache
flushes itself constantly.

Well... wish you lick.

3)

As for know, about sched_yied()
DESCRIPTION
....
  POSIX  systems on which sched_yield() is available define _POSIX_PRIOR-
       ITY_SCHEDULING in <unistd.h>.
...
CONFORMING TO
       POSIX.1-2001.

I guess you are gonna use that define for sure and fallback to the
current method if not defined ;)

2) send patch

1) send patch