Re: [PATCH v3 0/6] virtio_balloon: virtio 1 support

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Tue, 14 Apr 2015 11:21:11 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index f81b220..164e0c2 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -52,15 +52,31 @@ struct virtio_balloon_config {
>  #define VIRTIO_BALLOON_S_MEMTOT   5   /* Total amount of memory */
>  #define VIRTIO_BALLOON_S_NR       6
> 
> +/*
> + * Memory statistics structure.
> + * Driver fills an array of these structures and passes to device.
> + *
> + * NOTE: fields are laid out in a way that would make compiler add padding
> + * between and after fields, so we have to use compiler-specific attributes to
> + * pack it, to disable this padding. This also often causes compiler to
> + * generate suboptimal code.
> + *
> + * We maintain this for backwards compatibility, but don't follow this example.

s/this/the existing statistics structure/

> + *
> + * Do something like the below instead:

If you want to implement a similar structure, do...

Just that nobody gets the idea that they are supposed to implement new
balloon statistics ;)

> + *     struct virtio_balloon_stat {
> + *         __virtio16 tag;
> + *         __u8 reserved[6];
> + *         __virtio64 val;
> + *     };

(...)

> @@ -225,13 +219,8 @@ static inline void update_stat(struct virtio_balloon *vb, int idx,
>  			       u16 tag, u64 val)
>  {
>  	BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> -	if (virtio_has_feature(vb->vdev, VIRTIO_F_VERSION_1)) {
> -		vb->stats[idx].tag = cpu_to_le32(tag);
> -		vb->stats[idx].val = cpu_to_le64(val);
> -	} else {
> -		vb->legacy_stats[idx].tag = tag;
> -		vb->legacy_stats[idx].val = val;
> -	}
> +	vb->stats[idx].tag = cpu_to_virtio16(vb->vdev, tag);

Seems that nobody seemed to care much about statistics...

> +	vb->stats[idx].val = cpu_to_virtio64(vb->vdev, val);
>  }
> 
>  #define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
> 

With these changes merged in:

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>