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

[Chen Gang <gang.chen.5i5j@gmail.com>]

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'.
>>
>>
>> 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.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed