[Spice-devel] [PATCH 3/6] worker: make it clear it returns from process when no cmd

[Frediano Ziglio]

> 
> > 
> > On Tue, 2015-09-29 at 11:56 +0100, Frediano Ziglio wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > 
> > > ---
> > >  server/red_worker.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 2efb311..d8a081e 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -4966,12 +4966,12 @@ static int red_process_cursor(RedWorker *worker,
> > > uint32_t max_pipe_size, int *ri
> > >              if (worker->repoll_cursor_ring < CMD_RING_POLL_RETRIES) {
> > >                  worker->repoll_cursor_ring++;
> > >                  worker->event_timeout = MIN(worker->event_timeout,
> > >                  CMD_RING_POLL_TIMEOUT);
> > > -                break;
> > > +                return n;
> > >              }
> > >              if (worker->repoll_cursor_ring > CMD_RING_POLL_RETRIES ||
> > >                  worker->qxl->st->qif->req_cursor_notification(worker->qxl))
> > >                  {
> > >                  worker->repoll_cursor_ring++;
> > > -                break;
> > > +                return n;
> > >              }
> > >              continue;
> > >          }
> > > @@ -5028,12 +5028,12 @@ static int red_process_commands(RedWorker
> > > *worker,
> > > uint32_t max_pipe_size, int *
> > >              if (worker->repoll_cmd_ring < CMD_RING_POLL_RETRIES) {
> > >                  worker->repoll_cmd_ring++;
> > >                  worker->event_timeout = MIN(worker->event_timeout,
> > >                  CMD_RING_POLL_TIMEOUT);
> > > -                break;
> > > +                return n;
> > >              }
> > >              if (worker->repoll_cmd_ring > CMD_RING_POLL_RETRIES ||
> > >                           worker->qxl->st->qif->req_cmd_notification(worker->qxl))
> > >                           {
> > >                  worker->repoll_cmd_ring++;
> > > -                break;
> > > +                return n;
> > >              }
> > >              continue;
> > >          }
> > 
> > 
> > This change looks fine to me, but while looking at the code, I noticed
> > something unrelated. It seems that e.g.
> > worker->qxl->st->qif->req_cursor_notification() will never be called.
> > worker->repoll_cursor_ring is incremented if it is less than
> > CMD_RING_POLL_RETRIES and is incremented if it's greater than
> > CMD_RING_POLL_RETRIES, but it is not incremented if it is equal. Which
> > means that once it reaches CMD_RING_POLL_RETRIES, it will stay there and
> > the second if statement will never be satisfied. The same appears to be
> > true in the second hunk.
> > 
> > Jonathon
> > 
> 
> It's called when worker->repoll_cmd_ring == CMD_RING_POLL_RETRIES, in this
> case the counter
> will be incremented or not based on
> worker->qxl->st->qif->req_cursor_notification result.
> 
> Yes, the code is not really clear.
> 
> I also don't like these kind of polling, but this is another issue.
> The current pollings are 200 each 10 milliseconds which means it continue
> polling if no
> activities for 2 seconds. The guest should fill the buffer then signal the
> card, if there
> are no commands there should be no polling. Looks like this code is here to
> handle some
> buggy drivers which do not signal the card correctly.
> 

Tried to understand the history of this code however the first commit is:

commit c1b79eb035fa158fb2ac3bc8e559809611070016
Author: Yaniv Kamay <ykamay at redhat.com>
Date:   Sat Sep 19 21:25:46 2009 +0300

    fresh start

so the former history is not in this repository. Does somebody know where
to find the former repository?

Frediano