On Tue, Jun 12, 2018 at 10:42:25AM +0800, Xiao Guangrong wrote:
>
>
> On 06/11/2018 03:39 PM, Peter Xu wrote:
> > On Mon, Jun 04, 2018 at 05:55:09PM +0800, guangrong.xiao@xxxxxxxxx wrote:
> > > From: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxx>
> > >
> > > Instead of putting the main thread to sleep state to wait for
> > > free compression thread, we can directly post it out as normal
> > > page that reduces the latency and uses CPUs more efficiently
> >
> > The feature looks good, though I'm not sure whether we should make a
> > capability flag for this feature since otherwise it'll be hard to
> > switch back to the old full-compression way no matter for what
> > reason. Would that be a problem?
> >
>
> We assume this optimization should always be optimistic for all cases,
> particularly, we introduced the statistics of compression, then the user
> should adjust its parameters based on those statistics if anything works
> worse.
Ah, that'll be good.
>
> Furthermore, we really need to improve this optimization if it hurts
> any case rather than leaving a option to the user. :)
Yeah, even if we make it a parameter/capability we can still turn that
on by default in new versions but keep the old behavior in old
versions. :) The major difference is that, then we can still _have_ a
way to compress every page. I'm just thinking if we don't have a
switch for that then if someone wants to measure e.g. how a new
compression algo could help VM migration, then he/she won't be
possible to do that again since the numbers will be meaningless if
that bit is out of control on which page will be compressed.
Though I don't know how much use it'll bring... But if that won't be
too hard, it still seems good. Not a strong opinion.
>
> > >
> > > Signed-off-by: Xiao Guangrong <xiaoguangrong@xxxxxxxxxxx>
> > > ---
> > > migration/ram.c | 34 +++++++++++++++-------------------
> > > 1 file changed, 15 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 5bcbf7a9f9..0caf32ab0a 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -1423,25 +1423,18 @@ static int compress_page_with_multi_thread(RAMState *rs, RAMBlock *block,
> > > thread_count = migrate_compress_threads();
> > > qemu_mutex_lock(&comp_done_lock);
> >
> > Can we drop this lock in this case?
>
> The lock is used to protect comp_param[].done...
IMHO it's okay?
It's used in this way:
if (done) {
done = false;
}
So it only switches done from true->false.
And the compression thread is the only one that did the other switch
(false->true). IMHO this special case will allow no-lock since as
long as "done" is true here then current thread will be the only one
to modify it, then no race at all.
>
> Well, we are able to possibly remove it if we redesign the implementation, e.g, use atomic
> access for comp_param.done, however, it still can not work efficiently i believe. Please see
> more in the later reply to your comments in the cover-letter.
Will read that after it arrives; though I didn't receive a reply.
Have you missed clicking the "send" button? ;)
Regards,
--
Peter Xu
