Re: [Qemu-devel] [RFC v4 01/71] cpu: convert queued work to a QSIMPLEQ

[Emilio G. Cota]

On Mon, Oct 29, 2018 at 15:39:29 +0000, Alex Bennée wrote:
> 
> Emilio G. Cota <address@hidden> 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 <address@hidden>

Thanks!

                Emilio