[PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time

[PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time

[Richard Henderson]

Drop inline marker: let compiler decide.

Change return type to uint64_t: this matches the computation in the
return statement and the local variable assignment in the caller.

Rename local to dirty_ring_size_MB to fix typo.
Simplify conversion to MiB via qemu_target_page_bits and right shift.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/dirtylimit.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 82986c1499..71bf6dc7a4 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -232,18 +232,23 @@ bool dirtylimit_vcpu_index_valid(int cpu_index)
              cpu_index >= ms->smp.max_cpus);
 }
 
-static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
+static uint64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
 {
     static uint64_t max_dirtyrate;
-    uint32_t dirty_ring_size = kvm_dirty_ring_size();
-    uint64_t dirty_ring_size_meory_MB =
-        dirty_ring_size * qemu_target_page_size() >> 20;
+    unsigned target_page_bits = qemu_target_page_bits();
+    uint64_t dirty_ring_size_MB;
+
+    /* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
+    assert(target_page_bits < 20);
+
+    /* Convert ring size (pages) to MiB (2**20). */
+    dirty_ring_size_MB = kvm_dirty_ring_size() >> (20 - target_page_bits);
 
     if (max_dirtyrate < dirtyrate) {
         max_dirtyrate = dirtyrate;
     }
 
-    return dirty_ring_size_meory_MB * 1000000 / max_dirtyrate;
+    return dirty_ring_size_MB * 1000000 / max_dirtyrate;
 }
 
 static inline bool dirtylimit_done(uint64_t quota,
-- 
2.34.1

Re: [PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time

[Thomas Huth]

On 28/04/2023 12.34, Richard Henderson wrote:
> Drop inline marker: let compiler decide.
> 
> Change return type to uint64_t: this matches the computation in the
> return statement and the local variable assignment in the caller.
> 
> Rename local to dirty_ring_size_MB to fix typo.
> Simplify conversion to MiB via qemu_target_page_bits and right shift.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   softmmu/dirtylimit.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
> index 82986c1499..71bf6dc7a4 100644
> --- a/softmmu/dirtylimit.c
> +++ b/softmmu/dirtylimit.c
> @@ -232,18 +232,23 @@ bool dirtylimit_vcpu_index_valid(int cpu_index)
>                cpu_index >= ms->smp.max_cpus);
>   }
>   
> -static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
> +static uint64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
>   {
>       static uint64_t max_dirtyrate;
> -    uint32_t dirty_ring_size = kvm_dirty_ring_size();
> -    uint64_t dirty_ring_size_meory_MB =
> -        dirty_ring_size * qemu_target_page_size() >> 20;
> +    unsigned target_page_bits = qemu_target_page_bits();
> +    uint64_t dirty_ring_size_MB;
> +
> +    /* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
> +    assert(target_page_bits < 20);
> +
> +    /* Convert ring size (pages) to MiB (2**20). */
> +    dirty_ring_size_MB = kvm_dirty_ring_size() >> (20 - target_page_bits);
>   
>       if (max_dirtyrate < dirtyrate) {
>           max_dirtyrate = dirtyrate;
>       }
>   
> -    return dirty_ring_size_meory_MB * 1000000 / max_dirtyrate;
> +    return dirty_ring_size_MB * 1000000 / max_dirtyrate;
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 28/04/2023 12.34, Richard Henderson wrote:
>> Drop inline marker: let compiler decide.
>> Change return type to uint64_t: this matches the computation in the
>> return statement and the local variable assignment in the caller.
>> Rename local to dirty_ring_size_MB to fix typo.
>> Simplify conversion to MiB via qemu_target_page_bits and right shift.
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Not that I expect the amount of bytes in one machine to not fit in
64bit, but good change.

About the signesdness of variables, I really hate it, specially that
compiler not help here where arguments, operands and assignment place is
unsigned.  Sniff.

Re: [PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time

[Richard Henderson]

On 4/28/23 19:38, Juan Quintela wrote:
> Thomas Huth <thuth@redhat.com> wrote:
>> On 28/04/2023 12.34, Richard Henderson wrote:
>>> Drop inline marker: let compiler decide.
>>> Change return type to uint64_t: this matches the computation in the
>>> return statement and the local variable assignment in the caller.
>>> Rename local to dirty_ring_size_MB to fix typo.
>>> Simplify conversion to MiB via qemu_target_page_bits and right shift.
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks,

Queueing to tcg-next.


r~

Re: [PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time

[Philippe Mathieu-Daudé]

On 28/4/23 12:34, Richard Henderson wrote:
> Drop inline marker: let compiler decide.
> 
> Change return type to uint64_t: this matches the computation in the
> return statement and the local variable assignment in the caller.
> 
> Rename local to dirty_ring_size_MB to fix typo.
> Simplify conversion to MiB via qemu_target_page_bits and right shift.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   softmmu/dirtylimit.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)


> -static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
> +static uint64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
>   {
>       static uint64_t max_dirtyrate;
> -    uint32_t dirty_ring_size = kvm_dirty_ring_size();
> -    uint64_t dirty_ring_size_meory_MB =
> -        dirty_ring_size * qemu_target_page_size() >> 20;
> +    unsigned target_page_bits = qemu_target_page_bits();
> +    uint64_t dirty_ring_size_MB;
> +
> +    /* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
> +    assert(target_page_bits < 20);
> +
> +    /* Convert ring size (pages) to MiB (2**20). */
> +    dirty_ring_size_MB = kvm_dirty_ring_size() >> (20 - target_page_bits);
>   
>       if (max_dirtyrate < dirtyrate) {
>           max_dirtyrate = dirtyrate;
>       }
>   
> -    return dirty_ring_size_meory_MB * 1000000 / max_dirtyrate;
> +    return dirty_ring_size_MB * 1000000 / max_dirtyrate;

I wondered what was this 1000000 magic number. IIUC this is
converted to microseconds. We don't have such definition.
We could use (NANOSECONDS_PER_SECOND / 1000).
Somehow simpler is to rename dirtylimit_dirty_ring_full_time()
-> dirtylimit_dirty_ring_full_time_us(). Anyway,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   }
>   
>   static inline bool dirtylimit_done(uint64_t quota,