On Mon, Oct 29, 2018 at 15:39:29 +0000, Alex Bennée wrote:
>
> Emilio G. Cota <c...@braap.org> writes:
(snip)
> > @@ -357,7 +357,7 @@ struct CPUState {
> > sigjmp_buf jmp_env;
> >
> > QemuMutex work_mutex;
> > - struct qemu_work_item *queued_work_first, *queued_work_last;
> > + QSIMPLEQ_HEAD(, qemu_work_item) work_list;
>
> Would:
>
> QSIMPLEQ_HEAD(work_list, qemu_work_item);
>
> be neater?
That would expand to
struct CPUState {
...
struct work_list {
struct qemu_work_item *sqh_first;
struct qemu_work_item **sqh_last;
}; // <-- missing field name
...
};
, which doesn't declare an actual field in the struct.
> > +static inline bool cpu_work_list_empty(CPUState *cpu)
> > +{
> > + bool ret;
> > +
> > + qemu_mutex_lock(&cpu->work_mutex);
> > + ret = QSIMPLEQ_EMPTY(&cpu->work_list);
> > + qemu_mutex_unlock(&cpu->work_mutex);
> > + return ret;
>
> This could just be:
>
> return QSIMPLEQ_EMPTY_ATOMIC(&cpu->work_list)
Not quite; (1) the non-RCU version of the list does not set
pointers with atomic_set, so an atomic_read here would not be enough,
and (2) using the lock ensures that the read is up-to-date. These
two points are not a big deal though, since later in the
series ("cpu: protect most CPU state with cpu->lock") we hold the
CPU lock when calling this.
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
Thanks!
Emilio
