Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Maxim Levitsky]

On Wed, 2020-05-27 at 14:11 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:46PM +0300, Maxim Levitsky wrote:
> >  /* The operands of the minus operator must have the same type,
> >   * which must be the one that we specify in the cast.
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 56cee1483f..70877840a2 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
> > Error **errp)
> >          return;
> >      }
> >      dev = qdev_device_add(opts, &local_err);
> > +    drain_call_rcu();
> 
> Please include comments explaining what each drain waits for. Without
> comments we'll quickly lose track of why drain_call_rcu() calls are
> necessary (similar to documenting memory barrier or refcount inc/dec
> pairing).
> 
> > diff --git a/util/rcu.c b/util/rcu.c
> > index 60a37f72c3..e8b1c4d6c5 100644
> > --- a/util/rcu.c
> > +++ b/util/rcu.c
> > @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void 
> > (*func)(struct rcu_head *node))
> >      qemu_event_set(&rcu_call_ready_event);
> >  }
> >  
> > +
> > +struct rcu_drain {
> > +    struct rcu_head rcu;
> > +    QemuEvent drain_complete_event;
> > +};
> > +
> > +static void drain_rcu_callback(struct rcu_head *node)
> > +{
> > +    struct rcu_drain *event = (struct rcu_drain *)node;
> > +    qemu_event_set(&event->drain_complete_event);
> 
> A comment would be nice explaining that callbacks are invoked in
> sequence so we're sure that all previously scheduled callbacks have
> completed when drain_rcu_callback() is invoked.
> 
> > +}
> > +
> > +void drain_call_rcu(void)
> 
> Please document that the main loop mutex is dropped if it's held. This
> will prevent surprises and allow callers to think about thread-safety
> across this call.

Done.
> 
> Aside from the comment requests:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>


Best regards,
        Maxim levitsky