* [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
@ 2019-01-11 19:14 Markus Armbruster
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Markus Armbruster @ 2019-01-11 19:14 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, eblake, lbloch, qemu-block
Back in September, Leonid Block added a whole bunch of macros (commit
540b8492618) to improve readability of qcow2.h a bit (commit
b6a95c6d100). He later used them to fix the "vdi" driver's parameter
cluster_size's default value (commit 3dd5b8f4718). He has now
proposed a further patch[1] to auto-generate these macros. That patch
feels overengineered to me.
On closer examination, I found I dislike the macros before his new
patch. So did Eric Blake.
The macros exist because the common KiB, MiB, ... macros aren't usable
when you need a literal rather than a constant expression.
stringify() does, and we use it to define the QemuOpts default value.
Eric proposed to improve QemuOpts to accept integer default values,
too[2]. Before I review that patch series, I want to establish a
"stupidest solution that can possibly work" baseline. And that's what
this patch is.
[1] [PATCH v2 0/1] include: Auto-generate the sizes lookup table
Message-ID: <20190103213320.2653-1-lbloch@janustech.com>
[2] [PATCH v3 0/6] include: Auto-generate the sizes lookup table
Message-Id: <20190110191901.5082-1-eblake@redhat.com>
Markus Armbruster (1):
block: Eliminate the S_1KiB, S_2KiB, ... macros
block/qcow2.h | 10 +++---
block/vdi.c | 3 +-
include/qemu/units.h | 73 --------------------------------------------
3 files changed, 7 insertions(+), 79 deletions(-)
--
2.17.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-11 19:14 [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros Markus Armbruster
@ 2019-01-11 19:14 ` Markus Armbruster
2019-01-11 19:28 ` Eric Blake
` (2 more replies)
2019-01-13 9:42 ` [Qemu-devel] [PATCH 0/1] " Leonid Bloch
2019-01-25 10:46 ` Kevin Wolf
2 siblings, 3 replies; 10+ messages in thread
From: Markus Armbruster @ 2019-01-11 19:14 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, eblake, lbloch, qemu-block
We define 54 macros for the powers of two >= 1024. We use six, in six
macro definitions. Four of them could just as well use the common MiB
macro, so do that. The remaining two can't, because they get passed
to stringify. Replace the macro by the literal number there.
Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
has been good enough for more than seven years there.
This effectively reverts commit 540b8492618 and 1240ac558d3.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/qcow2.h | 10 +++---
block/vdi.c | 3 +-
include/qemu/units.h | 73 --------------------------------------------
3 files changed, 7 insertions(+), 79 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index a98d24500b..2380cbfb41 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -50,11 +50,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 S_8MiB
+#define QCOW_MAX_REFTABLE_SIZE (8 * MiB)
/* 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 S_32MiB
+#define QCOW_MAX_L1_SIZE (32 * MiB)
/* Allow for an average of 1k per snapshot table entry, should be plenty of
* space for snapshot names and IDs */
@@ -81,15 +81,15 @@
#define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
#ifdef CONFIG_LINUX
-#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB
+#define DEFAULT_L2_CACHE_MAX_SIZE (32 * MiB)
#define DEFAULT_CACHE_CLEAN_INTERVAL 600 /* seconds */
#else
-#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB
+#define DEFAULT_L2_CACHE_MAX_SIZE (8 * MiB)
/* Cache clean interval is currently available only on Linux, so must be 0 */
#define DEFAULT_CACHE_CLEAN_INTERVAL 0
#endif
-#define DEFAULT_CLUSTER_SIZE S_64KiB
+#define DEFAULT_CLUSTER_SIZE 65536
#define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
#define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
diff --git a/block/vdi.c b/block/vdi.c
index 2380daa583..bf1d19dd68 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -85,7 +85,8 @@
#define BLOCK_OPT_STATIC "static"
#define SECTOR_SIZE 512
-#define DEFAULT_CLUSTER_SIZE S_1MiB
+#define DEFAULT_CLUSTER_SIZE 1048576
+/* Note: can't use 1 * MiB, because it's passed to stringify() */
#if defined(CONFIG_VDI_DEBUG)
#define VDI_DEBUG 1
diff --git a/include/qemu/units.h b/include/qemu/units.h
index 1c959d182e..692db3fbb2 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,77 +17,4 @@
#define PiB (INT64_C(1) << 50)
#define EiB (INT64_C(1) << 60)
-/*
- * The following lookup table is intended to be used when a literal string of
- * the number of bytes is required (for example if it needs to be stringified).
- * It can also be used for generic shortcuts of power-of-two sizes.
- * This table is generated using the AWK script below:
- *
- * BEGIN {
- * suffix="KMGTPE";
- * for(i=10; i<64; i++) {
- * val=2**i;
- * s=substr(suffix, int(i/10), 1);
- * n=2**(i%10);
- * pad=21-int(log(n)/log(10));
- * printf("#define S_%d%siB %*d\n", n, s, pad, val);
- * }
- * }
- */
-
-#define S_1KiB 1024
-#define S_2KiB 2048
-#define S_4KiB 4096
-#define S_8KiB 8192
-#define S_16KiB 16384
-#define S_32KiB 32768
-#define S_64KiB 65536
-#define S_128KiB 131072
-#define S_256KiB 262144
-#define S_512KiB 524288
-#define S_1MiB 1048576
-#define S_2MiB 2097152
-#define S_4MiB 4194304
-#define S_8MiB 8388608
-#define S_16MiB 16777216
-#define S_32MiB 33554432
-#define S_64MiB 67108864
-#define S_128MiB 134217728
-#define S_256MiB 268435456
-#define S_512MiB 536870912
-#define S_1GiB 1073741824
-#define S_2GiB 2147483648
-#define S_4GiB 4294967296
-#define S_8GiB 8589934592
-#define S_16GiB 17179869184
-#define S_32GiB 34359738368
-#define S_64GiB 68719476736
-#define S_128GiB 137438953472
-#define S_256GiB 274877906944
-#define S_512GiB 549755813888
-#define S_1TiB 1099511627776
-#define S_2TiB 2199023255552
-#define S_4TiB 4398046511104
-#define S_8TiB 8796093022208
-#define S_16TiB 17592186044416
-#define S_32TiB 35184372088832
-#define S_64TiB 70368744177664
-#define S_128TiB 140737488355328
-#define S_256TiB 281474976710656
-#define S_512TiB 562949953421312
-#define S_1PiB 1125899906842624
-#define S_2PiB 2251799813685248
-#define S_4PiB 4503599627370496
-#define S_8PiB 9007199254740992
-#define S_16PiB 18014398509481984
-#define S_32PiB 36028797018963968
-#define S_64PiB 72057594037927936
-#define S_128PiB 144115188075855872
-#define S_256PiB 288230376151711744
-#define S_512PiB 576460752303423488
-#define S_1EiB 1152921504606846976
-#define S_2EiB 2305843009213693952
-#define S_4EiB 4611686018427387904
-#define S_8EiB 9223372036854775808
-
#endif
--
2.17.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
@ 2019-01-11 19:28 ` Eric Blake
2019-01-13 9:41 ` Leonid Bloch
2019-01-16 12:57 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2019-01-11 19:28 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, lbloch, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]
On 1/11/19 1:14 PM, Markus Armbruster wrote:
> We define 54 macros for the powers of two >= 1024. We use six, in six
> macro definitions. Four of them could just as well use the common MiB
> macro, so do that. The remaining two can't, because they get passed
> to stringify. Replace the macro by the literal number there.
> Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
> comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
> has been good enough for more than seven years there.
>
> This effectively reverts commit 540b8492618 and 1240ac558d3.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/qcow2.h | 10 +++---
> block/vdi.c | 3 +-
> include/qemu/units.h | 73 --------------------------------------------
> 3 files changed, 7 insertions(+), 79 deletions(-)
Renders part of my v3 series useless (since I effectively did the same
reversions), but is indeed the simplest baseline that can possibly work.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
2019-01-11 19:28 ` Eric Blake
@ 2019-01-13 9:41 ` Leonid Bloch
2019-01-14 10:36 ` Philippe Mathieu-Daudé
2019-01-16 12:57 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2 siblings, 1 reply; 10+ messages in thread
From: Leonid Bloch @ 2019-01-13 9:41 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, eblake, qemu-block
Hi,
On 1/11/19 9:14 PM, Markus Armbruster wrote:
> We define 54 macros for the powers of two >= 1024. We use six, in six
> macro definitions. Four of them could just as well use the common MiB
> macro, so do that. The remaining two can't, because they get passed
> to stringify. Replace the macro by the literal number there.
> Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
> comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
> has been good enough for more than seven years there.
>
> This effectively reverts commit 540b8492618 and 1240ac558d3.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/qcow2.h | 10 +++---
> block/vdi.c | 3 +-
> include/qemu/units.h | 73 --------------------------------------------
> 3 files changed, 7 insertions(+), 79 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a98d24500b..2380cbfb41 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -50,11 +50,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 S_8MiB
> +#define QCOW_MAX_REFTABLE_SIZE (8 * MiB)
>
> /* 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 S_32MiB
> +#define QCOW_MAX_L1_SIZE (32 * MiB)
>
> /* Allow for an average of 1k per snapshot table entry, should be plenty of
> * space for snapshot names and IDs */
> @@ -81,15 +81,15 @@
> #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
>
> #ifdef CONFIG_LINUX
> -#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB
> +#define DEFAULT_L2_CACHE_MAX_SIZE (32 * MiB)
> #define DEFAULT_CACHE_CLEAN_INTERVAL 600 /* seconds */
> #else
> -#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB
> +#define DEFAULT_L2_CACHE_MAX_SIZE (8 * MiB)
> /* Cache clean interval is currently available only on Linux, so must be 0 */
> #define DEFAULT_CACHE_CLEAN_INTERVAL 0
> #endif
>
> -#define DEFAULT_CLUSTER_SIZE S_64KiB
> +#define DEFAULT_CLUSTER_SIZE 65536
/* Note: can't use 64 * KiB here, because it's passed to stringify() */
Otherwise fine with me. The other solutions (including mine) indeed seem
overengineered compared to this.
Leonid.
>
> #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
> #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
> diff --git a/block/vdi.c b/block/vdi.c
> index 2380daa583..bf1d19dd68 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -85,7 +85,8 @@
> #define BLOCK_OPT_STATIC "static"
>
> #define SECTOR_SIZE 512
> -#define DEFAULT_CLUSTER_SIZE S_1MiB
> +#define DEFAULT_CLUSTER_SIZE 1048576
> +/* Note: can't use 1 * MiB, because it's passed to stringify() */
>
> #if defined(CONFIG_VDI_DEBUG)
> #define VDI_DEBUG 1
> diff --git a/include/qemu/units.h b/include/qemu/units.h
> index 1c959d182e..692db3fbb2 100644
> --- a/include/qemu/units.h
> +++ b/include/qemu/units.h
> @@ -17,77 +17,4 @@
> #define PiB (INT64_C(1) << 50)
> #define EiB (INT64_C(1) << 60)
>
> -/*
> - * The following lookup table is intended to be used when a literal string of
> - * the number of bytes is required (for example if it needs to be stringified).
> - * It can also be used for generic shortcuts of power-of-two sizes.
> - * This table is generated using the AWK script below:
> - *
> - * BEGIN {
> - * suffix="KMGTPE";
> - * for(i=10; i<64; i++) {
> - * val=2**i;
> - * s=substr(suffix, int(i/10), 1);
> - * n=2**(i%10);
> - * pad=21-int(log(n)/log(10));
> - * printf("#define S_%d%siB %*d\n", n, s, pad, val);
> - * }
> - * }
> - */
> -
> -#define S_1KiB 1024
> -#define S_2KiB 2048
> -#define S_4KiB 4096
> -#define S_8KiB 8192
> -#define S_16KiB 16384
> -#define S_32KiB 32768
> -#define S_64KiB 65536
> -#define S_128KiB 131072
> -#define S_256KiB 262144
> -#define S_512KiB 524288
> -#define S_1MiB 1048576
> -#define S_2MiB 2097152
> -#define S_4MiB 4194304
> -#define S_8MiB 8388608
> -#define S_16MiB 16777216
> -#define S_32MiB 33554432
> -#define S_64MiB 67108864
> -#define S_128MiB 134217728
> -#define S_256MiB 268435456
> -#define S_512MiB 536870912
> -#define S_1GiB 1073741824
> -#define S_2GiB 2147483648
> -#define S_4GiB 4294967296
> -#define S_8GiB 8589934592
> -#define S_16GiB 17179869184
> -#define S_32GiB 34359738368
> -#define S_64GiB 68719476736
> -#define S_128GiB 137438953472
> -#define S_256GiB 274877906944
> -#define S_512GiB 549755813888
> -#define S_1TiB 1099511627776
> -#define S_2TiB 2199023255552
> -#define S_4TiB 4398046511104
> -#define S_8TiB 8796093022208
> -#define S_16TiB 17592186044416
> -#define S_32TiB 35184372088832
> -#define S_64TiB 70368744177664
> -#define S_128TiB 140737488355328
> -#define S_256TiB 281474976710656
> -#define S_512TiB 562949953421312
> -#define S_1PiB 1125899906842624
> -#define S_2PiB 2251799813685248
> -#define S_4PiB 4503599627370496
> -#define S_8PiB 9007199254740992
> -#define S_16PiB 18014398509481984
> -#define S_32PiB 36028797018963968
> -#define S_64PiB 72057594037927936
> -#define S_128PiB 144115188075855872
> -#define S_256PiB 288230376151711744
> -#define S_512PiB 576460752303423488
> -#define S_1EiB 1152921504606846976
> -#define S_2EiB 2305843009213693952
> -#define S_4EiB 4611686018427387904
> -#define S_8EiB 9223372036854775808
> -
> #endif
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-11 19:14 [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros Markus Armbruster
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
@ 2019-01-13 9:42 ` Leonid Bloch
2019-01-14 6:54 ` Markus Armbruster
2019-01-25 10:46 ` Kevin Wolf
2 siblings, 1 reply; 10+ messages in thread
From: Leonid Bloch @ 2019-01-13 9:42 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, eblake, qemu-block
On 1/11/19 9:14 PM, Markus Armbruster wrote:
> Back in September, Leonid Block added a whole bunch of macros (commit
* Bloch. :)
> 540b8492618) to improve readability of qcow2.h a bit (commit
> b6a95c6d100). He later used them to fix the "vdi" driver's parameter
> cluster_size's default value (commit 3dd5b8f4718). He has now
> proposed a further patch[1] to auto-generate these macros. That patch
> feels overengineered to me.
>
> On closer examination, I found I dislike the macros before his new
> patch. So did Eric Blake.
>
> The macros exist because the common KiB, MiB, ... macros aren't usable
> when you need a literal rather than a constant expression.
> stringify() does, and we use it to define the QemuOpts default value.
>
> Eric proposed to improve QemuOpts to accept integer default values,
> too[2]. Before I review that patch series, I want to establish a
> "stupidest solution that can possibly work" baseline. And that's what
> this patch is.
>
> [1] [PATCH v2 0/1] include: Auto-generate the sizes lookup table
> Message-ID: <20190103213320.2653-1-lbloch@janustech.com>
>
> [2] [PATCH v3 0/6] include: Auto-generate the sizes lookup table
> Message-Id: <20190110191901.5082-1-eblake@redhat.com>
>
> Markus Armbruster (1):
> block: Eliminate the S_1KiB, S_2KiB, ... macros
>
> block/qcow2.h | 10 +++---
> block/vdi.c | 3 +-
> include/qemu/units.h | 73 --------------------------------------------
> 3 files changed, 7 insertions(+), 79 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-13 9:42 ` [Qemu-devel] [PATCH 0/1] " Leonid Bloch
@ 2019-01-14 6:54 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2019-01-14 6:54 UTC (permalink / raw)
To: Leonid Bloch; +Cc: qemu-devel, kwolf, qemu-block
Leonid Bloch <lbloch@janustech.com> writes:
> On 1/11/19 9:14 PM, Markus Armbruster wrote:
>> Back in September, Leonid Block added a whole bunch of macros (commit
>
> * Bloch. :)
I apologize for my carelessness. Explanation, no excuse:
$ git-log --author=armbru -Sblock -i --oneline | wc -l
167
$ git-log --author=armbru -Sbloch -i --oneline | wc -l
0
Déformation professionnelle...
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-13 9:41 ` Leonid Bloch
@ 2019-01-14 10:36 ` Philippe Mathieu-Daudé
2019-01-14 13:01 ` Markus Armbruster
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-01-14 10:36 UTC (permalink / raw)
To: Leonid Bloch, Markus Armbruster, qemu-devel; +Cc: kwolf, qemu-block
On 1/13/19 10:41 AM, Leonid Bloch wrote:
> On 1/11/19 9:14 PM, Markus Armbruster wrote:
>> We define 54 macros for the powers of two >= 1024. We use six, in six
>> macro definitions. Four of them could just as well use the common MiB
>> macro, so do that. The remaining two can't, because they get passed
>> to stringify. Replace the macro by the literal number there.
>> Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
>> comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
>> has been good enough for more than seven years there.
>>
>> This effectively reverts commit 540b8492618 and 1240ac558d3.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block/qcow2.h | 10 +++---
>> block/vdi.c | 3 +-
>> include/qemu/units.h | 73 --------------------------------------------
>> 3 files changed, 7 insertions(+), 79 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index a98d24500b..2380cbfb41 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -50,11 +50,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 S_8MiB
>> +#define QCOW_MAX_REFTABLE_SIZE (8 * MiB)
>>
>> /* 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 S_32MiB
>> +#define QCOW_MAX_L1_SIZE (32 * MiB)
>>
>> /* Allow for an average of 1k per snapshot table entry, should be plenty of
>> * space for snapshot names and IDs */
>> @@ -81,15 +81,15 @@
>> #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
>>
>> #ifdef CONFIG_LINUX
>> -#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB
>> +#define DEFAULT_L2_CACHE_MAX_SIZE (32 * MiB)
>> #define DEFAULT_CACHE_CLEAN_INTERVAL 600 /* seconds */
>> #else
>> -#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB
>> +#define DEFAULT_L2_CACHE_MAX_SIZE (8 * MiB)
>> /* Cache clean interval is currently available only on Linux, so must be 0 */
>> #define DEFAULT_CACHE_CLEAN_INTERVAL 0
>> #endif
>>
>> -#define DEFAULT_CLUSTER_SIZE S_64KiB
>> +#define DEFAULT_CLUSTER_SIZE 65536
>
> /* Note: can't use 64 * KiB here, because it's passed to stringify() */
Good idea.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Otherwise fine with me. The other solutions (including mine) indeed seem
> overengineered compared to this.
>
> Leonid.
>
>>
>> #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
>> #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 2380daa583..bf1d19dd68 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -85,7 +85,8 @@
>> #define BLOCK_OPT_STATIC "static"
>>
>> #define SECTOR_SIZE 512
>> -#define DEFAULT_CLUSTER_SIZE S_1MiB
>> +#define DEFAULT_CLUSTER_SIZE 1048576
>> +/* Note: can't use 1 * MiB, because it's passed to stringify() */
>>
>> #if defined(CONFIG_VDI_DEBUG)
>> #define VDI_DEBUG 1
>> diff --git a/include/qemu/units.h b/include/qemu/units.h
>> index 1c959d182e..692db3fbb2 100644
>> --- a/include/qemu/units.h
>> +++ b/include/qemu/units.h
>> @@ -17,77 +17,4 @@
>> #define PiB (INT64_C(1) << 50)
>> #define EiB (INT64_C(1) << 60)
>>
>> -/*
>> - * The following lookup table is intended to be used when a literal string of
>> - * the number of bytes is required (for example if it needs to be stringified).
>> - * It can also be used for generic shortcuts of power-of-two sizes.
>> - * This table is generated using the AWK script below:
>> - *
>> - * BEGIN {
>> - * suffix="KMGTPE";
>> - * for(i=10; i<64; i++) {
>> - * val=2**i;
>> - * s=substr(suffix, int(i/10), 1);
>> - * n=2**(i%10);
>> - * pad=21-int(log(n)/log(10));
>> - * printf("#define S_%d%siB %*d\n", n, s, pad, val);
>> - * }
>> - * }
>> - */
>> -
>> -#define S_1KiB 1024
>> -#define S_2KiB 2048
>> -#define S_4KiB 4096
>> -#define S_8KiB 8192
>> -#define S_16KiB 16384
>> -#define S_32KiB 32768
>> -#define S_64KiB 65536
>> -#define S_128KiB 131072
>> -#define S_256KiB 262144
>> -#define S_512KiB 524288
>> -#define S_1MiB 1048576
>> -#define S_2MiB 2097152
>> -#define S_4MiB 4194304
>> -#define S_8MiB 8388608
>> -#define S_16MiB 16777216
>> -#define S_32MiB 33554432
>> -#define S_64MiB 67108864
>> -#define S_128MiB 134217728
>> -#define S_256MiB 268435456
>> -#define S_512MiB 536870912
>> -#define S_1GiB 1073741824
>> -#define S_2GiB 2147483648
>> -#define S_4GiB 4294967296
>> -#define S_8GiB 8589934592
>> -#define S_16GiB 17179869184
>> -#define S_32GiB 34359738368
>> -#define S_64GiB 68719476736
>> -#define S_128GiB 137438953472
>> -#define S_256GiB 274877906944
>> -#define S_512GiB 549755813888
>> -#define S_1TiB 1099511627776
>> -#define S_2TiB 2199023255552
>> -#define S_4TiB 4398046511104
>> -#define S_8TiB 8796093022208
>> -#define S_16TiB 17592186044416
>> -#define S_32TiB 35184372088832
>> -#define S_64TiB 70368744177664
>> -#define S_128TiB 140737488355328
>> -#define S_256TiB 281474976710656
>> -#define S_512TiB 562949953421312
>> -#define S_1PiB 1125899906842624
>> -#define S_2PiB 2251799813685248
>> -#define S_4PiB 4503599627370496
>> -#define S_8PiB 9007199254740992
>> -#define S_16PiB 18014398509481984
>> -#define S_32PiB 36028797018963968
>> -#define S_64PiB 72057594037927936
>> -#define S_128PiB 144115188075855872
>> -#define S_256PiB 288230376151711744
>> -#define S_512PiB 576460752303423488
>> -#define S_1EiB 1152921504606846976
>> -#define S_2EiB 2305843009213693952
>> -#define S_4EiB 4611686018427387904
>> -#define S_8EiB 9223372036854775808
>> -
>> #endif
>>
Thanks Markus, Eric and Leonid for this cleanup!
I'm sorry all of you wasted so many ressources here.
Phil.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-14 10:36 ` Philippe Mathieu-Daudé
@ 2019-01-14 13:01 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2019-01-14 13:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Leonid Bloch, qemu-devel, kwolf, qemu-block
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> On 1/13/19 10:41 AM, Leonid Bloch wrote:
>> On 1/11/19 9:14 PM, Markus Armbruster wrote:
>>> We define 54 macros for the powers of two >= 1024. We use six, in six
>>> macro definitions. Four of them could just as well use the common MiB
>>> macro, so do that. The remaining two can't, because they get passed
>>> to stringify. Replace the macro by the literal number there.
>>> Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
>>> comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
>>> has been good enough for more than seven years there.
>>>
>>> This effectively reverts commit 540b8492618 and 1240ac558d3.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> block/qcow2.h | 10 +++---
>>> block/vdi.c | 3 +-
>>> include/qemu/units.h | 73 --------------------------------------------
>>> 3 files changed, 7 insertions(+), 79 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index a98d24500b..2380cbfb41 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -50,11 +50,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 S_8MiB
>>> +#define QCOW_MAX_REFTABLE_SIZE (8 * MiB)
>>>
>>> /* 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 S_32MiB
>>> +#define QCOW_MAX_L1_SIZE (32 * MiB)
>>>
>>> /* Allow for an average of 1k per snapshot table entry, should be plenty of
>>> * space for snapshot names and IDs */
>>> @@ -81,15 +81,15 @@
>>> #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
>>>
>>> #ifdef CONFIG_LINUX
>>> -#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB
>>> +#define DEFAULT_L2_CACHE_MAX_SIZE (32 * MiB)
>>> #define DEFAULT_CACHE_CLEAN_INTERVAL 600 /* seconds */
>>> #else
>>> -#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB
>>> +#define DEFAULT_L2_CACHE_MAX_SIZE (8 * MiB)
>>> /* Cache clean interval is currently available only on Linux, so must be 0 */
>>> #define DEFAULT_CACHE_CLEAN_INTERVAL 0
>>> #endif
>>>
>>> -#define DEFAULT_CLUSTER_SIZE S_64KiB
>>> +#define DEFAULT_CLUSTER_SIZE 65536
>>
>> /* Note: can't use 64 * KiB here, because it's passed to stringify() */
>
> Good idea.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
I omitted the comment here, because I find 64 * KiB hardly more readable
than 65536. We've done just fine with 65536 for more than seven years.
But if you guys want the comment, you can certainly have it.
>> Otherwise fine with me. The other solutions (including mine) indeed seem
>> overengineered compared to this.
>>
>> Leonid.
>>
>>>
>>> #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
>>> #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
>>> diff --git a/block/vdi.c b/block/vdi.c
>>> index 2380daa583..bf1d19dd68 100644
>>> --- a/block/vdi.c
>>> +++ b/block/vdi.c
>>> @@ -85,7 +85,8 @@
>>> #define BLOCK_OPT_STATIC "static"
>>>
>>> #define SECTOR_SIZE 512
>>> -#define DEFAULT_CLUSTER_SIZE S_1MiB
>>> +#define DEFAULT_CLUSTER_SIZE 1048576
>>> +/* Note: can't use 1 * MiB, because it's passed to stringify() */
>>>
>>> #if defined(CONFIG_VDI_DEBUG)
>>> #define VDI_DEBUG 1
>>> diff --git a/include/qemu/units.h b/include/qemu/units.h
>>> index 1c959d182e..692db3fbb2 100644
>>> --- a/include/qemu/units.h
>>> +++ b/include/qemu/units.h
>>> @@ -17,77 +17,4 @@
>>> #define PiB (INT64_C(1) << 50)
>>> #define EiB (INT64_C(1) << 60)
>>>
>>> -/*
>>> - * The following lookup table is intended to be used when a literal string of
>>> - * the number of bytes is required (for example if it needs to be stringified).
>>> - * It can also be used for generic shortcuts of power-of-two sizes.
>>> - * This table is generated using the AWK script below:
>>> - *
>>> - * BEGIN {
>>> - * suffix="KMGTPE";
>>> - * for(i=10; i<64; i++) {
>>> - * val=2**i;
>>> - * s=substr(suffix, int(i/10), 1);
>>> - * n=2**(i%10);
>>> - * pad=21-int(log(n)/log(10));
>>> - * printf("#define S_%d%siB %*d\n", n, s, pad, val);
>>> - * }
>>> - * }
>>> - */
>>> -
>>> -#define S_1KiB 1024
>>> -#define S_2KiB 2048
>>> -#define S_4KiB 4096
>>> -#define S_8KiB 8192
>>> -#define S_16KiB 16384
>>> -#define S_32KiB 32768
>>> -#define S_64KiB 65536
>>> -#define S_128KiB 131072
>>> -#define S_256KiB 262144
>>> -#define S_512KiB 524288
>>> -#define S_1MiB 1048576
>>> -#define S_2MiB 2097152
>>> -#define S_4MiB 4194304
>>> -#define S_8MiB 8388608
>>> -#define S_16MiB 16777216
>>> -#define S_32MiB 33554432
>>> -#define S_64MiB 67108864
>>> -#define S_128MiB 134217728
>>> -#define S_256MiB 268435456
>>> -#define S_512MiB 536870912
>>> -#define S_1GiB 1073741824
>>> -#define S_2GiB 2147483648
>>> -#define S_4GiB 4294967296
>>> -#define S_8GiB 8589934592
>>> -#define S_16GiB 17179869184
>>> -#define S_32GiB 34359738368
>>> -#define S_64GiB 68719476736
>>> -#define S_128GiB 137438953472
>>> -#define S_256GiB 274877906944
>>> -#define S_512GiB 549755813888
>>> -#define S_1TiB 1099511627776
>>> -#define S_2TiB 2199023255552
>>> -#define S_4TiB 4398046511104
>>> -#define S_8TiB 8796093022208
>>> -#define S_16TiB 17592186044416
>>> -#define S_32TiB 35184372088832
>>> -#define S_64TiB 70368744177664
>>> -#define S_128TiB 140737488355328
>>> -#define S_256TiB 281474976710656
>>> -#define S_512TiB 562949953421312
>>> -#define S_1PiB 1125899906842624
>>> -#define S_2PiB 2251799813685248
>>> -#define S_4PiB 4503599627370496
>>> -#define S_8PiB 9007199254740992
>>> -#define S_16PiB 18014398509481984
>>> -#define S_32PiB 36028797018963968
>>> -#define S_64PiB 72057594037927936
>>> -#define S_128PiB 144115188075855872
>>> -#define S_256PiB 288230376151711744
>>> -#define S_512PiB 576460752303423488
>>> -#define S_1EiB 1152921504606846976
>>> -#define S_2EiB 2305843009213693952
>>> -#define S_4EiB 4611686018427387904
>>> -#define S_8EiB 9223372036854775808
>>> -
>>> #endif
>>>
>
> Thanks Markus, Eric and Leonid for this cleanup!
>
> I'm sorry all of you wasted so many ressources here.
It happens :)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 1/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
2019-01-11 19:28 ` Eric Blake
2019-01-13 9:41 ` Leonid Bloch
@ 2019-01-16 12:57 ` Alberto Garcia
2 siblings, 0 replies; 10+ messages in thread
From: Alberto Garcia @ 2019-01-16 12:57 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: kwolf, lbloch, qemu-block
On Fri 11 Jan 2019 08:14:01 PM CET, Markus Armbruster wrote:
> We define 54 macros for the powers of two >= 1024. We use six, in six
> macro definitions. Four of them could just as well use the common MiB
> macro, so do that. The remaining two can't, because they get passed
> to stringify. Replace the macro by the literal number there.
> Slightly harder to read in one instance (1048576 vs. S_1MiB), so add a
> comment there. The other instance is a wash: 65536 vs S_64KiB. 65536
> has been good enough for more than seven years there.
>
> This effectively reverts commit 540b8492618 and 1240ac558d3.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros
2019-01-11 19:14 [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros Markus Armbruster
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
2019-01-13 9:42 ` [Qemu-devel] [PATCH 0/1] " Leonid Bloch
@ 2019-01-25 10:46 ` Kevin Wolf
2 siblings, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2019-01-25 10:46 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, eblake, lbloch, qemu-block
Am 11.01.2019 um 20:14 hat Markus Armbruster geschrieben:
> Back in September, Leonid Block added a whole bunch of macros (commit
> 540b8492618) to improve readability of qcow2.h a bit (commit
> b6a95c6d100). He later used them to fix the "vdi" driver's parameter
> cluster_size's default value (commit 3dd5b8f4718). He has now
> proposed a further patch[1] to auto-generate these macros. That patch
> feels overengineered to me.
>
> On closer examination, I found I dislike the macros before his new
> patch. So did Eric Blake.
>
> The macros exist because the common KiB, MiB, ... macros aren't usable
> when you need a literal rather than a constant expression.
> stringify() does, and we use it to define the QemuOpts default value.
>
> Eric proposed to improve QemuOpts to accept integer default values,
> too[2]. Before I review that patch series, I want to establish a
> "stupidest solution that can possibly work" baseline. And that's what
> this patch is.
>
> [1] [PATCH v2 0/1] include: Auto-generate the sizes lookup table
> Message-ID: <20190103213320.2653-1-lbloch@janustech.com>
>
> [2] [PATCH v3 0/6] include: Auto-generate the sizes lookup table
> Message-Id: <20190110191901.5082-1-eblake@redhat.com>
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-25 10:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 19:14 [Qemu-devel] [PATCH 0/1] block: Eliminate the S_1KiB, S_2KiB, ... macros Markus Armbruster
2019-01-11 19:14 ` [Qemu-devel] [PATCH 1/1] " Markus Armbruster
2019-01-11 19:28 ` Eric Blake
2019-01-13 9:41 ` Leonid Bloch
2019-01-14 10:36 ` Philippe Mathieu-Daudé
2019-01-14 13:01 ` Markus Armbruster
2019-01-16 12:57 ` [Qemu-devel] [Qemu-block] " Alberto Garcia
2019-01-13 9:42 ` [Qemu-devel] [PATCH 0/1] " Leonid Bloch
2019-01-14 6:54 ` Markus Armbruster
2019-01-25 10:46 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.