This allows to preserve the semantics of hmp_device_del,
that the device is deleted immediatly which was changed by previos
patch that delayed this to RCU callback

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 include/qemu/rcu.h |  1 +
 qdev-monitor.c     |  3 +++
 util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 570aa603eb..0e375ebe13 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -133,6 +133,7 @@ struct rcu_head {
 };
 
 extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
+extern void drain_call_rcu(void);
 
 /* 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();
+
     if (!dev) {
         error_propagate(errp, local_err);
         qemu_opts_del(opts);
@@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
         }
 
         qdev_unplug(dev, errp);
+        drain_call_rcu();
     }
 }
 
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);
+}
+
+void drain_call_rcu(void)
+{
+    struct rcu_drain rcu_drain;
+    bool locked = qemu_mutex_iothread_locked();
+
+    memset(&rcu_drain, 0, sizeof(struct rcu_drain));
+
+    if (locked) {
+        qemu_mutex_unlock_iothread();
+    }
+
+    qemu_event_init(&rcu_drain.drain_complete_event, false);
+    call_rcu1(&rcu_drain.rcu, drain_rcu_callback);
+    qemu_event_wait(&rcu_drain.drain_complete_event);
+
+    if (locked) {
+        qemu_mutex_lock_iothread();
+    }
+
+}
+
 void rcu_register_thread(void)
 {
     assert(rcu_reader.ctr == 0);
-- 
2.17.2

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.

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

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

Maxim Levitsky <mlevitsk@redhat.com> writes:

> This allows to preserve the semantics of hmp_device_del,
> that the device is deleted immediatly which was changed by previos
> patch that delayed this to RCU callback
>
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  include/qemu/rcu.h |  1 +
>  qdev-monitor.c     |  3 +++
>  util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 570aa603eb..0e375ebe13 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -133,6 +133,7 @@ struct rcu_head {
>  };
>  
>  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> +extern void drain_call_rcu(void);
>  
>  /* 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();
> +
>      if (!dev) {
>          error_propagate(errp, local_err);
>          qemu_opts_del(opts);
> @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
>          }
>  
>          qdev_unplug(dev, errp);
> +        drain_call_rcu();
>      }
>  }
>  

Subject claims "in hmp_device_del", code has it in qmp_device_add() and
qmp_device_del().  Please advise.

[...]

On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > This allows to preserve the semantics of hmp_device_del,
> > that the device is deleted immediatly which was changed by previos
> > patch that delayed this to RCU callback
> > 
> > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  include/qemu/rcu.h |  1 +
> >  qdev-monitor.c     |  3 +++
> >  util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 570aa603eb..0e375ebe13 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -133,6 +133,7 @@ struct rcu_head {
> >  };
> >  
> >  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > +extern void drain_call_rcu(void);
> >  
> >  /* 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();
> > +
> >      if (!dev) {
> >          error_propagate(errp, local_err);
> >          qemu_opts_del(opts);
> > @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
> >          }
> >  
> >          qdev_unplug(dev, errp);
> > +        drain_call_rcu();
> >      }
> >  }
> >  
> 
> Subject claims "in hmp_device_del", code has it in qmp_device_add() and
> qmp_device_del().  Please advise.

I added it in both, because addition of a device can fail and trigger removal,
which can also be now delayed due to RCU.
Since both device_add and device_del aren't used often, the overhead won't
be a problem IMHO.

Best regards,
	Maxim Levitsky

> 
> [...]

On 09/07/20 13:56, Maxim Levitsky wrote:
> On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote:
>> Maxim Levitsky <mlevitsk@redhat.com> writes:
>>
>>> This allows to preserve the semantics of hmp_device_del,
>>> that the device is deleted immediatly which was changed by previos
>>> patch that delayed this to RCU callback
>>>
>>> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>> ---
>>>  include/qemu/rcu.h |  1 +
>>>  qdev-monitor.c     |  3 +++
>>>  util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
>>>  3 files changed, 37 insertions(+)
>>>
>>> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
>>> index 570aa603eb..0e375ebe13 100644
>>> --- a/include/qemu/rcu.h
>>> +++ b/include/qemu/rcu.h
>>> @@ -133,6 +133,7 @@ struct rcu_head {
>>>  };
>>>  
>>>  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
>>> +extern void drain_call_rcu(void);
>>>  
>>>  /* 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();
>>> +
>>>      if (!dev) {
>>>          error_propagate(errp, local_err);
>>>          qemu_opts_del(opts);
>>> @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
>>>          }
>>>  
>>>          qdev_unplug(dev, errp);
>>> +        drain_call_rcu();
>>>      }
>>>  }
>>>  
>>
>> Subject claims "in hmp_device_del", code has it in qmp_device_add() and
>> qmp_device_del().  Please advise.
> 
> I added it in both, because addition of a device can fail and trigger removal,
> which can also be now delayed due to RCU.
> Since both device_add and device_del aren't used often, the overhead won't
> be a problem IMHO.

Ok, just mention this in the commit message.  It may also be a good idea
to move it from qmp_device_add to the error-propagation section of
qdev_device_add.

Paolo