[Mesa-dev] pipebuffer: Use pipe_condvar instead of ifdef'd sys_yield()

[José Fonseca]

I've looked and tested at your whole patch series.

As you said, condvars and barriers work now on windows albeit slowly;
which is a big improvement over not working at all as before.

However, after a more careful look to the pipebuffer change I realized
that it is counterproductive. The busy loop only happens when destroying
the context (i.e., typically once per application, often never if the
app invokes exit() before detroying the context). Your change optimizes
this unimportant case, at the expense of grabing a lock and using an
condition variables whenever the number of fences reaches zero, which is
much more often.

Note that during typical operation there is no busy loop -- pipebuffer
invokes the fence_finish() callback which should do something sensible
like waiting for an IRQ, or something like that.

Actually, even when destroying the fenced buffer manager the busy loop
never actually happens in practice. The lines

  while(fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
         ;

will destroy everything. The code looks as it is probably due to
historical reasons (in particular it's very tricky on windows to unload
the OpenGL driver DLL in some circumstances such as Ctrl+C and there was
a time where we tried to destroy the screen when contexts were still
active, but that has been fixed since). 

So the code 

   /* Wait on outstanding fences */
   while (fenced_mgr->num_fenced) {
      pipe_mutex_unlock(fenced_mgr->mutex);
#if defined(PIPE_OS_LINUX) || defined(PIPE_OS_BSD) ||
defined(PIPE_OS_SOLARIS)
      sched_yield();
#endif
      pipe_mutex_lock(fenced_mgr->mutex);
      while(fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
         ;
   }

could be replaced by 

   /* Wait on outstanding fences */
   if (fenced_mgr->num_fenced) {
      while(fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
         ;
   }

without any loss. (Unless there is an active thread, which means the
fenced_bufmgr_destroy() shouldn't be happening at all.)

So I'll apply your patch series, except the pipebuffer change.

Jose


On Tue, 2010-07-06 at 10:45 -0700, nobled wrote:
> Well, that's why I sent the "Implement pipe_condvar on win32" and "on
> Windows Vista" patches first. With the first patch, the pipebuffer
> code will do on Windows exactly what it already does right now on
> Unix-- unlock the mutex, wait for other threads, then try to lock it
> again. pipe_condvar on Windows (or any other platform) will *work*, it
> just won't be very efficient.
> 
> On Tue, Jul 6, 2010 at 6:46 AM, José Fonseca <jfonseca at vmware.com> wrote:
> > The patch looks good, but we don't support pipe_condvar on Windows yet.
> > I'd prefer to see the commit deferred until we do.
> >
> > Any takers to implement pipe_condvar on Windows? There's even a nice
> > tutorial on http://locklessinc.com/articles/pthreads_on_windows/
> >
> > Jose
> >
> > On Mon, 2010-07-05 at 09:53 -0700, nobled wrote:
> >> This way there's fewer ifdef's *and* less busy-waiting.
> >> ---
> >>  .../auxiliary/pipebuffer/pb_buffer_fenced.c        |   19 +++++++------------
> >>  1 files changed, 7 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> >> b/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> >> index d6cf640..db3617e 100644
> >> --- a/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> >> +++ b/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> >> @@ -34,13 +34,6 @@
> >>   */
> >>
> >>
> >> -#include "pipe/p_config.h"
> >> -
> >> -#if defined(PIPE_OS_LINUX) || defined(PIPE_OS_BSD) || defined(PIPE_OS_SOLARIS)
> >> -#include <unistd.h>
> >> -#include <sched.h>
> >> -#endif
> >> -
> >>  #include "pipe/p_compiler.h"
> >>  #include "pipe/p_defines.h"
> >>  #include "util/u_debug.h"
> >> @@ -81,6 +74,7 @@ struct fenced_manager
> >>      * Following members are mutable and protected by this mutex.
> >>      */
> >>     pipe_mutex mutex;
> >> +   pipe_condvar zero_fenced;
> >>
> >>     /**
> >>      * Fenced buffer list.
> >> @@ -307,6 +301,8 @@
> >>     LIST_DEL(&fenced_buf->head);
> >>     assert(fenced_mgr->num_fenced);
> >>     --fenced_mgr->num_fenced;
> >> +   if (fenced_mgr->num_fenced == 0)
> >> +      pipe_condvar_broadcast(fenced_mgr->zero_fenced);
> >>
> >>     LIST_ADDTAIL(&fenced_buf->head, &fenced_mgr->unfenced);
> >>     ++fenced_mgr->num_unfenced;
> >> @@ -1008,13 +1004,10 @@ fenced_bufmgr_destroy(struct pb_manager *mgr)
> >>
> >>     /* Wait on outstanding fences */
> >>     while (fenced_mgr->num_fenced) {
> >> -      pipe_mutex_unlock(fenced_mgr->mutex);
> >> -#if defined(PIPE_OS_LINUX) || defined(PIPE_OS_BSD) || defined(PIPE_OS_SOLARIS)
> >> -      sched_yield();
> >> -#endif
> >> -      pipe_mutex_lock(fenced_mgr->mutex);
> >>        while(fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
> >>           ;
> >> +      if (fenced_mgr->num_fenced)
> >> +         pipe_condvar_wait(fenced_mgr->zero_fenced, fenced_mgr->mutex);
> >>     }
> >>
> >>  #ifdef DEBUG
> >> @@ -1023,6 +1016,7 @@ fenced_bufmgr_destroy(struct pb_manager *mgr)
> >>
> >>     pipe_mutex_unlock(fenced_mgr->mutex);
> >>     pipe_mutex_destroy(fenced_mgr->mutex);
> >> +   pipe_condvar_destroy(fenced_mgr->zero_fenced);
> >>
> >>     if(fenced_mgr->provider)
> >>        fenced_mgr->provider->destroy(fenced_mgr->provider);
> >> @@ -1063,6 +1057,7 @@ fenced_bufmgr_create(struct pb_manager *provider,
> >>     LIST_INITHEAD(&fenced_mgr->unfenced);
> >>     fenced_mgr->num_unfenced = 0;
> >>
> >> +   pipe_condvar_init(fenced_mgr->zero_fenced);
> >>     pipe_mutex_init(fenced_mgr->mutex);
> >>
> >>     return &fenced_mgr->base;
> >
> >