Re: [Qemu-devel] [PATCH 1/3] qsp: QEMU's Synchronization Profiler

[Fam Zheng <famz@redhat.com>]

On Mon, 08/13 13:11, Emilio G. Cota wrote:
> +  --enable-sync-profiler) sync_profiler="yes"
> +  ;;

Curious, not asking for a change: can this be made a runtime option instead of
compile time, since there's no library dependencies? That should make this
somewhat easier to use.

> +
> +#define QSP_GEN_VOID(type_, qsp_t_, func_, impl_)                       \
> +    void func_(type_ *obj, const char *file, unsigned line)             \
> +    {                                                                   \
> +        struct qsp_entry *e = qsp_entry_get(obj, file, line, qsp_t_);   \
> +        int64_t t;                                                      \
> +                                                                        \

No qsp_init()?

> +        t = get_clock();                                                \
> +        impl_(obj, file, line);                                         \
> +        atomic_set(&e->ns, e->ns + get_clock() - t);                    \
> +        atomic_set(&e->n_acqs, e->n_acqs + 1);                          \
> +    }
> +
> +#define QSP_GEN_RET1(type_, qsp_t_, func_, impl_)                       \
> +    int func_(type_ *obj, const char *file, unsigned line)              \
> +    {                                                                   \
> +        struct qsp_entry *e = qsp_entry_get(obj, file, line, qsp_t_);   \
> +        int64_t t;                                                      \
> +        int err;                                                        \
> +                                                                        \

Same here.

> +        t = get_clock();                                                \
> +        err = impl_(obj, file, line);                                   \
> +        atomic_set(&e->ns, e->ns + get_clock() - t);                    \
> +        if (!err) {                                                     \
> +            atomic_set(&e->n_acqs, e->n_acqs + 1);                      \
> +        }                                                               \
> +        return err;                                                     \
> +    }
> +
> +QSP_GEN_VOID(QemuMutex, QSP_MUTEX, qsp_mutex_lock, qemu_mutex_lock_impl)
> +QSP_GEN_RET1(QemuMutex, QSP_MUTEX, qsp_mutex_trylock, qemu_mutex_trylock_impl)
> +
> +QSP_GEN_VOID(QemuRecMutex, QSP_REC_MUTEX, qsp_rec_mutex_lock,
> +             qemu_rec_mutex_lock_impl)
> +QSP_GEN_RET1(QemuRecMutex, QSP_REC_MUTEX, qsp_rec_mutex_trylock,
> +             qemu_rec_mutex_trylock_impl)
> +
> +void qsp_cond_wait(QemuCond *cond, QemuMutex *mutex, const char *file,
> +                   unsigned line)
> +{
> +    struct qsp_entry *e;
> +    int64_t t;
> +
> +    qsp_init();
> +
> +    e = qsp_entry_get(cond, file, line, QSP_CONDVAR);
> +    t = get_clock();
> +    qemu_cond_wait_impl(cond, mutex, file, line);
> +    atomic_set(&e->ns, e->ns + get_clock() - t);
> +    atomic_set(&e->n_acqs, e->n_acqs + 1);

Why not atomic_add (both here and in above macros)? Because fetching e->ns and
then updating it is not "atomic" this way.

Fam