[Spice-devel] [spice-server v2 1/6] Add RedCharDeviceWriteBufferPrivate

[Frediano Ziglio]

> 
> On Fri, Apr 28, 2017 at 05:25:31AM -0400, Frediano Ziglio wrote:
> > > 
> > > This is intended to hold the fields that only char-device.c has a use
> > > for, but for now this only adds the boilerplate for it, the next commit
> > > will move the relevant field there.
> > > 
> > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > ---
> > >  server/char-device.c | 5 +++++
> > >  server/char-device.h | 3 +++
> > >  2 files changed, 8 insertions(+)
> > > 
> > > diff --git a/server/char-device.c b/server/char-device.c
> > > index d32593209..fa137e416 100644
> > > --- a/server/char-device.c
> > > +++ b/server/char-device.c
> > > @@ -31,6 +31,9 @@
> > >  #define RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT 30000
> > >  #define MAX_POOL_SIZE (10 * 64 * 1024)
> > >  
> > > +struct RedCharDeviceWriteBufferPrivate {
> > > +};
> > > +
> > >  typedef struct RedCharDeviceClient RedCharDeviceClient;
> > >  struct RedCharDeviceClient {
> > >      RedCharDevice *dev;
> > > @@ -144,6 +147,7 @@ static void
> > > red_char_device_write_buffer_free(RedCharDeviceWriteBuffer *buf)
> > >          return;
> > >  
> > >      free(buf->buf);
> > > +    free(buf->priv);
> > >      free(buf);
> > >  }
> > >  
> > > @@ -540,6 +544,7 @@ static RedCharDeviceWriteBuffer
> > > *__red_char_device_write_buffer_get(
> > >          dev->priv->cur_pool_size -= ret->buf_size;
> > >      } else {
> > >          ret = spice_new0(RedCharDeviceWriteBuffer, 1);
> > > +        ret->priv = spice_new0(RedCharDeviceWriteBufferPrivate, 1);
> > >      }
> > >  
> > >      spice_assert(!ret->buf_used);
> > 
> > Would not be better if this is always allocated here to have a
> > public part and a private one after the public to avoid the
> > indirection penalty?
> > 
> > So kind of
> > 
> > struct RedCharDeviceWriteBufferPrivate {
> >    RedCharDeviceWriteBuffer base;
> >    .. additional fields here ...
> > };
> > 
> > ...
> > 
> > ret = &spice_new0(RedCharDeviceWriteBufferPrivate, 1)->base;
> 
> I considered playing tricks like
> ret = spice_malloc0(sizeof(RedCharDeviceWriteBuffer) +
> sizeof(RedCharDeviceWriteBufferPrivate));
> ret->priv = &ret[1];
> 
> to allocate it, but was not sure it was worth it over the simple version
> in this patch.
> 

a priv[1] would save you the indirection too.
Note that spice_malloc0(sizeof(RedCharDeviceWriteBuffer) +
 sizeof(RedCharDeviceWriteBufferPrivate)) could be not portable
as it assumes the size of the first structure is a multiple of the
alignment of the second.

> > 
> > > diff --git a/server/char-device.h b/server/char-device.h
> > > index 7854086c3..9bfd7c656 100644
> > > --- a/server/char-device.h
> > > +++ b/server/char-device.h
> > > @@ -146,6 +146,7 @@ GType red_char_device_get_type(void) G_GNUC_CONST;
> > >   * */
> > >  
> > >  /* buffer that is used for writing to the device */
> > > +typedef struct RedCharDeviceWriteBufferPrivate
> > > RedCharDeviceWriteBufferPrivate;
> > >  typedef struct RedCharDeviceWriteBuffer {
> > >      int origin;
> > >      RedClient *client; /* The client that sent the message to the
> > >      device.
> > > @@ -156,6 +157,8 @@ typedef struct RedCharDeviceWriteBuffer {
> > >      uint32_t buf_used;
> > >      uint32_t token_price;
> > >      uint32_t refs;
> > > +
> > > +    RedCharDeviceWriteBufferPrivate *priv;
> > >  } RedCharDeviceWriteBuffer;
> > >  
> > >  void red_char_device_reset_dev_instance(RedCharDevice *dev,
> > 
> > I would put client field (patch 3/6) as first to reduce structure size on
> > 64 bit.
> 
> I kept the same order as in the initial struct. However, moving things
> around does not seem to make a difference size-wise, padding is added to the
> end rather than just after the first member.
> 

Yes, but usually you add fields at the end so you'll use the padding,
paying attention to middle holes is harder and if you want to add
2 fields you would have to split the patch filling the hole and adding
another field which could be bad if the 2 fields are logically related.

> struct RedCharDeviceWriteBufferPrivate {
>         RedClient *                client;               /*     0     8 */
>         WriteBufferOrigin          origin;               /*     8     4 */
>         uint32_t                   token_price;          /*    12     4 */
>         uint32_t                   refs;                 /*    16     4 */
> 
>         /* size: 24, cachelines: 1, members: 4 */
>         /* padding: 4 */
>         /* last cacheline: 24 bytes */
> };
> 
> struct RedCharDeviceWriteBufferPrivate {
>         WriteBufferOrigin          origin;               /*     0     4 */
> 
>         /* XXX 4 bytes hole, try to pack */
> 
>         RedClient *                client;               /*     8     8 */
>         uint32_t                   token_price;          /*    16     4 */
>         uint32_t                   refs;                 /*    20     4 */
> 
>         /* size: 24, cachelines: 1, members: 4 */
>         /* sum members: 20, holes: 1, sum holes: 4 */
>         /* last cacheline: 24 bytes */
> };
> 
> Christophe
> 

Frediano