Re: [PATCH v4 3/7] util/oslib-posix: Introduce and use MemsetContext for touch_all_pages()

[David Hildenbrand <david@redhat.com>]

On 07.10.21 12:05, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> Let's minimize the number of global variables to prepare for
>> os_mem_prealloc() getting called concurrently and make the code a bit
>> easier to read.
>>
>> The only consumer that really needs a global variable is the sigbus
>> handler, which will require protection via a mutex in the future either way
>> as we cannot concurrently mess with the SIGBUS handler.
>>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   util/oslib-posix.c | 73 +++++++++++++++++++++++++++++-----------------
>>   1 file changed, 47 insertions(+), 26 deletions(-)
>>
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index cb89e07770..cf2ead54ad 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -73,21 +73,30 @@
>>   
>>   #define MAX_MEM_PREALLOC_THREAD_COUNT 16
>>   
>> +struct MemsetThread;
>> +
>> +typedef struct MemsetContext {
>> +    bool all_threads_created;
>> +    bool any_thread_failed;
>> +    struct MemsetThread *threads;
>> +    int num_threads;
>> +} MemsetContext;
>> +
>>   struct MemsetThread {
>>       char *addr;
>>       size_t numpages;
>>       size_t hpagesize;
>>       QemuThread pgthread;
>>       sigjmp_buf env;
>> +    MemsetContext *context;
>>   };
>>   typedef struct MemsetThread MemsetThread;
>>   
>> -static MemsetThread *memset_thread;
>> -static int memset_num_threads;
>> +/* used by sigbus_handler() */
>> +static MemsetContext *sigbus_memset_context;
>>   
>>   static QemuMutex page_mutex;
>>   static QemuCond page_cond;
>> -static bool threads_created_flag;
> 
> Is there a reason you didn't lift page_mutex and page_cond into the
> MemsetContext ?

Mostly for simplicity and I didn't think that it will really make a 
difference in practice.

In patch #6 I spelled out that this was done: "Note that page_mutex and 
page_cond are shared between concurrent invocations, which shouldn't be 
a problem."

> 
> (You don't need to change it for this series, just a thought;
> another thought is that I think we hav ea few threadpools like this
> with hooks to check they've all been created and to do something if one
> fails).

I can look into that as an add-on series once I have some spare cycles.

Thanks!

-- 
Thanks,

David / dhildenb