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

[David Hildenbrand]

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