* Chen Gang (gang.chen.5...@gmail.com) wrote:
>
> Firstly, thank you very much for reviewing the related 2 patches.
>
> On 06/03/2014 03:31 PM, ChenLiang wrote:
> > On 2014/6/2 20:35, Chen Gang wrote:
> >
> >> 'encoded_buf' and 'current_buf' are lock protected during using in
> >> save_xbzrle_page() in ram_save_page(), and during freeing in
> >> migration_end().
> >>
> >> So recommend to let them lock protected during starting, just like we
> >> have done to 'cache'.
This isn't one for Trivial - it needs some thought. Locks always need thought.
> >> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com>
> >> ---
> >> arch_init.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch_init.c b/arch_init.c
> >> index 23044c1..784922c 100644
> >> --- a/arch_init.c
> >> +++ b/arch_init.c
> >> @@ -783,12 +783,12 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >> error_report("Error creating cache");
> >> return -1;
> >> }
> >> - XBZRLE_cache_unlock();
> >>
> >> /* We prefer not to abort if there is no memory */
> >> XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
> >> if (!XBZRLE.encoded_buf) {
> >> error_report("Error allocating encoded_buf");
> >> + XBZRLE_cache_unlock();
> >> return -1;
> >> }
> >>
> >> @@ -797,8 +797,10 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >> error_report("Error allocating current_buf");
> >> g_free(XBZRLE.encoded_buf);
> >> XBZRLE.encoded_buf = NULL;
> >> + XBZRLE_cache_unlock();
> >> return -1;
> >> }
> >> + XBZRLE_cache_unlock();
> >>
> >> acct_clear();
> >> }
> >
> >
> > It isn't necessary.
> >
>
> For me, it is still necessary to be improved.
>
> - If this code is not performance sensitive:
>
> "always lock protected" will let source code easy understanding for
> readers, and easy sustainable for maintainers.
>
> - Else (performance sensitive):
>
> Need give related comments to it (e.g. for performance reason, let it
> out of lock protected). So readers easily understand, and the related
> maintainers may notice about it at any time.
>
> It is not good to give comments in a function, except the code is
> not used in normal way.
I think the locks for the cache were originally there to handle the race
between a resize (on the main thread) against the use (in the migration thread).
That doesn't happen for the encoded_buf so I don't think we have that problem.
Is there any other case that it could race?
I can't see one - I think all of the other start/end is done within the
migration
thread.
Dave
>
>
> Thanks.
> --
> Chen Gang
>
> Open, share, and attitude like air, water, and life which God blessed
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
