[Qemu-devel] [PATCH v2 0/3] some fix in tests/migration

[Qemu-devel] [PATCH v2 0/3] some fix in tests/migration

[Mao Zhongyi]

This patchset mainly fixes memory leak, typo and return
value of stress function in stress test.

v2->v1:
- use g_autofree to release memory automatically instead
  of free().                     [Alex Bennée]
                      
Cc: armbru@redhat.com 
Cc: laurent@vivier.eu 
Cc: tony.nguyen@bt.com
Cc: alex.bennee@linaro.org

Mao Zhongyi (3):
  tests/migration: mem leak fix
  tests/migration: fix a typo in comment
  tests/migration：fix unreachable path in stress test

 tests/migration/stress.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/3] tests/migration: mem leak fix

[Mao Zhongyi]

‘data’ has the possibility of memory leaks， so use the
glic macros g_autofree recommended by CODING_STYLE.rst
to automatically release the memory that returned from
g_malloc().

Cc: armbru@redhat.com
Cc: laurent@vivier.eu
Cc: tony.nguyen@bt.com
Cc: alex.bennee@linaro.org

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 tests/migration/stress.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index d9aa4afe92..6cbb2d49d3 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -170,10 +170,10 @@ static unsigned long long now(void)
 static int stressone(unsigned long long ramsizeMB)
 {
     size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
-    char *ram = malloc(ramsizeMB * 1024 * 1024);
+    g_autofree char *ram = malloc(ramsizeMB * 1024 * 1024);
     char *ramptr;
     size_t i, j, k;
-    char *data = malloc(PAGE_SIZE);
+    g_autofree char *data = malloc(PAGE_SIZE);
     char *dataptr;
     size_t nMB = 0;
     unsigned long long before, after;
@@ -186,7 +186,6 @@ static int stressone(unsigned long long ramsizeMB)
     if (!data) {
         fprintf(stderr, "%s (%d): ERROR: cannot allocate %d bytes of RAM: %s\n",
                 argv0, gettid(), PAGE_SIZE, strerror(errno));
-        free(ram);
         return -1;
     }
 
@@ -198,8 +197,6 @@ static int stressone(unsigned long long ramsizeMB)
     memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
 
     if (random_bytes(data, PAGE_SIZE) < 0) {
-        free(ram);
-        free(data);
         return -1;
     }
 
@@ -227,9 +224,6 @@ static int stressone(unsigned long long ramsizeMB)
             }
         }
     }
-
-    free(data);
-    free(ram);
 }
 
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 1/3] tests/migration: mem leak fix

[Alex Bennée]

Mao Zhongyi <maozhongyi@cmss.chinamobile.com> writes:

> ‘data’ has the possibility of memory leaks， so use the
> glic macros g_autofree recommended by CODING_STYLE.rst

nit: glib

> to automatically release the memory that returned from
> g_malloc().
>
> Cc: armbru@redhat.com
> Cc: laurent@vivier.eu
> Cc: tony.nguyen@bt.com
> Cc: alex.bennee@linaro.org
>
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tests/migration/stress.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index d9aa4afe92..6cbb2d49d3 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -170,10 +170,10 @@ static unsigned long long now(void)
>  static int stressone(unsigned long long ramsizeMB)
>  {
>      size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
> -    char *ram = malloc(ramsizeMB * 1024 * 1024);
> +    g_autofree char *ram = malloc(ramsizeMB * 1024 * 1024);
>      char *ramptr;
>      size_t i, j, k;
> -    char *data = malloc(PAGE_SIZE);
> +    g_autofree char *data = malloc(PAGE_SIZE);
>      char *dataptr;
>      size_t nMB = 0;
>      unsigned long long before, after;
> @@ -186,7 +186,6 @@ static int stressone(unsigned long long ramsizeMB)
>      if (!data) {
>          fprintf(stderr, "%s (%d): ERROR: cannot allocate %d bytes of RAM: %s\n",
>                  argv0, gettid(), PAGE_SIZE, strerror(errno));
> -        free(ram);
>          return -1;
>      }
>
> @@ -198,8 +197,6 @@ static int stressone(unsigned long long ramsizeMB)
>      memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
>
>      if (random_bytes(data, PAGE_SIZE) < 0) {
> -        free(ram);
> -        free(data);
>          return -1;
>      }
>
> @@ -227,9 +224,6 @@ static int stressone(unsigned long long ramsizeMB)
>              }
>          }
>      }
> -
> -    free(data);
> -    free(ram);
>  }


--
Alex Bennée

