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

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

[Chen Gang]

'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();
     }
-- 
1.7.11.7

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

[ChenLiang]

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.

Best regards.
ChenLiang

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

[Chen Gang]

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

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

[Dr. David Alan Gilbert]

* 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

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

[Chen Gang]

Firstly, thank you very much for reviewing the related 2 patches.


On 06/04/2014 06:01 PM, Dr. David Alan Gilbert wrote:
> * 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.
> 

OK, I shall notice about it next time.

"Locks always need thought", so prefer some 'templates' to simplify
thinking, especially for some simple locks (not nested, either not
performance sensitive).


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

What you thought and what Chen Liang though are fine to me. But I guess
it is a simple lock, if we let them always lock protected, we need not
think them more.

One simple lock 'template' can be: "always let it lock protected during
its whole life", that will let all related readers and maintainers easy
understanding and avoid to think of more.

Our lock is not nested, although I am not quit sure whether it is
performance sensitive (I guess not).


Thanks.
-- 
Chen Gang

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

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

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

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

I'd recommend against routing locking patches through -trivial.  The
code has maintainers, and you cc'ed them.

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

[Chen Gang]

On 06/04/2014 04:20 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> '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'.
> 
> I'd recommend against routing locking patches through -trivial.  The
> code has maintainers, and you cc'ed them.
> 

OK, I shall remove -trivial for locking patches, next time.

Thanks.
-- 
Chen Gang

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