Re: [PATCH v2 1/2] util/log: Derive thread id from getpid() on hosts w/o gettid() syscall

[Greg Kurz <groug@kaod.org>]

On Wed, 19 Oct 2022 16:57:54 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Wed, Oct 19, 2022 at 05:16:50PM +0200, Greg Kurz wrote:
> > A subsequent patch needs to be able to differentiate the main QEMU
> > thread from other threads. An obvious way to do so is to compare
> > log_thread_id() and getpid(), based on the fact that they are equal
> > for the main thread on systems that have the gettid() syscall (e.g.
> > linux).
> > 
> > Adapt the fallback code for systems without gettid() to provide the
> > same assumption.
> > 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  util/log.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/log.c b/util/log.c
> > index d6eb0378c3a3..e1c2535cfcd2 100644
> > --- a/util/log.c
> > +++ b/util/log.c
> > @@ -72,8 +72,13 @@ static int log_thread_id(void)
> >  #elif defined(SYS_gettid)
> >      return syscall(SYS_gettid);
> >  #else
> > +    static __thread int my_id = -1;
> >      static int counter;
> > -    return qatomic_fetch_inc(&counter);
> > +
> > +    if (my_id == -1) {
> > +        my_id = getpid() + qatomic_fetch_inc(&counter);
> > +    }
> > +    return my_id;
> 
> This doesn't look safe for linux-user when we fork, but don't exec.
> 

... which is a "dangerous" situation if the parent is already
multi-threaded at fork() time. The child thread must only call
async-signal-safe functions and...


> The getpid() will change after the fork, but counter won't be
> reset, so a thread in the parent could clash with a thread
> in the forked child.
> 

... pthread_create() isn't one AFAIK. This case has undefined
behavior.

Anyway, no matter what we do, even with a regular fork+exec pattern,
log_thread_id() no longer guarantees unique values for all threads
that could be running concurrently (unlike gettid() or counter++),
e.g. 
- parent process with pid A and one extra thread
  => parent uses thread ids A and A+1
- fork child process with pid B == A+1
- child execs
  => child uses thread id A+1

> I feel like if we want to check for the main thread, we should
> be using pthread_self(), and compare result against the value
> cached from main. Or cache in a __constructor__ function in
> log.c to keep it isolated from main().
> 

Hmm... pthread_self() is only guaranteed to be unique within
a process. It doesn't look safe either to compare results
of pthread_self() from different process contexts.

> 
> With regards,
> Daniel

Thanks for bringing this corner case up ! It highlights that
I should definitely go for another approach that doesn't
require to check for the main thread at all.

Cheers,

--
Greg