[PATCH 0/2] virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl

*** BLURB HERE *** Michal Prívozník (2): docs: Clarify 'burst' units for QoS virnetdevopenvswitch: Fix 'burst' value passed to ovs-vsctl docs/formatnetwork.html.in | 2 +- src/util/virnetdevbandwidth.h | 8 ++++---- src/util/virnetdevopenvswitch.c | 16 +++++++++------- 3 files changed, 14 insertions(+), 12 deletions(-) -- 2.34.1

The burst attribute for bandwidth specifies how much bytes can be transmitted in a single burst. Therefore, the unit is in multiples of 1024 (thus kibibytes) not SI-like 1000. It always has been like that. The 'tc' output is still confusing though, for instance: # tc class add dev $DEV parent 1: classid 1:1 htb rate 1000kbps burst 2097152 # tc class show dev vnet2 class htb 1:1 root rate 8Mbit ceil 8Mbit burst 2Mb cburst 1600b Please note that 2097152 = 2*1024*1024. Even the man page is confusing. From tc(8): kb or k Kilobytes mb or m Megabytes But I guess this is because 'tc' predates IEC standardisation of binary multiples and thus can't change without breaking scripts parsing its output. And while at it, adjust _virNetDevBandwidthRate struct member description, to make it obvious which members use SI/IEC units. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatnetwork.html.in | 2 +- src/util/virnetdevbandwidth.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index b1b2391f43..fad43f77ea 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -634,7 +634,7 @@ </dd> <dt><code>burst</code></dt> <dd> - Optional attribute which specifies the amount of kilobytes that + Optional attribute which specifies the amount of kibibytes that can be transmitted in a single burst at <code>peak</code> speed. </dd> <dt><code>floor</code></dt> diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 3d520721f6..c82029fc0f 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -23,10 +23,10 @@ typedef struct _virNetDevBandwidthRate virNetDevBandwidthRate; struct _virNetDevBandwidthRate { - unsigned long long average; /* kbytes/s */ - unsigned long long peak; /* kbytes/s */ - unsigned long long floor; /* kbytes/s */ - unsigned long long burst; /* kbytes */ + unsigned long long average; /* kilobytes/s */ + unsigned long long peak; /* kilobytes/s */ + unsigned long long floor; /* kilobytes/s */ + unsigned long long burst; /* kibibytes */ }; typedef struct _virNetDevBandwidth virNetDevBandwidth; -- 2.34.1

On Tue, Jan 04, 2022 at 01:53:05PM +0100, Michal Privoznik wrote:
The burst attribute for bandwidth specifies how much bytes can be transmitted in a single burst. Therefore, the unit is in multiples of 1024 (thus kibibytes) not SI-like 1000. It always has been like that.
s/always has/has always/ I guess and Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

As described in the previous commit, the units for 'burst' are kibibytes and not kilobytes, i.e. multiples of 1024 not 1000. Therefore, when constructing ovs-vsctl command the burst value must be multiplied by 1024 and not just 1000. And because ovs expects this size in bits the value has to be multiplied again by 8. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1510237#c26 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevopenvswitch.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 5dab545037..8741e0ed3a 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -638,11 +638,13 @@ virNetDevOpenvswitchFindUUID(const char *table, } /* - * Average, peak, floor and burst in virNetDevBandwidth are in kbytes. - * However other_config in ovs qos is in bit. - * ingress_policing_rate in ovs interface is in kbit. + * In virNetDevBandwidthRate, average, peak, floor are in kilobyes and burst in + * kibibytes. However other_config in ovs qos is in bits and + * ingress_policing_rate in ovs interface is in kilobit for + * ingress_policing_rate and kibibit for ingress_policing_rate. */ -#define VIR_NETDEV_TX_TO_OVS 8000 +#define KBYTE_TO_BITS(x) (x * 8000ULL) +#define KIBIBYTE_TO_BITS(x) (x * 8192ULL) #define VIR_NETDEV_RX_TO_OVS 8 /** @@ -717,11 +719,11 @@ virNetDevOpenvswitchInterfaceSetQos(const char *ifname, g_autofree char *qos_uuid = NULL; g_autofree char *queue_uuid = NULL; - average = g_strdup_printf("%llu", tx->average * VIR_NETDEV_TX_TO_OVS); + average = g_strdup_printf("%llu", KBYTE_TO_BITS(tx->average)); if (tx->burst) - burst = g_strdup_printf("%llu", tx->burst * VIR_NETDEV_TX_TO_OVS); + burst = g_strdup_printf("%llu", KIBIBYTE_TO_BITS(tx->burst)); if (tx->peak) - peak = g_strdup_printf("%llu", tx->peak * VIR_NETDEV_TX_TO_OVS); + peak = g_strdup_printf("%llu", KBYTE_TO_BITS(tx->peak)); virUUIDFormat(vmuuid, vmuuidstr); vmid_ex_id = g_strdup_printf("external-ids:vm-id=\"%s\"", vmuuidstr); -- 2.34.1

On Tue, Jan 04, 2022 at 01:53:06PM +0100, Michal Privoznik wrote:
As described in the previous commit, the units for 'burst' are kibibytes and not kilobytes, i.e. multiples of 1024 not 1000. Therefore, when constructing ovs-vsctl command the burst value must be multiplied by 1024 and not just 1000. And because ovs expects this size in bits the value has to be multiplied again by 8.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1510237#c26 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevopenvswitch.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 5dab545037..8741e0ed3a 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -638,11 +638,13 @@ virNetDevOpenvswitchFindUUID(const char *table, }
/* - * Average, peak, floor and burst in virNetDevBandwidth are in kbytes. - * However other_config in ovs qos is in bit. - * ingress_policing_rate in ovs interface is in kbit. + * In virNetDevBandwidthRate, average, peak, floor are in kilobyes and burst in + * kibibytes. However other_config in ovs qos is in bits and + * ingress_policing_rate in ovs interface is in kilobit for + * ingress_policing_rate and kibibit for ingress_policing_rate.
I guess you meant ingress_policing_burst at the end and the sentence could be written a bit less confusingly. With that fixed Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (2)
-
Martin Kletzander
-
Michal Privoznik