From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1NAZNl-0002uf-7w
	for qemu-devel@nongnu.org; Tue, 17 Nov 2009 20:30:53 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1NAZNg-0002u5-Ec
	for qemu-devel@nongnu.org; Tue, 17 Nov 2009 20:30:52 -0500
Received: from [199.232.76.173] (port=37831 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1NAZNg-0002u2-9r
	for qemu-devel@nongnu.org; Tue, 17 Nov 2009 20:30:48 -0500
Received: from ozlabs.org ([203.10.76.45]:41751)
	by monty-python.gnu.org with esmtps
	(TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60)
	(envelope-from <rusty@rustcorp.com.au>) id 1NAZNf-00040N-6v
	for qemu-devel@nongnu.org; Tue, 17 Nov 2009 20:30:47 -0500
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Wed, 18 Nov 2009 12:00:39 +1030
References: <1258488996.2820.35.camel@aglitke>
	<1258490189.2820.37.camel@aglitke>
In-Reply-To: <1258490189.2820.37.camel@aglitke>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="utf-8"
Content-Transfer-Encoding: 7bit
Message-Id: <200911181200.39971.rusty@rustcorp.com.au>
Subject: [Qemu-devel] Re: virtio: Add memory statistics reporting to the
	balloon driver (V2)
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Adam Litke <agl@us.ibm.com>
Cc: linux-kernel@vger.kernel.org, Anthony Liguori <aliguori@us.ibm.com>, virtualization@lists.linux-foundation.org, qemu-devel@nongnu.org, Avi Kivity <avi@redhat.com>

On Wed, 18 Nov 2009 07:06:29 am Adam Litke wrote:
> virtio: Add memory statistics reporting to the balloon driver (V2)
> 
> Changes since V1:
>  - Use a virtqueue instead of the device config space

Hi Adam,

    If Anthony's happy, I'm happy with this approach.

Couple of minor points:

> +static inline void update_stat(struct virtio_balloon *vb, int idx,
> +				unsigned int tag, unsigned long val)
> +{
> +	BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
> +	vb->stats[idx].tag = tag;
> +	vb->stats[idx].val = cpu_to_le32(val);
> +}

The little-endian conversion of the balloon driver is a historical mistake
(no other driver does this).  Let's not extend it to the stats.

Here you've done one and not the other, which is even worse.  (Sparse would
have found this, I assume).

> +struct virtio_balloon_stat
> +{
> +	__le16 tag;
> +	__le32 val;
> +};

Is 32 bits sufficient?  A big machine might get over 4bn faults, and certainly
4 TB of memory isn't that far away.

Thanks,
Rusty.