On 05/09/2016 16:55, Alex Bennée wrote:
>> > -/* These are no-ops because we are not threadsafe. */
>> > -static inline void cpu_exec_start(CPUState *cpu)
>> > -{
>> > -}
>> > -
>> > -static inline void cpu_exec_end(CPUState *cpu)
>> > -{
>> > -}
>> > -
>> > -static inline void start_exclusive(void)
>> > -{
>> > -}
>> > -
>> > -static inline void end_exclusive(void)
>> > -{
>> > -}
>> > -
>
> Does this mean BSD user now gets exclusive support as well or is other
> plumbing required for the tb_flush to work?
It does, though I have forgotten to add the call to process the work items.
>> > void fork_start(void)
>> > {
>> > }
>> > diff --git a/cpus-common.c b/cpus-common.c
>> > index 12729e5..47f7c06 100644
>> > --- a/cpus-common.c
>> > +++ b/cpus-common.c
>> > @@ -23,11 +23,21 @@
>> > #include "exec/memory-internal.h"
>> >
>> > static QemuMutex qemu_cpu_list_mutex;
>> > +static QemuCond exclusive_cond;
>> > +static QemuCond exclusive_resume;
>> > static QemuCond qemu_work_cond;
>> >
>> > +static int pending_cpus;
>> > +
>> > void qemu_init_cpu_list(void)
>> > {
>> > + /* This is needed because qemu_init_cpu_list is also called by the
>> > + * child process in a fork. */
>> > + pending_cpus = 0;
>> > +
>> > qemu_mutex_init(&qemu_cpu_list_mutex);
>> > + qemu_cond_init(&exclusive_cond);
>> > + qemu_cond_init(&exclusive_resume);
>> > qemu_cond_init(&qemu_work_cond);
>> > }
>> >
>> > @@ -52,6 +62,12 @@ static int cpu_get_free_index(void)
>> > return cpu_index;
>> > }
>> >
>> > +static void finish_safe_work(CPUState *cpu)
>> > +{
>> > + cpu_exec_start(cpu);
>> > + cpu_exec_end(cpu);
>> > +}
>> > +
>> > void cpu_list_add(CPUState *cpu)
>> > {
>> > qemu_mutex_lock(&qemu_cpu_list_mutex);
>> > @@ -62,6 +78,8 @@ void cpu_list_add(CPUState *cpu)
>> >
>> > QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>> > qemu_mutex_unlock(&qemu_cpu_list_mutex);
>> > +
>> > + finish_safe_work(cpu);
>> > }
>> >
>> > void cpu_list_remove(CPUState *cpu)
>> > @@ -131,6 +149,70 @@ void async_run_on_cpu(CPUState *cpu, run_on_cpu_func
>> > func, void *data)
>> > queue_work_on_cpu(cpu, wi);
>> > }
>> >
>> > +/* Wait for pending exclusive operations to complete. The exclusive lock
>> > + must be held. */
>> > +static inline void exclusive_idle(void)
>> > +{
>> > + while (pending_cpus) {
>> > + qemu_cond_wait(&exclusive_resume, &qemu_cpu_list_mutex);
>> > + }
>> > +}
>> > +
>> > +/* Start an exclusive operation.
>> > + Must only be called from outside cpu_exec, takes
>> > + qemu_cpu_list_mutex. */
>> > +void start_exclusive(void)
>> > +{
>> > + CPUState *other_cpu;
>> > +
>> > + qemu_mutex_lock(&qemu_cpu_list_mutex);
>> > + exclusive_idle();
>> > +
>> > + /* Make all other cpus stop executing. */
>> > + pending_cpus = 1;
>> > + CPU_FOREACH(other_cpu) {
>> > + if (other_cpu->running) {
>> > + pending_cpus++;
>> > + qemu_cpu_kick(other_cpu);
>> > + }
>> > + }
>> > + if (pending_cpus > 1) {
>> > + qemu_cond_wait(&exclusive_cond, &qemu_cpu_list_mutex);
>> > + }
>> > +}
>> > +
>> > +/* Finish an exclusive operation. Releases qemu_cpu_list_mutex. */
>> > +void end_exclusive(void)
>> > +{
>> > + pending_cpus = 0;
>> > + qemu_cond_broadcast(&exclusive_resume);
>> > + qemu_mutex_unlock(&qemu_cpu_list_mutex);
>> > +}
>> > +
>> > +/* Wait for exclusive ops to finish, and begin cpu execution. */
>> > +void cpu_exec_start(CPUState *cpu)
>> > +{
>> > + qemu_mutex_lock(&qemu_cpu_list_mutex);
>> > + exclusive_idle();
>> > + cpu->running = true;
>> > + qemu_mutex_unlock(&qemu_cpu_list_mutex);
>> > +}
>> > +
>> > +/* Mark cpu as not executing, and release pending exclusive ops. */
>> > +void cpu_exec_end(CPUState *cpu)
>> > +{
>> > + qemu_mutex_lock(&qemu_cpu_list_mutex);
>> > + cpu->running = false;
>> > + if (pending_cpus > 1) {
>> > + pending_cpus--;
>> > + if (pending_cpus == 1) {
>> > + qemu_cond_signal(&exclusive_cond);
>> > + }
>> > + }
>> > + exclusive_idle();
>> > + qemu_mutex_unlock(&qemu_cpu_list_mutex);
>> > +}
>> > +
>> > void process_queued_cpu_work(CPUState *cpu)
>> > {
>> > struct qemu_work_item *wi;
>> > diff --git a/cpus.c b/cpus.c
>> > index e1bdc16..b024604 100644
>> > --- a/cpus.c
>> > +++ b/cpus.c
>> > @@ -1456,7 +1456,9 @@ static int tcg_cpu_exec(CPUState *cpu)
>> > cpu->icount_decr.u16.low = decr;
>> > cpu->icount_extra = count;
>> > }
>> > + cpu_exec_start(cpu);
>> > ret = cpu_exec(cpu);
>> > + cpu_exec_end(cpu);
>> > #ifdef CONFIG_PROFILER
>> > tcg_time += profile_getclock() - ti;
>> > #endif
>> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> > index d7688f6..0e04e8f 100644
>> > --- a/include/qom/cpu.h
>> > +++ b/include/qom/cpu.h
>> > @@ -249,7 +249,8 @@ struct qemu_work_item {
>> > * @nr_threads: Number of threads within this CPU.
>> > * @numa_node: NUMA node this CPU is belonging to.
>> > * @host_tid: Host thread ID.
>> > - * @running: #true if CPU is currently running (usermode).
>> > + * @running: #true if CPU is currently running;
>> > + * valid under cpu_list_lock.
>> > * @created: Indicates whether the CPU thread has been successfully
>> > created.
>> > * @interrupt_request: Indicates a pending interrupt request.
>> > * @halted: Nonzero if the CPU is in suspended state.
>> > @@ -826,6 +827,43 @@ void cpu_remove_sync(CPUState *cpu);
>> > void process_queued_cpu_work(CPUState *cpu);
>> >
>> > /**
>> > + * cpu_exec_start:
>> > + * @cpu: The CPU for the current thread.
>> > + *
>> > + * Record that a CPU has started execution and can be interrupted with
>> > + * cpu_exit.
>> > + */
>> > +void cpu_exec_start(CPUState *cpu);
>> > +
>> > +/**
>> > + * cpu_exec_end:
>> > + * @cpu: The CPU for the current thread.
>> > + *
>> > + * Record that a CPU has stopped execution and safe work can be executed
>> > + * without interrupting it.
>> > + */
>> > +void cpu_exec_end(CPUState *cpu);
>> > +
>> > +/**
>> > + * start_exclusive:
>> > + * @cpu: The CPU for the current thread.
>> > + *
>> > + * Similar to async_safe_work_run_on_cpu, but the work item is only
>> > + * run on one CPU and is between start_exclusive and end_exclusive.
>> > + * Returns with the CPU list lock taken (which nests outside all
>> > + * other locks except the BQL).
>> > + */
>> > +void start_exclusive(void);
>
> Hmm the comment here looks as though it has been transplanted or is
> somehow mangled. The function doesn't take @cpu and isn't like
> async_safe_wrok_run_on_cpu.
It doesn't take @cpu, but it is kind of like async_safe_run_on_cpu.
However, async_safe_run_on_cpu doesn't exist yet. :)
Paolo
