[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to

From: Juan Quintela
Subject: Re: [PATCH 10/21] migration: Move rate_limit_max and rate_limit_used to migration_stats
Date: Mon, 15 May 2023 19:16:45 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)

Cédric Le Goater <clg@kaod.org> wrote:
> On 5/15/23 15:09, Juan Quintela wrote:
>> Cédric Le Goater <clg@kaod.org> wrote:
>>> On 5/8/23 15:08, Juan Quintela wrote:
>>>> This way we can make them atomic and use this functions from any
>>>> place.  I also moved all functions that use rate_limit to
>>>> migration-stats.
>>>> Functions got renamed, they are not qemu_file anymore.
>>>> qemu_file_rate_limit -> migration_rate_limit_exceeded
>>>> qemu_file_set_rate_limit -> migration_rate_limit_set
>>>> qemu_file_get_rate_limit -> migration_rate_limit_get
>>>> qemu_file_reset_rate_limit -> migration_rate_limit_reset
>>>> qemu_file_acct_rate_limit -> migration_rate_limit_account.
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> If you have any good suggestion for better names, I am all ears.
>>> May be :
>>>   qemu_file_rate_limit -> migration_rate_limit_is_exceeded
>> I try not to put _is_ in function names.  If it needs to be there, I
>> think that I need to rename the functino.
> It is common practice for functions doing a simple test and returning a bool.
> No big deal anyway.
>  > migration_rate_limit_exceeded()
>> seems clear to me.
>>>   qemu_file_acct_rate_limit -> migration_rate_limit_inc
>> My problem for this one is that we are not increasing the
>> rate_limit, we
>> are "decreasing" the amount of data we have for this period.  That is
>> why I thought about _account(), but who knows.
>>> Also, migration_rate_limit() would need some prefix to understand what is
>>> its purpose.
>> What do you mean here?
> I am referring to :
>   /* Returns true if the rate limiting was broken by an urgent request */
>   bool migration_rate_limit(void)
>   {
>       ...
>       return urgent;
>   }

out of ideas:

- the good
  *we wait if we have to
- the bad
  we can be interrupted if there is anything urgent
  we only wait if counters says that we have to

* we always check
* we return a value consistent with checking
* but we check if we have to wait, not if there is anythying urgent

I am leaving it with migration_rate_limit() name until someone cames
with a better one.  It is not worse than what we have in.

> which existed prior to the name changes and I thought migration_rate_limit()
> would suffer the same fate. May be keep the '_limit' suffix for this one if
> you remove it for the others ?

I am no sure if migration_rate() is better than migration_rate_limit().

Later, Juan.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]