Re: [PATCH v2 1/3] tests/migration: mem leak fix

[Laurent Vivier]

Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
> ‘data’ has the possibility of memory leaks， so use the
> glic macros g_autofree recommended by CODING_STYLE.rst
> to automatically release the memory that returned from
> g_malloc().
> 
> Cc: armbru@redhat.com
> Cc: laurent@vivier.eu
> Cc: tony.nguyen@bt.com
> Cc: alex.bennee@linaro.org
> 
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> ---
>  tests/migration/stress.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index d9aa4afe92..6cbb2d49d3 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -170,10 +170,10 @@ static unsigned long long now(void)
>  static int stressone(unsigned long long ramsizeMB)
>  {
>      size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
> -    char *ram = malloc(ramsizeMB * 1024 * 1024);
> +    g_autofree char *ram = malloc(ramsizeMB * 1024 * 1024);
>      char *ramptr;
>      size_t i, j, k;
> -    char *data = malloc(PAGE_SIZE);
> +    g_autofree char *data = malloc(PAGE_SIZE);

So perhaps g_malloc() could be a better choice as it will exit on
allocation failure?

Thanks,
Laurent

Re: [PATCH v2 1/3] tests/migration: mem leak fix

[maozy]

On 10/1/19 11:31 PM, Laurent Vivier wrote:
> Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
>> ‘data’ has the possibility of memory leaks， so use the
>> glic macros g_autofree recommended by CODING_STYLE.rst
>> to automatically release the memory that returned from
>> g_malloc().
>>
>> Cc: armbru@redhat.com
>> Cc: laurent@vivier.eu
>> Cc: tony.nguyen@bt.com
>> Cc: alex.bennee@linaro.org
>>
>> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
>> ---
>>   tests/migration/stress.c | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
>> index d9aa4afe92..6cbb2d49d3 100644
>> --- a/tests/migration/stress.c
>> +++ b/tests/migration/stress.c
>> @@ -170,10 +170,10 @@ static unsigned long long now(void)
>>   static int stressone(unsigned long long ramsizeMB)
>>   {
>>       size_t pagesPerMB = 1024 * 1024 / PAGE_SIZE;
>> -    char *ram = malloc(ramsizeMB * 1024 * 1024);
>> +    g_autofree char *ram = malloc(ramsizeMB * 1024 * 1024);
>>       char *ramptr;
>>       size_t i, j, k;
>> -    char *data = malloc(PAGE_SIZE);
>> +    g_autofree char *data = malloc(PAGE_SIZE);
> So perhaps g_malloc() could be a better choice as it will exit on
> allocation failure?

right,  will fix it in next.

Thanks,
Mao
>
> Thanks,
> Laurent
>
>

[Qemu-devel] [PATCH v2 2/3] tests/migration: fix a typo in comment

[Mao Zhongyi]

Cc: armbru@redhat.com
Cc: laurent@vivier.eu
Cc: tony.nguyen@bt.com

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/migration/stress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index 6cbb2d49d3..19a6eff5fd 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -191,7 +191,7 @@ static int stressone(unsigned long long ramsizeMB)
 
     /* We don't care about initial state, but we do want
      * to fault it all into RAM, otherwise the first iter
-     * of the loop below will be quite slow. We cna't use
+     * of the loop below will be quite slow. We can't use
      * 0x0 as the byte as gcc optimizes that away into a
      * calloc instead :-) */
     memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
-- 
2.17.1

Re: [PATCH v2 2/3] tests/migration: fix a typo in comment

[Laurent Vivier]

Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
> Cc: armbru@redhat.com
> Cc: laurent@vivier.eu
> Cc: tony.nguyen@bt.com
> 
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/migration/stress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index 6cbb2d49d3..19a6eff5fd 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -191,7 +191,7 @@ static int stressone(unsigned long long ramsizeMB)
>  
>      /* We don't care about initial state, but we do want
>       * to fault it all into RAM, otherwise the first iter
> -     * of the loop below will be quite slow. We cna't use
> +     * of the loop below will be quite slow. We can't use
>       * 0x0 as the byte as gcc optimizes that away into a
>       * calloc instead :-) */
>      memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH v2 2/3] tests/migration: fix a typo in comment

[Philippe Mathieu-Daudé]

On 9/11/19 5:31 AM, Mao Zhongyi wrote:
> Cc: armbru@redhat.com
> Cc: laurent@vivier.eu
> Cc: tony.nguyen@bt.com
> 
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/migration/stress.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index 6cbb2d49d3..19a6eff5fd 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -191,7 +191,7 @@ static int stressone(unsigned long long ramsizeMB)
>   
>       /* We don't care about initial state, but we do want
>        * to fault it all into RAM, otherwise the first iter
> -     * of the loop below will be quite slow. We cna't use
> +     * of the loop below will be quite slow. We can't use
>        * 0x0 as the byte as gcc optimizes that away into a
>        * calloc instead :-) */
>       memset(ram, 0xfe, ramsizeMB * 1024 * 1024);
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[Qemu-devel] [PATCH v2 3/3] tests/migration：fix unreachable path in stress test

[Mao Zhongyi]

if stress function always return 0, the path
'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
so fix it to allow the test failed.

Cc: armbru@redhat.com
Cc: laurent@vivier.eu
Cc: tony.nguyen@bt.com

Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
---
 tests/migration/stress.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/migration/stress.c b/tests/migration/stress.c
index 19a6eff5fd..35903d90c4 100644
--- a/tests/migration/stress.c
+++ b/tests/migration/stress.c
@@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
             }
         }
     }
