Re: [Qemu-devel] [PATCH for-4.0 v9 15/16] qemu_thread: supplement error handling for touch_all_pages

[fei <lifei1214@126.com>]

> 在 2019年1月8日，02:13，Markus Armbruster <armbru@redhat.com> 写道：
> 
> Fei Li <fli@suse.com> writes:
> 
>> Supplement the error handling for touch_all_pages: add an Error
>> parameter for it to propagate the error to its caller to do the
>> handling in case it fails.
>> 
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>> ---
>> util/oslib-posix.c | 25 ++++++++++++++++---------
>> 1 file changed, 16 insertions(+), 9 deletions(-)
>> 
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index 251e2f1aea..afc1d99093 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -431,15 +431,17 @@ static inline int get_memset_num_threads(int smp_cpus)
>> }
>> 
>> static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
>> -                            int smp_cpus)
>> +                            int smp_cpus, Error **errp)
>> {
>>     size_t numpages_per_thread;
>>     size_t size_per_thread;
>>     char *addr = area;
>>     int i = 0;
>> +    int started_thread = 0;
>> 
>>     memset_thread_failed = false;
>>     memset_num_threads = get_memset_num_threads(smp_cpus);
>> +    started_thread = memset_num_threads;
>>     memset_thread = g_new0(MemsetThread, memset_num_threads);
>>     numpages_per_thread = (numpages / memset_num_threads);
>>     size_per_thread = (hpagesize * numpages_per_thread);
>> @@ -448,14 +450,18 @@ static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
>>         memset_thread[i].numpages = (i == (memset_num_threads - 1)) ?
>>                                     numpages : numpages_per_thread;
>>         memset_thread[i].hpagesize = hpagesize;
>> -        /* TODO: let the callers handle the error instead of abort() here */
>> -        qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
>> -                           do_touch_pages, &memset_thread[i],
>> -                           QEMU_THREAD_JOINABLE, &error_abort);
>> +        if (!qemu_thread_create(&memset_thread[i].pgthread, "touch_pages",
>> +                                do_touch_pages, &memset_thread[i],
>> +                                QEMU_THREAD_JOINABLE, errp)) {
>> +            memset_thread_failed = true;
>> +            started_thread = i;
>> +            goto out;
> 
> break rather than goto, please.
Ok
> 
>> +        }
>>         addr += size_per_thread;
>>         numpages -= numpages_per_thread;
>>     }
>> -    for (i = 0; i < memset_num_threads; i++) {
>> +out:
>> +    for (i = 0; i < started_thread; i++) {
>>         qemu_thread_join(&memset_thread[i].pgthread);
>>     }
> 
> I don't like how @started_thread is computed.  The name suggests it's
> the number of threads started so far.  That's the case when you
> initialize it to zero.  But then you immediately set it to
> memset_thread().  It again becomes the case only when you break the loop
> on error, or when you complete it successfully.
> 
> There's no need for @started_thread, since the number of threads created
> is readily available as @i:
> 
>       memset_num_threads = i;
Thanks for this wonderful suggestion! This helps a lot! ;)
>       for (i = 0; i < memset_num_threads; i++) {
>           qemu_thread_join(&memset_thread[i].pgthread);
>       }
> 
> Rest of the function:
> 
>>     g_free(memset_thread);
>       memset_thread = NULL;
> 
>       return memset_thread_failed;
>   }
> 
> If do_touch_pages() set memset_thread_failed(), we return false without
> setting an error.  I believe you should
> 
>       if (memset_thread_failed) {
>           error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
>               "pages available to allocate guest RAM");
>           return false;
>       }
>       return true;
> 
Ok
> here, and ...
> 
>> @@ -471,6 +477,7 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>>     struct sigaction act, oldact;
>>     size_t hpagesize = qemu_fd_getpagesize(fd);
>>     size_t numpages = DIV_ROUND_UP(memory, hpagesize);
>> +    Error *local_err = NULL;
>> 
>>     memset(&act, 0, sizeof(act));
>>     act.sa_handler = &sigbus_handler;
>> @@ -484,9 +491,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory, int smp_cpus,
>>     }
>> 
>>     /* touch pages simultaneously */
>> -    if (touch_all_pages(area, hpagesize, numpages, smp_cpus)) {
>> -        error_setg(errp, "os_mem_prealloc: Insufficient free host memory "
>> -            "pages available to allocate guest RAM");
>> +    if (touch_all_pages(area, hpagesize, numpages, smp_cpus, &local_err)) {
>> +        error_propagate_prepend(errp, local_err, "os_mem_prealloc: Insufficient"
>> +            " free host memory pages available to allocate guest RAM: ");
>>     }
> 
> ... not mess with the error message here, i.e.
> 
>       touch_all_pages(area, hpagesize, numpages, smp_cpus), errp);
Ok, will amend this in the next version. 

Have a nice day, thanks again
Fei
> 
>> 
>>     ret = sigaction(SIGBUS, &oldact, NULL);