[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 01/19] configure: add --enable-tsan flag + fiber annotations
From: |
Robert Foley |
Subject: |
Re: [PATCH 01/19] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext |
Date: |
Wed, 17 Jun 2020 17:27:23 -0400 |
Hi,
On Wed, 17 Jun 2020 at 10:24, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Fri, May 22, 2020 at 12:07:37PM -0400, Robert Foley wrote:
> > +#define UC_DEBUG 0
> > +#if UC_DEBUG && defined(CONFIG_TSAN)
> > +#define UC_TRACE(fmt, ...) fprintf(stderr, "%s:%d:%p " fmt "\n", \
> > + __func__, __LINE__, __tsan_get_current_fiber(), ##__VA_ARGS__);
> > +#else
> > +#define UC_TRACE(fmt, ...)
> > +#endif
>
> QEMU has tracing support, see docs/devel/tracing.txt. These fprintfs
> should be trace events defined in the util/trace-events file.
Thanks for the details. We removed this tracing in a later patch.
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg02506.html
>
> > +
> > /**
> > * Per-thread coroutine bookkeeping
> > */
> > @@ -65,7 +80,20 @@ union cc_arg {
> > int i[2];
> > };
> >
> > -static void finish_switch_fiber(void *fake_stack_save)
> > +/* QEMU_ALWAYS_INLINE only does so if __OPTIMIZE__, so we cannot use it. */
> > +static inline __attribute__((always_inline))
>
> Please document why always_inline is necessary here and in other
> functions. Is it for performance or because the __tsan_*() functions
> need to be called from a the parent function?
We will look into this and add documentation here or (if it is no
longer needed),
remove the inline.
<snip>
> > -static void start_switch_fiber(void **fake_stack_save,
> > - const void *bottom, size_t size)
> > +static inline __attribute__((always_inline)) void start_switch_fiber(
> > + CoroutineAction action, void **fake_stack_save,
> > + const void *bottom, size_t size, void *new_fiber)
> > {
> > #ifdef CONFIG_ASAN
> > - __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
> > + if (action == COROUTINE_TERMINATE) {
> > + __sanitizer_start_switch_fiber(
> > + action == COROUTINE_TERMINATE ? NULL : fake_stack_save,
>
> The if statement already checks action == COROUTINE_TERMINATE, why is it
> being checked again?
>
> I think the old behavior can be retained by dropping the if statement
> like this:
>
> __sanitizer_start_switch_fiber(action == COROUTINE_TERMINATE ?
> NULL : fake_stack_save,
> bottom, size);
>
> > + bottom, size);
Good point. We did change this by dropping the if in a later patch.
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg02506.html
> > + }
> > +#endif
> > +#ifdef CONFIG_TSAN
> > + void *curr_fiber =
> > + __tsan_get_current_fiber();
> > + __tsan_acquire(curr_fiber);
> > +
> > + UC_TRACE("Current fiber: %p.", curr_fiber);
> > + *fake_stack_save = curr_fiber;
> > + UC_TRACE("Switch to fiber %p", new_fiber);
> > + __tsan_switch_to_fiber(new_fiber, 0); /* 0=synchronize */
> > #endif
> > }
>
> Please split start_switch_fiber() into two functions:
> start_switch_fiber_asan() and start_switch_fiber_tsan(). That way the
> asan- and tsan-specific arguments can be kept separate and the
> co->tsan_* fields only need to be compiled in when CONFIG_TSAN is
> defined.
>
> For example:
>
> static inline __attribute__((always_inline))
> void start_switch_fiber_tsan(void **fake_stack_save,
> CoroutineUContext *co,
> bool caller)
> {
> #ifdef CONFIG_TSAN
> void *new_fiber = caller ?
> co->tsan_caller_fiber :
> co->tsan_co_fiber;
> void *curr_fiber = __tsan_get_current_fiber();
> __tsan_acquire(curr_fiber);
>
> UC_TRACE("Current fiber: %p.", curr_fiber);
> *fake_stack_save = curr_fiber;
> UC_TRACE("Switch to fiber %p", new_fiber);
> __tsan_switch_to_fiber(new_fiber, 0); /* 0=synchronize */
> #endif
> }
>
> This does two things:
> 1. Unrelated ASAN and TSAN code is separate and each function only
> has arguments that are actually needed.
> 2. The co->tsan_caller_fiber and co->tsan_co_fiber fields are only
> access from within #ifdef CONFIG_TSAN.
Makes sense, we will make these changes and submit a cleanup patch.
Thanks & Regards,
-Rob