Re: [PULL 06/30] softmmu/dirtylimit: Implement virtual CPU throttle

[Hyman]

在 2022/7/29 22:14, Richard Henderson 写道:
> On 7/29/22 06:31, Peter Maydell wrote:
> > On Wed, 20 Jul 2022 at 12:30, Dr. David Alan Gilbert (git)
> > <dgilbert@redhat.com> wrote:
> > > From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> > >
> > > Setup a negative feedback system when vCPU thread
> > > handling KVM_EXIT_DIRTY_RING_FULL exit by introducing
> > > throttle_us_per_full field in struct CPUState. Sleep
> > > throttle_us_per_full microseconds to throttle vCPU
> > > if dirtylimit is in service.
> > >
> > > Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > Message-Id: 
> > > <977e808e03a1cef5151cae75984658b6821be618.1656177590.git.huangy81@chinatelecom.cn> 
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> >
> > Hi; Coverity points out a problem with this code (CID 1490787):
Thanks for pointing out this bug.  I'm making a access request for
https://scan.coverity.com so that coverity problem can be found once new
series be posted. Hoping this bug doesn't appear anymore. :)
> > > +static inline int64_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 * TARGET_PAGE_SIZE >> 20;
> >
> > Because dirty_ring_size and TARGET_PAGE_SIZE are both 32 bits,
> > this multiplication will be done as a 32-bit operation,
> > which could overflow. You should cast one of the operands
> > to uint64_t to ensure that the operation is done as a 64 bit
> > multiplication.
>
> To compute MB, you don't need multiplication:
>
>    dirty_ring_size >> (20 - TARGET_PAGE_BITS)
>
> In addition, why the mismatch in type?  dirty_ring_size_memory_MB can 
> never be larger than dirty_ring_size.
>
>
> r~
I'll post bugfix patch later as your suggestion, please review, thanks.
> > Side note: typo in the variable name: should be 'memory'.
> >
> >
> > > +    if (max_dirtyrate < dirtyrate) {
> > > +        max_dirtyrate = dirtyrate;
> > > +    }
> > > +
> > > +    return dirty_ring_size_meory_MB * 1000000 / max_dirtyrate;
> > > +}
> >
> > thanks
> > -- PMM