[Spice-devel] [PATCH 6/7] worker: move red_record_event

[Frediano Ziglio]

> I think that perhaps this commit could be combined with a previous one.
> Another comment below
> 

I think so, there is no reason to have patch and refactoring on the same patch series.

> On Thu, 2015-08-13 at 16:25 +0100, Frediano Ziglio wrote:
> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > 
> > ---
> >  server/red_record_qxl.c | 12 ++++++++++++
> >  server/red_record_qxl.h |  2 ++
> >  server/red_worker.c     | 24 ++++--------------------
> >  3 files changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/server/red_record_qxl.c b/server/red_record_qxl.c
> > index 481eb16..a72a200 100644
> > --- a/server/red_record_qxl.c
> > +++ b/server/red_record_qxl.c
> > @@ -792,3 +792,15 @@ void red_record_dev_input_primary_surface_create(FILE
> > *fd,
> >      write_binary(fd, "data", line_0 ? abs(surface->stride)*surface->height
> >      : 0,
> >          line_0);
> >  }
> > +
> > +void red_record_event(FILE *fd, int what, uint32_t type, unsigned long ts)
> > +{
> > +    static int counter = 0;
> > +
> > +    // TODO: record the size of the packet in the header. This would make
> > +    // navigating it much faster (well, I can add an index while I'm at
> > it..)
> > +    // and make it trivial to get a histogram from a file.
> > +    // But to implement that I would need some temporary buffer for each
> > event.
> > +    // (that can be up to VGA_FRAMEBUFFER large)
> > +    fprintf(fd, "event %d %d %u %lu\n", counter++, what, type, ts);
> > +}
> > diff --git a/server/red_record_qxl.h b/server/red_record_qxl.h
> > index 1be9e41..f7dcdc0 100644
> > --- a/server/red_record_qxl.h
> > +++ b/server/red_record_qxl.h
> > @@ -41,4 +41,6 @@ void red_record_cursor_cmd(FILE *fd, RedMemSlotInfo
> > *slots, int group_id,
> >  void red_record_dev_input_primary_surface_create(
> >                             FILE *fd, QXLDevSurfaceCreate *surface, uint8_t
> >                             *line_0);
> >  
> > +void red_record_event(FILE *fd, int what, uint32_t type, unsigned long
> > ts);
> > +
> >  #endif
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 0540249..98a39dd 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -167,7 +167,6 @@ static inline red_time_t timespec_to_red_time(struct
> > timespec *time)
> >      return time->tv_sec * (1000 * 1000 * 1000) + time->tv_nsec;
> >  }
> >  
> > -#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> >  static clockid_t clock_id;
> >  
> >  typedef unsigned long stat_time_t;
> > @@ -180,6 +179,7 @@ static stat_time_t stat_now(void)
> >      return ts.tv_nsec + ts.tv_sec * 1000 * 1000 * 1000;
> >  }
> >  
> > +#if defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> >  double stat_cpu_time_to_sec(stat_time_t time)
> >  {
> >      return (double)time / (1000 * 1000 * 1000);
> > @@ -5019,24 +5019,10 @@ static RedDrawable *red_drawable_new(void)
> >      return red;
> >  }
> >  
> > -static void red_record_event(RedWorker *worker, int what, uint32_t type)
> > -{
> > -    struct timespec ts;
> > -    static int counter = 0;
> > -
> > -    // TODO: record the size of the packet in the header. This would make
> > -    // navigating it much faster (well, I can add an index while I'm at
> > it..)
> > -    // and make it trivial to get a histogram from a file.
> > -    // But to implement that I would need some temporary buffer for each
> > event.
> > -    // (that can be up to VGA_FRAMEBUFFER large)
> > -    clock_gettime(worker->record_clock_id, &ts);
> > -    fprintf(worker->record_fd, "event %d %d %d %ld\n", counter++, what,
> > type,
> > -        ts.tv_nsec + ts.tv_sec * 1000 * 1000 * 1000);
> > -}
> > -
> >  static void red_record_command(RedWorker *worker, QXLCommandExt ext_cmd)
> >  {
> > -    red_record_event(worker, 0, ext_cmd.cmd.type);
> > +    red_record_event(worker->record_fd, 0, ext_cmd.cmd.type, stat_now());
> 
> Here we're now using stat_now() instead of getting the timestamp with :
> 
> 	clock_gettime(worker->record_clock_id, &ts);
> 
> stat_now() gets the timestamp by calling:
> 
> 	clock_gettime(clock_id, &ts);
> 
> where 'clock_id' is a global variable in red_worker.c
> 
> This means that the 'worker->record_clock_id' variable is now completely
> unused.
> 
> 

Yes, and also are the same timer.
Could you have multiple red_workers in the same process ??

However I think that these timers are user in later patches but I agree should
be removed in this series.

> > +
> >      switch (ext_cmd.cmd.type) {
> >      case QXL_CMD_DRAW:
> >          red_record_drawable(worker->record_fd, &worker->mem_slots,
> >          ext_cmd.group_id,
> > @@ -11939,7 +11925,7 @@ static void worker_dispatcher_record(void *opaque,
> > uint32_t message_type, void *
> >      RedWorker *worker = opaque;
> >  
> >      if (worker->record_fd) {
> > -        red_record_event(worker, 1, message_type);
> > +        red_record_event(worker->record_fd, 1, message_type, stat_now());
> >      }
> >  }
> >  
> > @@ -12247,11 +12233,9 @@ SPICE_GNUC_NORETURN void *red_worker_main(void
> > *arg)
> >      spice_assert(MAX_PIPE_SIZE > WIDE_CLIENT_ACK_WINDOW &&
> >             MAX_PIPE_SIZE > NARROW_CLIENT_ACK_WINDOW); //ensure wakeup by
> >             ack message
> >  
> > -#if  defined(RED_WORKER_STAT) || defined(COMPRESS_STAT)
> >      if (pthread_getcpuclockid(pthread_self(), &clock_id)) {
> >          spice_error("pthread_getcpuclockid failed");
> >      }
> > -#endif
> >  
> >      red_init(worker, (WorkerInitData *)arg);
> >  
> 
> 
> 

Frediano