Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Always be sure that 'encoded_buf' and 'current_buf' are lock protected

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Chen Gang (gang.chen.5i5j@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.5i5j@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 / dgilbert@redhat.com / Manchester, UK