[PATCH] virnetdevbandwidth.c: Put a limit to "quantum"

Michal Privoznik posted 1 patch 1 week, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/dc5cc8f30836ed004aa50ddaf36f2e405eccb0ad.1714026897.git.mprivozn@redhat.com
src/util/virnetdevbandwidth.c | 6 ++++++
1 file changed, 6 insertions(+)
[PATCH] virnetdevbandwidth.c: Put a limit to "quantum"
Posted by Michal Privoznik 1 week, 1 day ago
The "quantum" attribute of HTB is documented as:

  Number of bytes to serve from this class before the scheduler
  moves to the next class.

Since v1.3.2-rc1~225 we compute what we think is the appropriate
value and pass it on the TC command line. But kernel and
subsequently TC use uint32_t to store this value. If we compute
value outside of this type then TC fails and prints usage which
we then interpret as an error message. Needlessly long error
message. While there's not much we can do about the latter, we
can put a cap on the value and stop tickling this behavior of TC.

Fixes: 065054daa71f645fc83aff0271f194d326208616
Resolves: https://issues.redhat.com/browse/RHEL-34112
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virnetdevbandwidth.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index ec41666f67..7f5714a33f 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -46,6 +46,7 @@ virNetDevBandwidthCmdAddOptimalQuantum(virCommand *cmd,
                                        const virNetDevBandwidthRate *rate)
 {
     const unsigned long long mtu = 1500;
+    const unsigned long long r2q_limit = (1ULL << 32) -1;
     unsigned long long r2q;
 
     /* When two or more classes compete for unused bandwidth they are each
@@ -60,6 +61,11 @@ virNetDevBandwidthCmdAddOptimalQuantum(virCommand *cmd,
     if (!r2q)
         r2q = 1;
 
+    /* But there's an internal limit in TC (well, kernel's implementation of
+     * HTB) for quantum: it has to fit into u32. Put a cap there. */
+    if (r2q > r2q_limit)
+        r2q = r2q_limit;
+
     virCommandAddArg(cmd, "quantum");
     virCommandAddArgFormat(cmd, "%llu", r2q);
 }
-- 
2.43.2
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] virnetdevbandwidth.c: Put a limit to "quantum"
Posted by Peter Krempa 1 week, 1 day ago
On Thu, Apr 25, 2024 at 08:34:57 +0200, Michal Privoznik wrote:
> The "quantum" attribute of HTB is documented as:
> 
>   Number of bytes to serve from this class before the scheduler
>   moves to the next class.
> 
> Since v1.3.2-rc1~225 we compute what we think is the appropriate
> value and pass it on the TC command line. But kernel and
> subsequently TC use uint32_t to store this value. If we compute
> value outside of this type then TC fails and prints usage which
> we then interpret as an error message. Needlessly long error
> message. While there's not much we can do about the latter, we
> can put a cap on the value and stop tickling this behavior of TC.
> 
> Fixes: 065054daa71f645fc83aff0271f194d326208616
> Resolves: https://issues.redhat.com/browse/RHEL-34112
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virnetdevbandwidth.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
> index ec41666f67..7f5714a33f 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c
> @@ -46,6 +46,7 @@ virNetDevBandwidthCmdAddOptimalQuantum(virCommand *cmd,
>                                         const virNetDevBandwidthRate *rate)
>  {
>      const unsigned long long mtu = 1500;
> +    const unsigned long long r2q_limit = (1ULL << 32) -1;

This quacks like  'UINT32_MAX'

>      unsigned long long r2q;
>  
>      /* When two or more classes compete for unused bandwidth they are each
> @@ -60,6 +61,11 @@ virNetDevBandwidthCmdAddOptimalQuantum(virCommand *cmd,
>      if (!r2q)
>          r2q = 1;
>  
> +    /* But there's an internal limit in TC (well, kernel's implementation of
> +     * HTB) for quantum: it has to fit into u32. Put a cap there. */
> +    if (r2q > r2q_limit)
> +        r2q = r2q_limit;

And you could use the constant directly here, which would also address
the broken spacing around subtraction in the variable you've added.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org