+    return 0;
 }
 
 
@@ -248,9 +249,7 @@ static int stress(unsigned long long ramsizeGB, int ncpus)
                        stressthread,   &ramsizeMB);
     }
 
-    stressone(ramsizeMB);
-
-    return 0;
+    return stressone(ramsizeMB);
 }
 
 
-- 
2.17.1

Re: [PATCH v2 3/3] tests/migration：fix unreachable path in stress test

[maozy]

ping...


On 9/11/19 11:31 AM, Mao Zhongyi wrote:
> if stress function always return 0, the path
> 'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
> so fix it to allow the test failed.
>
> Cc: armbru@redhat.com
> Cc: laurent@vivier.eu
> Cc: tony.nguyen@bt.com
>
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> ---
>   tests/migration/stress.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index 19a6eff5fd..35903d90c4 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
>               }
>           }
>       }
> +    return 0;
>   }
>   
>   
> @@ -248,9 +249,7 @@ static int stress(unsigned long long ramsizeGB, int ncpus)
>                          stressthread,   &ramsizeMB);
>       }
>   
> -    stressone(ramsizeMB);
> -
> -    return 0;
> +    return stressone(ramsizeMB);
>   }
>   
>   

Re: [PATCH v2 3/3] tests/migration：fix unreachable path in stress test

[Laurent Vivier]

Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
> if stress function always return 0, the path
> 'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
> so fix it to allow the test failed.
> 
> Cc: armbru@redhat.com
> Cc: laurent@vivier.eu
> Cc: tony.nguyen@bt.com
> 
> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
> ---
>  tests/migration/stress.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
> index 19a6eff5fd..35903d90c4 100644
> --- a/tests/migration/stress.c
> +++ b/tests/migration/stress.c
> @@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
>              }
>          }
>      }
> +    return 0;
>  }

before the return, we have an infinite loop "while(1) { }".

So this part is dead code.

In fact, if the function exits, it's because it fails, otherwise it
loops infinitely, so I think we should change its type to void and
stress should always return -1.

Thanks,
Laurent

Re: _[PATCH_v2_3/3]_tests/migration：fix_unreachable_path_in_stress_test

[maozy]

Hi,  Laurent

On 10/1/19 11:46 PM, Laurent Vivier wrote:
> Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
>> if stress function always return 0, the path
>> 'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
>> so fix it to allow the test failed.
>>
>> Cc: armbru@redhat.com
>> Cc: laurent@vivier.eu
>> Cc: tony.nguyen@bt.com
>>
>> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
>> ---
>>   tests/migration/stress.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
>> index 19a6eff5fd..35903d90c4 100644
>> --- a/tests/migration/stress.c
>> +++ b/tests/migration/stress.c
>> @@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
>>               }
>>           }
>>       }
>> +    return 0;
>>   }
> before the return, we have an infinite loop "while(1) { }".
>
> So this part is dead code.
>
> In fact, if the function exits, it's because it fails, otherwise it
> loops infinitely, so I think we should change its type to void and
> stress should always return -1.
Yes, I think it's ok to change stressone typo to void because
no one cares about its return value, but if make stress always
return -1, main will always exited in exit_failure, like this:

...
     if (stress(ramsizeGB, ncpus) < 0)
         exit_failure();

     exit_success();
}

so, perhaps also change stress typo to void may be good. then:

...
     stress(ramsizeGB, ncpus);

     exit_success();
}

Anther way , make stressone return 0 when infinite loop fails to
exit, then main can handle both success and failure case.

