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

[Cédric Le Goater]

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;
  }

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 ?

Thanks,

C.


> This is the only rate_limit that I can think in migration.
>
> > Do we really need "_limit" in the names ?
>
> You have a point here.
>
> If nobody complains/suggest anything else, I will drop the _limit for
> the next submission.
>
> Thanks very much.