| Message ID | 20180921172310.10068-4-lbloch@janustech.com |
|---|---|
| State | New |
| Headers | show |
| Series | Take the image size into account when allocating the L2 cache | expand |
On 9/21/18 12:23 PM, Leonid Bloch wrote: > Signed-off-by: Leonid Bloch <lbloch@janustech.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 2 +- > block/qcow2.h | 9 +++++---- > 2 files changed, 6 insertions(+), 5 deletions(-) > > +++ b/block/qcow2.h > @@ -27,6 +27,7 @@ > > #include "crypto/block.h" > #include "qemu/coroutine.h" > +#include "qemu/units.h" > > //#define DEBUG_ALLOC > //#define DEBUG_ALLOC2 > @@ -43,11 +44,11 @@ > > /* 8 MB refcount table is enough for 2 PB images at 64k cluster size > * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ > -#define QCOW_MAX_REFTABLE_SIZE 0x800000 > +#define QCOW_MAX_REFTABLE_SIZE S_8MiB Why not (8 * MiB)? Where does this get stringified that you have to introduce a new macro? Can we instead fix the stringification location to call a convenience helper that converts arbitrary values into convenient strings at runtime, rather than having to worry about a separate macro just for stringification purposes? > > /* 32 MB L1 table is enough for 2 PB images at 64k cluster size > * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ > -#define QCOW_MAX_L1_SIZE 0x2000000 > +#define QCOW_MAX_L1_SIZE S_32MiB Again, why not (32 * MiB)? > > /* Allow for an average of 1k per snapshot table entry, should be plenty of > * space for snapshot names and IDs */ > @@ -75,9 +76,9 @@ > > /* Whichever is more */ > #define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */ > -#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */ > +#define DEFAULT_L2_CACHE_SIZE S_1MiB and (1 * MiB) > > -#define DEFAULT_CLUSTER_SIZE 65536 > +#define DEFAULT_CLUSTER_SIZE S_64KiB and (64 * KiB)
On 9/24/18 7:59 PM, Eric Blake wrote: > On 9/21/18 12:23 PM, Leonid Bloch wrote: >> Signed-off-by: Leonid Bloch <lbloch@janustech.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> block/qcow2.c | 2 +- >> block/qcow2.h | 9 +++++---- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> > >> +++ b/block/qcow2.h >> @@ -27,6 +27,7 @@ >> #include "crypto/block.h" >> #include "qemu/coroutine.h" >> +#include "qemu/units.h" >> //#define DEBUG_ALLOC >> //#define DEBUG_ALLOC2 >> @@ -43,11 +44,11 @@ >> /* 8 MB refcount table is enough for 2 PB images at 64k cluster size >> * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ >> -#define QCOW_MAX_REFTABLE_SIZE 0x800000 >> +#define QCOW_MAX_REFTABLE_SIZE S_8MiB > > Why not (8 * MiB)? Where does this get stringified that you have to > introduce a new macro? Can we instead fix the stringification location > to call a convenience helper that converts arbitrary values into > convenient strings at runtime, rather than having to worry about a > separate macro just for stringification purposes? - Why not (8 * MiB)? Because we have the lookup table from the previous patch. - Why do we need this table? Because some values (not this one specifically, but for example DEFAULT_CLUSTER_SIZE) get stringified at compile time (not during runtime), and for these values the only two options are to use such a table for convenient writing, or to write the number of bytes explicitly in the definition. Again, while the necessity for such a table comes from stringification, it has an added value of short and clear size definitions. > >> /* 32 MB L1 table is enough for 2 PB images at 64k cluster size >> * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ >> -#define QCOW_MAX_L1_SIZE 0x2000000 >> +#define QCOW_MAX_L1_SIZE S_32MiB > > Again, why not (32 * MiB)? Same as above. > >> /* Allow for an average of 1k per snapshot table entry, should be >> plenty of >> * space for snapshot names and IDs */ >> @@ -75,9 +76,9 @@ >> /* Whichever is more */ >> #define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */ >> -#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */ >> +#define DEFAULT_L2_CACHE_SIZE S_1MiB > > and (1 * MiB) Same as above. > >> -#define DEFAULT_CLUSTER_SIZE 65536 >> +#define DEFAULT_CLUSTER_SIZE S_64KiB > > and (64 * KiB) Because: block/qcow2.c 4726: .def_value_str = stringify(DEFAULT_CLUSTER_SIZE) (and similar in some other files). Leonid.
diff --git a/block/qcow2.c b/block/qcow2.c index ec9e6238a0..67cc82f0b9 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -830,7 +830,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, } } else { if (!l2_cache_size_set) { - *l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE, + *l2_cache_size = MAX(DEFAULT_L2_CACHE_SIZE, (uint64_t)DEFAULT_L2_CACHE_CLUSTERS * s->cluster_size); } diff --git a/block/qcow2.h b/block/qcow2.h index 81b844e936..a8d6f757b1 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -27,6 +27,7 @@ #include "crypto/block.h" #include "qemu/coroutine.h" +#include "qemu/units.h" //#define DEBUG_ALLOC //#define DEBUG_ALLOC2 @@ -43,11 +44,11 @@ /* 8 MB refcount table is enough for 2 PB images at 64k cluster size * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ -#define QCOW_MAX_REFTABLE_SIZE 0x800000 +#define QCOW_MAX_REFTABLE_SIZE S_8MiB /* 32 MB L1 table is enough for 2 PB images at 64k cluster size * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ -#define QCOW_MAX_L1_SIZE 0x2000000 +#define QCOW_MAX_L1_SIZE S_32MiB /* Allow for an average of 1k per snapshot table entry, should be plenty of * space for snapshot names and IDs */ @@ -75,9 +76,9 @@ /* Whichever is more */ #define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */ -#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */ +#define DEFAULT_L2_CACHE_SIZE S_1MiB -#define DEFAULT_CLUSTER_SIZE 65536 +#define DEFAULT_CLUSTER_SIZE S_64KiB #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"