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

[Denis Vlasenko]

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?

Best regards,
--
vda