what do you think?

Thanks,
Mao
> Thanks,
> Laurent
>

Re: _[PATCH_v2_3/3]_tests/migration：fix_unreachable_path_in_stress_test

[Laurent Vivier]

Le 03/10/2019 à 09:17, maozy a écrit :
> Hi,  Laurent
> 
> On 10/1/19 11:46 PM, Laurent Vivier wrote:
>> Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
>>> if stress function always return 0, the path
>>> 'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
>>> so fix it to allow the test failed.
>>>
>>> Cc: armbru@redhat.com
>>> Cc: laurent@vivier.eu
>>> Cc: tony.nguyen@bt.com
>>>
>>> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
>>> ---
>>>   tests/migration/stress.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
>>> index 19a6eff5fd..35903d90c4 100644
>>> --- a/tests/migration/stress.c
>>> +++ b/tests/migration/stress.c
>>> @@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
>>>               }
>>>           }
>>>       }
>>> +    return 0;
>>>   }
>> before the return, we have an infinite loop "while(1) { }".
>>
>> So this part is dead code.
>>
>> In fact, if the function exits, it's because it fails, otherwise it
>> loops infinitely, so I think we should change its type to void and
>> stress should always return -1.
> Yes, I think it's ok to change stressone typo to void because
> no one cares about its return value, but if make stress always
> return -1, main will always exited in exit_failure, like this:
> 
> ...
>     if (stress(ramsizeGB, ncpus) < 0)
>         exit_failure();
> 
>     exit_success();
> }
> 
> so, perhaps also change stress typo to void may be good. then:
> 
> ...
>     stress(ramsizeGB, ncpus);
> 
>     exit_success();
> }
> 
> Anther way , make stressone return 0 when infinite loop fails to
> exit, then main can handle both success and failure case.
> 
> what do you think?
> 

If stressone() or stress() exits it's because of a failure because the
test runs forever otherwise.

So I think there is no problem to use exit_failure() as the exit
function of main(): it should never be reached if the test runs without
error.

Thanks,
Laurent

Re: _[PATCH_v2_3/3]_tests/migration：fix_unreachable_path_in_stress_test

[maozy]

On 10/3/19 5:23 PM, Laurent Vivier wrote:
> Le 03/10/2019 à 09:17, maozy a écrit :
>> Hi,  Laurent
>>
>> On 10/1/19 11:46 PM, Laurent Vivier wrote:
>>> Le 11/09/2019 à 05:31, Mao Zhongyi a écrit :
>>>> if stress function always return 0, the path
>>>> 'if (stress(ramsizeGB, ncpus) < 0)' is nerver unreachable,
>>>> so fix it to allow the test failed.
>>>>
>>>> Cc: armbru@redhat.com
>>>> Cc: laurent@vivier.eu
>>>> Cc: tony.nguyen@bt.com
>>>>
>>>> Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com>
>>>> ---
>>>>    tests/migration/stress.c | 5 ++---
>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tests/migration/stress.c b/tests/migration/stress.c
>>>> index 19a6eff5fd..35903d90c4 100644
>>>> --- a/tests/migration/stress.c
>>>> +++ b/tests/migration/stress.c
>>>> @@ -224,6 +224,7 @@ static int stressone(unsigned long long ramsizeMB)
>>>>                }
>>>>            }
>>>>        }
>>>> +    return 0;
>>>>    }
>>> before the return, we have an infinite loop "while(1) { }".
>>>
>>> So this part is dead code.
>>>
>>> In fact, if the function exits, it's because it fails, otherwise it
>>> loops infinitely, so I think we should change its type to void and
>>> stress should always return -1.
>> Yes, I think it's ok to change stressone typo to void because
>> no one cares about its return value, but if make stress always
>> return -1, main will always exited in exit_failure, like this:
>>
>> ...
>>      if (stress(ramsizeGB, ncpus) < 0)
>>          exit_failure();
>>
>>      exit_success();
>> }
>>
>> so, perhaps also change stress typo to void may be good. then:
>>
>> ...
>>      stress(ramsizeGB, ncpus);
>>
>>      exit_success();
>> }
>>
>> Anther way , make stressone return 0 when infinite loop fails to
>> exit, then main can handle both success and failure case.
>>
>> what do you think?
>>
> 
> If stressone() or stress() exits it's because of a failure because the
> test runs forever otherwise.
> 
> So I think there is no problem to use exit_failure() as the exit
> function of main(): it should never be reached if the test runs without
> error.

OK, I see, thanks a lot.

> 
> Thanks,
> Laurent
>