[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.

Frediano