Re: [PATCH v2 1/3] accel: introduce accelerator blocker API

[Paolo Bonzini]

On 11/10/22 17:48, Emanuele Giuseppe Esposito wrote:
> +/*
> + * QEMU accel blocker class

"Lock to inhibit accelerator ioctls"

> + *
> + * Copyright (c) 2014 Red Hat Inc.

2022, you can also add an Author line.

> +static int accel_in_ioctls(void)

Return bool (and return early if ret becomes true).

> +void accel_ioctl_inhibit_begin(void)
> +{
> +    CPUState *cpu;
> +
> +    /*
> +     * We allow to inhibit only when holding the BQL, so we can identify
> +     * when an inhibitor wants to issue an ioctl easily.
> +     */
> +    g_assert(qemu_mutex_iothread_locked());
> +
> +    /* Block further invocations of the ioctls outside the BQL.  */
> +    CPU_FOREACH(cpu) {
> +        qemu_lockcnt_lock(&cpu->in_ioctl_lock);
> +    }
> +    qemu_lockcnt_lock(&accel_in_ioctl_lock);
> +
> +    /* Keep waiting until there are running ioctls */
> +    while (accel_in_ioctls()) {
> +        /* Reset event to FREE. */
> +        qemu_event_reset(&accel_in_ioctl_event);
> +
> +        if (accel_in_ioctls()) {
> +
> +            CPU_FOREACH(cpu) {
> +                /* exit the ioctl */
> +                qemu_cpu_kick(cpu);

Only kick if the lockcnt count is > 0? (this is not racy; if it is == 0, 
it cannot ever become > 0 again while the lock is taken)

> diff --git a/include/sysemu/accel-blocker.h b/include/sysemu/accel-blocker.h
> new file mode 100644
> index 0000000000..135ebea566
> --- /dev/null
> +++ b/include/sysemu/accel-blocker.h
> @@ -0,0 +1,45 @@
> +/*
> + * Accelerator blocking API, to prevent new ioctls from starting and wait the
> + * running ones finish.
> + * This mechanism differs from pause/resume_all_vcpus() in that it does not
> + * release the BQL.
> + *
> + *  Copyright (c) 2014 Red Hat Inc.

2022, you can also add an Author line here too.

> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef ACCEL_BLOCKER_H
> +#define ACCEL_BLOCKER_H
> +
> +#include "qemu/osdep.h"
> +#include "qemu/accel.h"

qemu/accel.h not needed?

> +#include "sysemu/cpus.h"
> +
> +extern void accel_blocker_init(void);
> +
> +/*
> + * accel_set_in_ioctl/accel_cpu_set_in_ioctl:
> + * Mark when ioctl is about to run or just finished.
> + * If @in_ioctl is true, then mark it is beginning. Otherwise marks that it is
> + * ending.
> + *
> + * These functions will block after accel_ioctl_inhibit_begin() is called,
> + * preventing new ioctls to run. They will continue only after
> + * accel_ioctl_inibith_end().
> + */
> +extern void accel_set_in_ioctl(bool in_ioctl);
> +extern void accel_cpu_set_in_ioctl(CPUState *cpu, bool in_ioctl);

Why not just

extern void accel_ioctl_begin(void);
extern void accel_ioctl_end(void);
extern void accel_cpu_ioctl_begin(CPUState *cpu);
extern void accel_cpu_ioctl_end(CPUState *cpu);

?

Otherwise it's very nice.

Paolo