[Spice-devel] [PATCH 01/17] Add RedsState arg to all stat functions

Jonathon Jongsma jjongsma at redhat.com
Fri Feb 12 16:26:48 UTC 2016


On Fri, 2016-02-12 at 11:20 -0500, Frediano Ziglio wrote:
> 
> ----- Original Message -----
> > From: "Frediano Ziglio" <fziglio at redhat.com>
> > To: "Jonathon Jongsma" <jjongsma at redhat.com>
> > Cc: spice-devel at lists.freedesktop.org
> > Sent: Friday, February 12, 2016 4:11:36 PM
> > Subject: Re: [Spice-devel] [PATCH 01/17] Add RedsState arg to all stat
> > functions
> > 
> > > 
> > > On Thu, 2016-02-11 at 14:27 -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > > > > 
> > > > > ---
> > > > >  server/dcc-send.c        |  6 +++---
> > > > >  server/display-channel.c | 12 ++++++------
> > > > >  server/main-channel.c    |  2 +-
> > > > >  server/red-channel.c     |  4 ++--
> > > > >  server/red-worker.c      | 12 ++++++------
> > > > >  server/reds.c            | 22 +++++++++++-----------
> > > > >  server/reds.h            |  3 ---
> > > > >  server/stat.h            | 22 ++++++++++++----------
> > > > >  8 files changed, 41 insertions(+), 42 deletions(-)
> > > > > 
> > > > > diff --git a/server/dcc-send.c b/server/dcc-send.c
> > > > > index 3af5760..ca7e7e2 100644
> > > > > --- a/server/dcc-send.c
> > > > > +++ b/server/dcc-send.c
> > > > > @@ -207,13 +207,13 @@ static void
> > > > > red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> > > > >                  io_image->descriptor.flags |=
> > > > >                  SPICE_IMAGE_FLAGS_CACHE_ME;
> > > > >                  dcc->send_data.pixmap_cache_items[dcc
> > > > > ->send_data.num_pixmap_cache_items++]
> > > > >                  =
> > > > >                                                                       
> > > > >       
> > > > >     image->descriptor.id;
> > > > > -
> > > > > stat_inc_counter(display_channel->add_to_cache_counter,
> > > > > 1);
> > > > > +                reds_stat_inc_counter(reds,
> > > > > display_channel->add_to_cache_counter, 1);
> > > > 
> > > > Before I forget, I would rather avoid to add reds_ prefix to these
> > > > functions/macros.
> > > 
> > > I don't really care either way. I just renamed them since the first
> > > RedsState
> > > parameter made them act like a member function of RedsState. But they are
> > > a
> > > bit
> > > of a special case.
> > > 
> > > 
> > > > 
> > > > >              }
> > > > >          }
> > > > >      }
> > > > >  
> > > > >      if (!(io_image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {
> > > > > -        stat_inc_counter(display_channel->non_cache_counter, 1);
> > > > > +        reds_stat_inc_counter(reds,
> > > > > display_channel->non_cache_counter,
> > > > > 1);
> > > > >      }
> > > > >  }
> > > > >  
> > > > > @@ -376,7 +376,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> > > > > *dcc,
> > > > > SpiceMarshaller *m,
> > > > >                                       &bitmap_palette_out,
> > > > >                                       &lzplt_palette_out);
> > > > >                  spice_assert(bitmap_palette_out == NULL);
> > > > >                  spice_assert(lzplt_palette_out == NULL);
> > > > > -                stat_inc_counter(display->cache_hits_counter, 1);
> > > > > +                reds_stat_inc_counter(reds,
> > > > > display->cache_hits_counter,
> > > > > 1);
> > > > >                  pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> > > > >                  return FILL_BITS_TYPE_CACHE;
> > > > >              } else {
> > > > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > > > index 78c984f..5b496ae 100644
> > > > > --- a/server/display-channel.c
> > > > > +++ b/server/display-channel.c
> > > > > @@ -2047,12 +2047,12 @@ DisplayChannel* display_channel_new(RedWorker
> > > > > *worker, int migrate, int stream_v
> > > > >      stat_init(&display->__exclude_stat, "__exclude", stat_clock);
> > > > >  #ifdef RED_STATISTICS
> > > > >      RedChannel *channel = RED_CHANNEL(display);
> > > > > -    display->cache_hits_counter = stat_add_counter(channel->stat,
> > > > > -
> > > > > "cache_hits",
> > > > > TRUE);
> > > > > -    display->add_to_cache_counter = stat_add_counter(channel->stat,
> > > > > -
> > > > >  "add_to_cache",
> > > > > TRUE);
> > > > > -    display->non_cache_counter = stat_add_counter(channel->stat,
> > > > > -                                                         
> > > > >  "non_cache",
> > > > > TRUE);
> > > > > +    display->cache_hits_counter = reds_stat_add_counter(reds, channel
> > > > > ->stat,
> > > > > +                                                        "cache_hits",
> > > > > TRUE);
> > > > > +    display->add_to_cache_counter = reds_stat_add_counter(reds,
> > > > > channel->stat,
> > > > > +
> > > > > "add_to_cache",
> > > > > TRUE);
> > > > > +    display->non_cache_counter = reds_stat_add_counter(reds,
> > > > > channel->stat,
> > > > > +                                                       "non_cache",
> > > > > TRUE);
> > > > >  #endif
> > > > >      stat_compress_init(&display->lz_stat, "lz", stat_clock);
> > > > >      stat_compress_init(&display->glz_stat, "glz", stat_clock);
> > > > > diff --git a/server/main-channel.c b/server/main-channel.c
> > > > > index d69095d..8b1d7d4 100644
> > > > > --- a/server/main-channel.c
> > > > > +++ b/server/main-channel.c
> > > > > @@ -985,7 +985,7 @@ static int
> > > > > main_channel_handle_parsed(RedChannelClient
> > > > > *rcc, uint32_t size, uint
> > > > >              red_channel_client_handle_message(rcc, size, type,
> > > > >              message);
> > > > >          }
> > > > >  #ifdef RED_STATISTICS
> > > > > -        reds_update_stat_value(roundtrip);
> > > > > +        reds_update_stat_value(reds, roundtrip);
> > > > >  #endif
> > > > >          break;
> > > > >      }
> > > > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > > > index 190779a..a6069a9 100644
> > > > > --- a/server/red-channel.c
> > > > > +++ b/server/red-channel.c
> > > > > @@ -396,7 +396,7 @@ static void red_channel_client_on_output(void
> > > > > *opaque,
> > > > > int n)
> > > > >      if (rcc->connectivity_monitor.timer) {
> > > > >          rcc->connectivity_monitor.out_bytes += n;
> > > > >      }
> > > > > -    stat_inc_counter(rcc->channel->out_bytes_counter, n);
> > > > > +    reds_stat_inc_counter(reds, rcc->channel->out_bytes_counter, n);
> > > > >  }
> > > > >  
> > > > >  static void red_channel_client_on_input(void *opaque, int n)
> > > > > @@ -1162,7 +1162,7 @@ void red_channel_set_stat_node(RedChannel
> > > > > *channel,
> > > > > StatNodeRef stat)
> > > > >  
> > > > >  #ifdef RED_STATISTICS
> > > > >      channel->stat = stat;
> > > > > -    channel->out_bytes_counter = stat_add_counter(stat, "out_bytes",
> > > > > TRUE);
> > > > > +    channel->out_bytes_counter = reds_stat_add_counter(reds, stat,
> > > > > "out_bytes", TRUE);
> > > > >  #endif
> > > > >  }
> > > > >  
> > > > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > > > index e7d0671..7e5742f 100644
> > > > > --- a/server/red-worker.c
> > > > > +++ b/server/red-worker.c
> > > > > @@ -244,7 +244,7 @@ static int red_process_display(RedWorker *worker,
> > > > > int
> > > > > *ring_is_empty)
> > > > >              red_record_qxl_command(worker->record_fd,
> > > > >              &worker->mem_slots,
> > > > >              ext_cmd,
> > > > >                                     stat_now(CLOCK_MONOTONIC));
> > > > >  
> > > > > -        stat_inc_counter(worker->command_counter, 1);
> > > > > +        reds_stat_inc_counter(reds, worker->command_counter, 1);
> > > > >          worker->display_poll_tries = 0;
> > > > >          switch (ext_cmd.cmd.type) {
> > > > >          case QXL_CMD_DRAW: {
> > > > > @@ -521,7 +521,7 @@ CommonChannel *red_worker_new_channel(RedWorker
> > > > > *worker,
> > > > > int size,
> > > > >                                          channel_cbs,
> > > > >                                          migration_flags);
> > > > >      spice_return_val_if_fail(channel, NULL);
> > > > > -    red_channel_set_stat_node(channel, stat_add_node(worker->stat,
> > > > > name,
> > > > > TRUE));
> > > > > +    red_channel_set_stat_node(channel, reds_stat_add_node(reds,
> > > > > worker->stat, name, TRUE));
> > > > >  
> > > > >      common = (CommonChannel *)channel;
> > > > >      common->worker = worker;
> > > > > @@ -856,7 +856,7 @@ static void handle_dev_wakeup(void *opaque, void
> > > > > *payload)
> > > > >  {
> > > > >      RedWorker *worker = opaque;
> > > > >  
> > > > > -    stat_inc_counter(worker->wakeup_counter, 1);
> > > > > +    reds_stat_inc_counter(reds, worker->wakeup_counter, 1);
> > > > >      red_dispatcher_clear_pending(worker->red_dispatcher,
> > > > >      RED_DISPATCHER_PENDING_WAKEUP);
> > > > >  }
> > > > >  
> > > > > @@ -1541,9 +1541,9 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > > > > RedDispatcher *red_dispatcher)
> > > > >  #ifdef RED_STATISTICS
> > > > >      char worker_str[20];
> > > > >      sprintf(worker_str, "display[%d]", worker->qxl->id);
> > > > > -    worker->stat = stat_add_node(INVALID_STAT_REF, worker_str, TRUE);
> > > > > -    worker->wakeup_counter = stat_add_counter(worker->stat,
> > > > > "wakeups",
> > > > > TRUE);
> > > > > -    worker->command_counter = stat_add_counter(worker->stat,
> > > > > "commands",
> > > > > TRUE);
> > > > > +    worker->stat = reds_stat_add_node(reds, INVALID_STAT_REF,
> > > > > worker_str,
> > > > > TRUE);
> > > > > +    worker->wakeup_counter = reds_stat_add_counter(reds, worker
> > > > > ->stat,
> > > > > "wakeups", TRUE);
> > > > > +    worker->command_counter = reds_stat_add_counter(reds,
> > > > > worker->stat,
> > > > > "commands", TRUE);
> > > > >  #endif
> > > > >  
> > > > >      worker->dispatch_watch =
> > > > > diff --git a/server/reds.c b/server/reds.c
> > > > > index 16327b5..c33aded 100644
> > > > > --- a/server/reds.c
> > > > > +++ b/server/reds.c
> > > > > @@ -216,7 +216,7 @@ static void reds_link_free(RedLinkInfo *link)
> > > > >  
> > > > >  #ifdef RED_STATISTICS
> > > > >  
> > > > > -static void insert_stat_node(StatNodeRef parent, StatNodeRef ref)
> > > > > +static void reds_insert_stat_node(RedsState *reds, StatNodeRef
> > > > > parent,
> > > > > StatNodeRef ref)
> > > > >  {
> > > > >      SpiceStatNode *node = &reds->stat->nodes[ref];
> > > > >      uint32_t pos = INVALID_STAT_REF;
> > > > > @@ -243,7 +243,7 @@ static void insert_stat_node(StatNodeRef parent,
> > > > > StatNodeRef ref)
> > > > >      }
> > > > >  }
> > > > >  
> > > > > -StatNodeRef stat_add_node(StatNodeRef parent, const char *name, int
> > > > > visible)
> > > > > +StatNodeRef reds_stat_add_node(RedsState *reds, StatNodeRef parent,
> > > > > const
> > > > > char *name, int visible)
> > > > >  {
> > > > >      StatNodeRef ref;
> > > > >      SpiceStatNode *node;
> > > > > @@ -280,12 +280,12 @@ StatNodeRef stat_add_node(StatNodeRef parent,
> > > > > const
> > > > > char *name, int visible)
> > > > >      node->value = 0;
> > > > >      node->flags = SPICE_STAT_NODE_FLAG_ENABLED | (visible ?
> > > > >      SPICE_STAT_NODE_FLAG_VISIBLE : 0);
> > > > >      g_strlcpy(node->name, name, sizeof(node->name));
> > > > > -    insert_stat_node(parent, ref);
> > > > > +    reds_insert_stat_node(reds, parent, ref);
> > > > >      pthread_mutex_unlock(&reds->stat_lock);
> > > > >      return ref;
> > > > >  }
> > > > >  
> > > > > -static void stat_remove(SpiceStatNode *node)
> > > > > +static void reds_stat_remove(RedsState *reds, SpiceStatNode *node)
> > > > >  {
> > > > >      pthread_mutex_lock(&reds->stat_lock);
> > > > >      node->flags &= ~SPICE_STAT_NODE_FLAG_ENABLED;
> > > > > @@ -294,14 +294,14 @@ static void stat_remove(SpiceStatNode *node)
> > > > >      pthread_mutex_unlock(&reds->stat_lock);
> > > > >  }
> > > > >  
> > > > > -void stat_remove_node(StatNodeRef ref)
> > > > > +void reds_stat_remove_node(RedsState *reds, StatNodeRef ref)
> > > > >  {
> > > > > -    stat_remove(&reds->stat->nodes[ref]);
> > > > > +    reds_stat_remove(reds, &reds->stat->nodes[ref]);
> > > > >  }
> > > > >  
> > > > > -uint64_t *stat_add_counter(StatNodeRef parent, const char *name, int
> > > > > visible)
> > > > > +uint64_t *reds_stat_add_counter(RedsState *reds, StatNodeRef parent,
> > > > > const
> > > > > char *name, int visible)
> > > > >  {
> > > > > -    StatNodeRef ref = stat_add_node(parent, name, visible);
> > > > > +    StatNodeRef ref = reds_stat_add_node(reds, parent, name,
> > > > > visible);
> > > > >      SpiceStatNode *node;
> > > > >  
> > > > >      if (ref == INVALID_STAT_REF) {
> > > > > @@ -312,12 +312,12 @@ uint64_t *stat_add_counter(StatNodeRef parent,
> > > > > const
> > > > > char *name, int visible)
> > > > >      return &node->value;
> > > > >  }
> > > > >  
> > > > > -void stat_remove_counter(uint64_t *counter)
> > > > > +void reds_stat_remove_counter(RedsState *reds, uint64_t *counter)
> > > > >  {
> > > > > -    stat_remove((SpiceStatNode *)(counter - offsetof(SpiceStatNode,
> > > > > value)));
> > > > > +    reds_stat_remove(reds, (SpiceStatNode *)(counter -
> > > > > offsetof(SpiceStatNode, value)));
> > > > >  }
> > > > >  
> > > > > -void reds_update_stat_value(uint32_t value)
> > > > > +void reds_update_stat_value(RedsState *reds, uint32_t value)
> > > > >  {
> > > > >      RedsStatValue *stat_value = &reds->roundtrip_stat;
> > > > >  
> > > > > diff --git a/server/reds.h b/server/reds.h
> > > > > index d9f4e83..6caed73 100644
> > > > > --- a/server/reds.h
> > > > > +++ b/server/reds.h
> > > > > @@ -85,9 +85,6 @@ typedef struct MainMigrateData MainMigrateData;
> > > > >  void reds_marshall_migrate_data(RedsState *reds, SpiceMarshaller *m);
> > > > >  void reds_fill_channels(RedsState *reds, SpiceMsgChannels
> > > > >  *channels_info);
> > > > >  int reds_get_n_channels(RedsState *reds);
> > > > > -#ifdef RED_STATISTICS
> > > > > -void reds_update_stat_value(uint32_t value);
> > > > > -#endif
> > > > >  
> > > > >  /* callbacks from main channel messages */
> > > > >  
> > > > > diff --git a/server/stat.h b/server/stat.h
> > > > > index 6cb0046..ad0ea96 100644
> > > > > --- a/server/stat.h
> > > > > +++ b/server/stat.h
> > > > > @@ -23,25 +23,27 @@
> > > > >  
> > > > >  typedef uint32_t StatNodeRef;
> > > > >  #define INVALID_STAT_REF (~(StatNodeRef)0)
> > > > > +typedef struct RedsState RedsState;
> > > > >  
> > > > 
> > > > I don't like this typedef here, is out of "standard".
> > > > We usually typedef in the same header we define it, unless is private.
> > > 
> > > True, I believe I added this typedef as a compromise since including the
> > > full
> > > reds.h here would have created a circular dependency:
> > > 
> > > make[4]: Entering directory '/home/jjongsma/work/spice/_build/server'
> > >   CC       agent-msg-filter.lo
> > > In file included from ../../server/red-channel.h:35:0,
> > >                  from ../../server/reds.h:30,
> > >                  from ../../server/agent-msg-filter.c:27:
> > > ../../server/stat.h:29:32: error: unknown type name 'RedsState'
> > >  StatNodeRef reds_stat_add_node(RedsState *reds, StatNodeRef parent, const
> > >  char
> > > *name, int visible);
> > >                                 ^
> > > 
> > > 
> > > I can try to find a different approach if you want, but I suspect it will
> > > involve a lot of shuffling of code.
> > > 
> > > 
> > 
> > Usually my approach is (in the headers) just to declare the structure like
> > 
> > struct Dummy;
> > 
> > and use the full name like
> > 
> > void do_something(struct Dummy *dummy);
> > 
> > we had a similar discussion with Marc-Andre and one option would be to move
> > all struct typedefs' in red-common.h (which is included in all files).
> > I think would be worth but could be quite some manual work.
> > Also I think this is not the style GObject is using.
> > 
> 
> Actually for RedsState there is a solution, in spice-server.h you have
> 
> typedef struct RedsState SpiceServer;
> 
> so if you use SpiceServer instead of RedsState you don't need to use typedef
> (as spice-server.h is included from red-common.h).

Ah, that seems like a reasonable compromise. will post a follow-up patch.


> 
> Frediano
> 
> 
> > 
> > > > 
> > > > >  #ifdef RED_STATISTICS
> > > > > -StatNodeRef stat_add_node(StatNodeRef parent, const char *name, int
> > > > > visible);
> > > > > -void stat_remove_node(StatNodeRef node);
> > > > > -uint64_t *stat_add_counter(StatNodeRef parent, const char *name, int
> > > > > visible);
> > > > > -void stat_remove_counter(uint64_t *counter);
> > > > > +StatNodeRef reds_stat_add_node(RedsState *reds, StatNodeRef parent,
> > > > > const
> > > > > char *name, int visible);
> > > > > +void reds_stat_remove_node(RedsState *reds, StatNodeRef node);
> > > > > +uint64_t *reds_stat_add_counter(RedsState *reds, StatNodeRef parent,
> > > > > const
> > > > > char *name, int visible);
> > > > > +void reds_stat_remove_counter(RedsState *reds, uint64_t *counter);
> > > > > +void reds_update_stat_value(RedsState *reds, uint32_t value);
> > > > >  
> > > > > -#define stat_inc_counter(counter, value) {  \
> > > > > +#define reds_stat_inc_counter(reds, counter, value) {  \
> > > > >      if (counter) {                          \
> > > > >          *(counter) += (value);              \
> > > > >      }                                       \
> > > > >  }
> > > > >  
> > > > >  #else
> > > > > -#define stat_add_node(p, n, v) INVALID_STAT_REF
> > > > > -#define stat_remove_node(n)
> > > > > -#define stat_add_counter(p, n, v) NULL
> > > > > -#define stat_remove_counter(c)
> > > > > -#define stat_inc_counter(c, v)
> > > > > +#define reds_stat_add_node(r, p, n, v) INVALID_STAT_REF
> > > > > +#define reds_stat_remove_node(r, n)
> > > > > +#define reds_stat_add_counter(r, p, n, v) NULL
> > > > > +#define reds_stat_remove_counter(r, c)
> > > > > +#define reds_stat_inc_counter(r, c, v)
> > > > >  #endif /* RED_STATISTICS */
> > > > >  
> > > > >  typedef uint64_t stat_time_t;
> > > > 
> > > > Frediano
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 


More information about the Spice-devel mailing list