Re: [Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

2013-01-17 Thread Benoît Canet 
Le Wednesday 16 Jan 2013 à 13:10:12 (-0700), Eric Blake a écrit :
 On 01/16/2013 09:25 AM, Benoît Canet wrote:
  ---
   block/qcow2-dedup.c |   13 +
   block/qcow2.h   |1 +
   2 files changed, 14 insertions(+)
  
  diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
  index db23b71..4305746 100644
  --- a/block/qcow2-dedup.c
  +++ b/block/qcow2-dedup.c
  @@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
   {
   qcow2_dedup_free(bs);
   }
  +
  +#define GTREE_NODE_SIZE sizeof(int) * 5
 
 Improperly parenthesized.  Also, this feels like a magic number, is
 there an actual sizeof(struct) you could use instead of hand-computing
 how much is used per node?

No the glib implementation totally hide it's structures.

Re: [Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

2013-01-17 Thread Eric Blake 
On 01/17/2013 04:11 AM, Benoît Canet wrote:
 Le Wednesday 16 Jan 2013 à 13:10:12 (-0700), Eric Blake a écrit :
 On 01/16/2013 09:25 AM, Benoît Canet wrote:
 ---
  block/qcow2-dedup.c |   13 +
  block/qcow2.h   |1 +
  2 files changed, 14 insertions(+)

 diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
 index db23b71..4305746 100644
 --- a/block/qcow2-dedup.c
 +++ b/block/qcow2-dedup.c
 @@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
  {
  qcow2_dedup_free(bs);
  }
 +
 +#define GTREE_NODE_SIZE sizeof(int) * 5

 Improperly parenthesized.  Also, this feels like a magic number, is
 there an actual sizeof(struct) you could use instead of hand-computing
 how much is used per node?
 
 No the glib implementation totally hide it's structures.

Also, are you sure that this value is correct on both 32-bit and 64-bit
machines, or does a gtree node include a void* which changes how much
memory it uses based on platform?  Even adding a comment pointing to the
internal glib implementation that you were copying from would be
helpful, so that it doesn't feel quite so magic.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

2013-01-16 Thread Benoît Canet 
---
 block/qcow2-dedup.c |   13 +
 block/qcow2.h   |1 +
 2 files changed, 14 insertions(+)

diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
index db23b71..4305746 100644
--- a/block/qcow2-dedup.c
+++ b/block/qcow2-dedup.c
@@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
 {
 qcow2_dedup_free(bs);
 }
+
+#define GTREE_NODE_SIZE sizeof(int) * 5
+
+void qcow2_dedup_update_metrics(BlockDriverState *bs)
+{
+BDRVQcowState *s = bs-opaque;
+
+uint64_t nb_hashs = s-dedup_metrics.ram_hash_creations -
+s-dedup_metrics.ram_hash_deletions;
+
+s-dedup_metrics.ram_usage = nb_hashs * GTREE_NODE_SIZE * 2;
+s-dedup_metrics.ram_usage += nb_hashs * sizeof(QCowHashNode);
+}
diff --git a/block/qcow2.h b/block/qcow2.h
index 0729ff2..d8e8539 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -510,5 +510,6 @@ void qcow2_dedup_refcount_half_max_reached(BlockDriverState 
*bs,
 bool qcow2_dedup_is_running(BlockDriverState *bs);
 int qcow2_dedup_init(BlockDriverState *bs);
 void qcow2_dedup_close(BlockDriverState *bs);
+void qcow2_dedup_update_metrics(BlockDriverState *bs);
 
 #endif
-- 
1.7.10.4

Re: [Qemu-devel] [RFC V1 12/14] qcow2: Add qcow2_dedup_update_metrics to compute dedup RAM usage.

2013-01-16 Thread Eric Blake 
On 01/16/2013 09:25 AM, Benoît Canet wrote:
 ---
  block/qcow2-dedup.c |   13 +
  block/qcow2.h   |1 +
  2 files changed, 14 insertions(+)
 
 diff --git a/block/qcow2-dedup.c b/block/qcow2-dedup.c
 index db23b71..4305746 100644
 --- a/block/qcow2-dedup.c
 +++ b/block/qcow2-dedup.c
 @@ -1311,3 +1311,16 @@ void qcow2_dedup_close(BlockDriverState *bs)
  {
  qcow2_dedup_free(bs);
  }
 +
 +#define GTREE_NODE_SIZE sizeof(int) * 5

Improperly parenthesized.  Also, this feels like a magic number, is
there an actual sizeof(struct) you could use instead of hand-computing
how much is used per node?

 +
 +void qcow2_dedup_update_metrics(BlockDriverState *bs)
 +{
 +BDRVQcowState *s = bs-opaque;
 +
 +uint64_t nb_hashs = s-dedup_metrics.ram_hash_creations -
 +s-dedup_metrics.ram_hash_deletions;
 +
 +s-dedup_metrics.ram_usage = nb_hashs * GTREE_NODE_SIZE * 2;

But you got lucky that order of operations didn't care about the missing
() here.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature