[Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base

Victor Kaplansky posted 1 patch 6 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171128133123.17372-1-vkaplans@redhat.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
docs/interop/vhost-user.txt | 6 ++++++
1 file changed, 6 insertions(+)
[Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Victor Kaplansky 6 years, 3 months ago
From: Victor Kaplansky <vkaplans@dell9020.localdomain>

If we allow qemu to change logging area after it was already established,
it may require from the backend to acquire a lock on each access to
the log_base, which has a potential quite a big performance hit.

Thus we would like to clarify in the spec, that qemu is not expected
to resize or remap the logging area, and backend implementations
can safely ignore subsequent requests to log_base modifications.

Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 docs/interop/vhost-user.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
index 954771d0d8..7ab31e57ef 100644
--- a/docs/interop/vhost-user.txt
+++ b/docs/interop/vhost-user.txt
@@ -257,6 +257,12 @@ Where addr is the guest physical address.
 
 Use atomic operations, as the log may be concurrently manipulated.
 
+Note that master is not expected to issue more than one VHOST_USER_SET_LOG_BASE
+request before the rings are fully stopped by the master. Thus no modifications
+to log_base address are allowed before the rings are restated and the client
+can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the log_base
+address has been established.
+
 Note that when logging modifications to the used ring (when VHOST_VRING_F_LOG
 is set for this ring), log_guest_addr should be used to calculate the log
 offset: the write to first byte of the used ring is logged at this offset from
-- 
2.14.2

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Dr. David Alan Gilbert 6 years, 3 months ago
* Victor Kaplansky (vkaplans@redhat.com) wrote:
> From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> 
> If we allow qemu to change logging area after it was already established,
> it may require from the backend to acquire a lock on each access to
> the log_base, which has a potential quite a big performance hit.
> 
> Thus we would like to clarify in the spec, that qemu is not expected
> to resize or remap the logging area, and backend implementations
> can safely ignore subsequent requests to log_base modifications.

There's quite a bit of code in vhost to cope with changes in mappings
of regions; and there's already code in there to handle log size
changes:

static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
{
    struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
    uint64_t log_base = (uintptr_t)log->log;
    int r;

    /* inform backend of log switching, this must be done before
       releasing the current log, to ensure no logging is lost */
    r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);

so when is a log resize legal?

Dave

> Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  docs/interop/vhost-user.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index 954771d0d8..7ab31e57ef 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -257,6 +257,12 @@ Where addr is the guest physical address.
>  
>  Use atomic operations, as the log may be concurrently manipulated.
>  
> +Note that master is not expected to issue more than one VHOST_USER_SET_LOG_BASE
> +request before the rings are fully stopped by the master. Thus no modifications
> +to log_base address are allowed before the rings are restated and the client
> +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the log_base
> +address has been established.
> +
>  Note that when logging modifications to the used ring (when VHOST_VRING_F_LOG
>  is set for this ring), log_guest_addr should be used to calculate the log
>  offset: the write to first byte of the used ring is logged at this offset from
> -- 
> 2.14.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Michael S. Tsirkin 6 years, 3 months ago
On Tue, Nov 28, 2017 at 04:04:21PM +0000, Dr. David Alan Gilbert wrote:
> * Victor Kaplansky (vkaplans@redhat.com) wrote:
> > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > 
> > If we allow qemu to change logging area after it was already established,
> > it may require from the backend to acquire a lock on each access to
> > the log_base, which has a potential quite a big performance hit.
> > 
> > Thus we would like to clarify in the spec, that qemu is not expected
> > to resize or remap the logging area, and backend implementations
> > can safely ignore subsequent requests to log_base modifications.
> 
> There's quite a bit of code in vhost to cope with changes in mappings
> of regions; and there's already code in there to handle log size
> changes:
> 
> static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
> {
>     struct vhost_log *log = vhost_log_get(size, vhost_dev_log_is_shared(dev));
>     uint64_t log_base = (uintptr_t)log->log;
>     int r;
> 
>     /* inform backend of log switching, this must be done before
>        releasing the current log, to ensure no logging is lost */
>     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> 
> so when is a log resize legal?
> 
> Dave

Log base has nothing to do with log resize.

It supplies the physical address of the ring
for dirty logging purposes.




> > Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> > Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >  docs/interop/vhost-user.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > index 954771d0d8..7ab31e57ef 100644
> > --- a/docs/interop/vhost-user.txt
> > +++ b/docs/interop/vhost-user.txt
> > @@ -257,6 +257,12 @@ Where addr is the guest physical address.
> >  
> >  Use atomic operations, as the log may be concurrently manipulated.
> >  
> > +Note that master is not expected to issue more than one VHOST_USER_SET_LOG_BASE
> > +request before the rings are fully stopped by the master. Thus no modifications
> > +to log_base address are allowed before the rings are restated and the client
> > +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the log_base
> > +address has been established.
> > +
> >  Note that when logging modifications to the used ring (when VHOST_VRING_F_LOG
> >  is set for this ring), log_guest_addr should be used to calculate the log
> >  offset: the write to first byte of the used ring is logged at this offset from
> > -- 
> > 2.14.2
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Victor Kaplansky 6 years, 3 months ago
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: "Victor Kaplansky" <vkaplans@redhat.com>, qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>
> Sent: Tuesday, November 28, 2017 6:12:44 PM
> Subject: Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
> 
> On Tue, Nov 28, 2017 at 04:04:21PM +0000, Dr. David Alan Gilbert wrote:
> > * Victor Kaplansky (vkaplans@redhat.com) wrote:
> > > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > > 
> > > If we allow qemu to change logging area after it was already established,
> > > it may require from the backend to acquire a lock on each access to
> > > the log_base, which has a potential quite a big performance hit.
> > > 
> > > Thus we would like to clarify in the spec, that qemu is not expected
> > > to resize or remap the logging area, and backend implementations
> > > can safely ignore subsequent requests to log_base modifications.
> > 
> > There's quite a bit of code in vhost to cope with changes in mappings
> > of regions; and there's already code in there to handle log size
> > changes:
> > 
> > static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t
> > size)
> > {
> >     struct vhost_log *log = vhost_log_get(size,
> >     vhost_dev_log_is_shared(dev));
> >     uint64_t log_base = (uintptr_t)log->log;
> >     int r;
> > 
> >     /* inform backend of log switching, this must be done before
> >        releasing the current log, to ensure no logging is lost */
> >     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > 
> > so when is a log resize legal?
> > 
> > Dave
> 
> Log base has nothing to do with log resize.
> 
> It supplies the physical address of the ring
> for dirty logging purposes.
> 

Right. Just for complete picture, it also supplies the address for
dirty logging of where descriptors can point, in other words for logging
whole guest's physical memory. It is why a single log_base is enough for
whole device, and there is no need to specify it for every ring separately.

Having said that, there is an obscure possibility to place the ring in
a memory different from guest's physical memory, but as far as I know there
is no backend that supports this now.

The introduction of this clarification to the vhost-user spec, either with
*must* or without it, will enable us to fix in DPDK several BZs open for qemu
and related to migration failure of MQ vhost-user.

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Dr. David Alan Gilbert 6 years, 3 months ago
* Victor Kaplansky (vkaplans@redhat.com) wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: "Victor Kaplansky" <vkaplans@redhat.com>, qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>
> > Sent: Tuesday, November 28, 2017 6:12:44 PM
> > Subject: Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
> > 
> > On Tue, Nov 28, 2017 at 04:04:21PM +0000, Dr. David Alan Gilbert wrote:
> > > * Victor Kaplansky (vkaplans@redhat.com) wrote:
> > > > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > > > 
> > > > If we allow qemu to change logging area after it was already established,
> > > > it may require from the backend to acquire a lock on each access to
> > > > the log_base, which has a potential quite a big performance hit.
> > > > 
> > > > Thus we would like to clarify in the spec, that qemu is not expected
> > > > to resize or remap the logging area, and backend implementations
> > > > can safely ignore subsequent requests to log_base modifications.
> > > 
> > > There's quite a bit of code in vhost to cope with changes in mappings
> > > of regions; and there's already code in there to handle log size
> > > changes:
> > > 
> > > static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t
> > > size)
> > > {
> > >     struct vhost_log *log = vhost_log_get(size,
> > >     vhost_dev_log_is_shared(dev));
> > >     uint64_t log_base = (uintptr_t)log->log;
> > >     int r;
> > > 
> > >     /* inform backend of log switching, this must be done before
> > >        releasing the current log, to ensure no logging is lost */
> > >     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > > 
> > > so when is a log resize legal?
> > > 
> > > Dave
> > 
> > Log base has nothing to do with log resize.
> > 
> > It supplies the physical address of the ring
> > for dirty logging purposes.
> > 
> 
> Right. Just for complete picture, it also supplies the address for
> dirty logging of where descriptors can point, in other words for logging
> whole guest's physical memory. It is why a single log_base is enough for
> whole device, and there is no need to specify it for every ring separately.
> 
> Having said that, there is an obscure possibility to place the ring in
> a memory different from guest's physical memory, but as far as I know there
> is no backend that supports this now.
> 
> The introduction of this clarification to the vhost-user spec, either with
> *must* or without it, will enable us to fix in DPDK several BZs open for qemu
> and related to migration failure of MQ vhost-user.

OK thanks for the clarification Michael, Victor!

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Michael S. Tsirkin 6 years, 3 months ago
On Tue, Nov 28, 2017 at 05:43:16PM +0000, Dr. David Alan Gilbert wrote:
> * Victor Kaplansky (vkaplans@redhat.com) wrote:
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > Cc: "Victor Kaplansky" <vkaplans@redhat.com>, qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>
> > > Sent: Tuesday, November 28, 2017 6:12:44 PM
> > > Subject: Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
> > > 
> > > On Tue, Nov 28, 2017 at 04:04:21PM +0000, Dr. David Alan Gilbert wrote:
> > > > * Victor Kaplansky (vkaplans@redhat.com) wrote:
> > > > > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > > > > 
> > > > > If we allow qemu to change logging area after it was already established,
> > > > > it may require from the backend to acquire a lock on each access to
> > > > > the log_base, which has a potential quite a big performance hit.
> > > > > 
> > > > > Thus we would like to clarify in the spec, that qemu is not expected
> > > > > to resize or remap the logging area, and backend implementations
> > > > > can safely ignore subsequent requests to log_base modifications.
> > > > 
> > > > There's quite a bit of code in vhost to cope with changes in mappings
> > > > of regions; and there's already code in there to handle log size
> > > > changes:
> > > > 
> > > > static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t
> > > > size)
> > > > {
> > > >     struct vhost_log *log = vhost_log_get(size,
> > > >     vhost_dev_log_is_shared(dev));
> > > >     uint64_t log_base = (uintptr_t)log->log;
> > > >     int r;
> > > > 
> > > >     /* inform backend of log switching, this must be done before
> > > >        releasing the current log, to ensure no logging is lost */
> > > >     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > > > 
> > > > so when is a log resize legal?
> > > > 
> > > > Dave
> > > 
> > > Log base has nothing to do with log resize.
> > > 
> > > It supplies the physical address of the ring
> > > for dirty logging purposes.
> > > 
> > 
> > Right. Just for complete picture, it also supplies the address for
> > dirty logging of where descriptors can point, in other words for logging
> > whole guest's physical memory. It is why a single log_base is enough for
> > whole device, and there is no need to specify it for every ring separately.
> > 
> > Having said that, there is an obscure possibility to place the ring in
> > a memory different from guest's physical memory, but as far as I know there
> > is no backend that supports this now.
> > 
> > The introduction of this clarification to the vhost-user spec, either with
> > *must* or without it, will enable us to fix in DPDK several BZs open for qemu
> > and related to migration failure of MQ vhost-user.
> 
> OK thanks for the clarification Michael, Victor!
> 
> Dave

Sorry, what I said was completely wrong. Was mixing up vhost and
vhost-user.

> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Michael S. Tsirkin 6 years, 3 months ago
On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> 
> If we allow qemu to change logging area after it was already established,
> it may require from the backend to acquire a lock on each access to
> the log_base, which has a potential quite a big performance hit.
> 
> Thus we would like to clarify in the spec, that qemu is not expected
> to resize or remap the logging area, and backend implementations
> can safely ignore subsequent requests to log_base modifications.
> 
> Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  docs/interop/vhost-user.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index 954771d0d8..7ab31e57ef 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -257,6 +257,12 @@ Where addr is the guest physical address.
>  
>  Use atomic operations, as the log may be concurrently manipulated.
>  
> +Note that master is not expected to issue more than one VHOST_USER_SET_LOG_BASE
> +request before the rings are fully stopped by the master. Thus no modifications
> +to log_base address are allowed before the rings are restated and the client
> +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the log_base
> +address has been established.
> +

Do we actually issue VHOST_USER_SET_LOG_BASE when ring is started?

Can we put it simpler:

	master must not send VHOST_USER_SET_LOG_BASE when ring
	is started and logging of used ring writes is started.

?

>  Note that when logging modifications to the used ring (when VHOST_VRING_F_LOG
>  is set for this ring), log_guest_addr should be used to calculate the log
>  offset: the write to first byte of the used ring is logged at this offset from
> -- 
> 2.14.2

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Victor Kaplansky 6 years, 3 months ago
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: "Victor Kaplansky" <vkaplans@redhat.com>
> Cc: qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>
> Sent: Tuesday, November 28, 2017 6:16:32 PM
> Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> 
> On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > 
> > If we allow qemu to change logging area after it was already established,
> > it may require from the backend to acquire a lock on each access to
> > the log_base, which has a potential quite a big performance hit.

> 
> Do we actually issue VHOST_USER_SET_LOG_BASE when ring is started?
> 
> Can we put it simpler:
> 
> 	master must not send VHOST_USER_SET_LOG_BASE when ring
> 	is started and logging of used ring writes is started.
> 
> ?

I'm OK with this phrasing from DPDK point of view, the problem is
that right now qemu issues VHOST_USER_SET_LOG_BASE for each queue
pair, which causes backend to remap logging area, which is very
prone to race conditions without full fledged lock on log_base variable.

The several set_log_base requests by qemu are to the same logging area
with the same size, but a backend cannot know this, since the information is
passed by fd (file descriptor).

So, if we use the word *must* it will *require* a change in QEMU, to
issue single set_log_base for all queue pairs, and not once per queue
pair, as it is done now.

> 
> >  Note that when logging modifications to the used ring (when
> >  VHOST_VRING_F_LOG
> >  is set for this ring), log_guest_addr should be used to calculate the log
> >  offset: the write to first byte of the used ring is logged at this offset
> >  from
> > --
> > 2.14.2
> 

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Michael S. Tsirkin 6 years, 3 months ago
On Tue, Nov 28, 2017 at 12:29:04PM -0500, Victor Kaplansky wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: "Victor Kaplansky" <vkaplans@redhat.com>
> > Cc: qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>
> > Sent: Tuesday, November 28, 2017 6:16:32 PM
> > Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> > 
> > On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> > > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > > 
> > > If we allow qemu to change logging area after it was already established,
> > > it may require from the backend to acquire a lock on each access to
> > > the log_base, which has a potential quite a big performance hit.
> 
> > 
> > Do we actually issue VHOST_USER_SET_LOG_BASE when ring is started?
> > 
> > Can we put it simpler:
> > 
> > 	master must not send VHOST_USER_SET_LOG_BASE when ring
> > 	is started and logging of used ring writes is started.
> > 
> > ?
> 
> I'm OK with this phrasing from DPDK point of view, the problem is
> that right now qemu issues VHOST_USER_SET_LOG_BASE for each queue
> pair, which causes backend to remap logging area, which is very
> prone to race conditions without full fledged lock on log_base variable.
> 
> The several set_log_base requests by qemu are to the same logging area
> with the same size, but a backend cannot know this, since the information is
> passed by fd (file descriptor).
> 
> So, if we use the word *must* it will *require* a change in QEMU, to
> issue single set_log_base for all queue pairs, and not once per queue
> pair, as it is done now.

Oh I was confusing VHOST_USER_SET_LOG_BASE with VHOST_SET_LOG_BASE.

Sorry, pls disregard.




> > 
> > >  Note that when logging modifications to the used ring (when
> > >  VHOST_VRING_F_LOG
> > >  is set for this ring), log_guest_addr should be used to calculate the log
> > >  offset: the write to first byte of the used ring is logged at this offset
> > >  from
> > > --
> > > 2.14.2
> > 

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Victor Kaplansky 6 years, 3 months ago
> > +Note that master is not expected to issue more than one
> > VHOST_USER_SET_LOG_BASE
> > +request before the rings are fully stopped by the master. Thus no
> > modifications
> > +to log_base address are allowed before the rings are restated and the
> > client
> > +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the
> > log_base
> > +address has been established.
> > +
> 
> Do we actually issue VHOST_USER_SET_LOG_BASE when ring is started?

Sorry, missed this question in the previous answer.
Yes, actually qemu issues set_log_base() only in the process of migration,
and before that rings are enabled and actively updated.

> 
> Can we put it simpler:
> 
> 	master must not send VHOST_USER_SET_LOG_BASE when ring
> 	is started and logging of used ring writes is started.
> 
> ?

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Michael S. Tsirkin 6 years, 3 months ago
On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> 
> If we allow qemu to change logging area after it was already established,
> it may require from the backend to acquire a lock on each access to
> the log_base, which has a potential quite a big performance hit.
> 
> Thus we would like to clarify in the spec, that qemu is not expected
> to resize or remap the logging area, and backend implementations
> can safely ignore subsequent requests to log_base modifications.
> 
> Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I'm not sure we can do this.


Log resizing as a result of memory hotplug might force
log base changes.

Backends need to use something like
rcu to avoid need for locking.

Apropos I wonder whether it's a bug that vhost_dev_start
calls vhost_set_log_base after starting rings.

Same question for the iotlb callback.




> ---
>  docs/interop/vhost-user.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index 954771d0d8..7ab31e57ef 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -257,6 +257,12 @@ Where addr is the guest physical address.
>  
>  Use atomic operations, as the log may be concurrently manipulated.
>  
> +Note that master is not expected to issue more than one VHOST_USER_SET_LOG_BASE
> +request before the rings are fully stopped by the master. Thus no modifications
> +to log_base address are allowed before the rings are restated and the client
> +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the log_base
> +address has been established.
> +
>  Note that when logging modifications to the used ring (when VHOST_VRING_F_LOG
>  is set for this ring), log_guest_addr should be used to calculate the log
>  offset: the write to first byte of the used ring is logged at this offset from
> -- 
> 2.14.2

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Victor Kaplansky 6 years, 3 months ago
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: "Victor Kaplansky" <vkaplans@redhat.com>
> Cc: qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>, "Jason Wang" <jasowang@redhat.com>
> Sent: Tuesday, November 28, 2017 8:06:52 PM
> Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> 
> On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > 
> > If we allow qemu to change logging area after it was already established,
> > it may require from the backend to acquire a lock on each access to
> > the log_base, which has a potential quite a big performance hit.
> > 
> > Thus we would like to clarify in the spec, that qemu is not expected
> > to resize or remap the logging area, and backend implementations
> > can safely ignore subsequent requests to log_base modifications.
> > 
> > Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> > Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I'm not sure we can do this.
> 
> 
> Log resizing as a result of memory hotplug might force
> log base changes.

I agree. Memory hotplug during live migration will cause a
bunch of problems not only to dirty page logging, but to a
regular backend function, because memory regions have to be
redefined before descriptors pointing to the new memory
arrive a backend. Which means, that in DPDK we must to
guard by some kind of synchronization mechanism all accesses
to guest2qemu memory translation. This will have an impact
on the backend performace even with light-weighted RCU.

So, I propose to handle memory hot plug by stopping rings,
then changing memory regions and log_base, and then re-enabling
rings.

What do you say?


> 
> Backends need to use something like
> rcu to avoid need for locking.
> 
> Apropos I wonder whether it's a bug that vhost_dev_start
> calls vhost_set_log_base after starting rings.

In vhost-user backend before vhost_user_set_log_base is called for first
time, log_base is initialized to zero. Which causes backend (DPDK) to
skip logging. Thus theoretically setting log_base on an active ring,
may cause to skip single (very first) dirty logging. In the reality,
it has very small chances to happen, since sending the response from
the backend to qemu by a socket takes much longer then a time gap
between reading log_base variable and using it.


> 
> Same question for the iotlb callback.
> 
> 
> 
> 
> > ---
> >  docs/interop/vhost-user.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > index 954771d0d8..7ab31e57ef 100644
> > --- a/docs/interop/vhost-user.txt
> > +++ b/docs/interop/vhost-user.txt
> > @@ -257,6 +257,12 @@ Where addr is the guest physical address.
> >  
> >  Use atomic operations, as the log may be concurrently manipulated.
> >  
> > +Note that master is not expected to issue more than one
> > VHOST_USER_SET_LOG_BASE
> > +request before the rings are fully stopped by the master. Thus no
> > modifications
> > +to log_base address are allowed before the rings are restated and the
> > client
> > +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the
> > log_base
> > +address has been established.
> > +
> >  Note that when logging modifications to the used ring (when
> >  VHOST_VRING_F_LOG
> >  is set for this ring), log_guest_addr should be used to calculate the log
> >  offset: the write to first byte of the used ring is logged at this offset
> >  from
> > --
> > 2.14.2
> 

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Michael S. Tsirkin 6 years, 3 months ago
On Tue, Nov 28, 2017 at 01:34:19PM -0500, Victor Kaplansky wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > To: "Victor Kaplansky" <vkaplans@redhat.com>
> > Cc: qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>, "Jason Wang" <jasowang@redhat.com>
> > Sent: Tuesday, November 28, 2017 8:06:52 PM
> > Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> > 
> > On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> > > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > > 
> > > If we allow qemu to change logging area after it was already established,
> > > it may require from the backend to acquire a lock on each access to
> > > the log_base, which has a potential quite a big performance hit.
> > > 
> > > Thus we would like to clarify in the spec, that qemu is not expected
> > > to resize or remap the logging area, and backend implementations
> > > can safely ignore subsequent requests to log_base modifications.
> > > 
> > > Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> > > Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > 
> > I'm not sure we can do this.
> > 
> > 
> > Log resizing as a result of memory hotplug might force
> > log base changes.
> 
> I agree. Memory hotplug during live migration will cause a
> bunch of problems not only to dirty page logging, but to a
> regular backend function, because memory regions have to be
> redefined before descriptors pointing to the new memory
> arrive a backend. Which means, that in DPDK we must to
> guard by some kind of synchronization mechanism all accesses
> to guest2qemu memory translation. This will have an impact
> on the backend performace even with light-weighted RCU.
> 
> So, I propose to handle memory hot plug by stopping rings,
> then changing memory regions and log_base, and then re-enabling
> rings.
> 
> What do you say?

Why not have backend do this internally when it gets
the log_base message?


> 
> > 
> > Backends need to use something like
> > rcu to avoid need for locking.
> > 
> > Apropos I wonder whether it's a bug that vhost_dev_start
> > calls vhost_set_log_base after starting rings.
> 
> In vhost-user backend before vhost_user_set_log_base is called for first
> time, log_base is initialized to zero. Which causes backend (DPDK) to
> skip logging. Thus theoretically setting log_base on an active ring,
> may cause to skip single (very first) dirty logging. In the reality,
> it has very small chances to happen, since sending the response from
> the backend to qemu by a socket takes much longer then a time gap
> between reading log_base variable and using it.

I suspect it's worth fixing though.


> 
> > 
> > Same question for the iotlb callback.
> > 
> > 
> > 
> > 
> > > ---
> > >  docs/interop/vhost-user.txt | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > > index 954771d0d8..7ab31e57ef 100644
> > > --- a/docs/interop/vhost-user.txt
> > > +++ b/docs/interop/vhost-user.txt
> > > @@ -257,6 +257,12 @@ Where addr is the guest physical address.
> > >  
> > >  Use atomic operations, as the log may be concurrently manipulated.
> > >  
> > > +Note that master is not expected to issue more than one
> > > VHOST_USER_SET_LOG_BASE
> > > +request before the rings are fully stopped by the master. Thus no
> > > modifications
> > > +to log_base address are allowed before the rings are restated and the
> > > client
> > > +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the
> > > log_base
> > > +address has been established.
> > > +
> > >  Note that when logging modifications to the used ring (when
> > >  VHOST_VRING_F_LOG
> > >  is set for this ring), log_guest_addr should be used to calculate the log
> > >  offset: the write to first byte of the used ring is logged at this offset
> > >  from
> > > --
> > > 2.14.2
> > 

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Victor Kaplansky 6 years, 3 months ago
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: "Victor Kaplansky" <vkaplans@redhat.com>
> Cc: qemu-devel@nongnu.org, "Maxime Coquelin" <maxime.coquelin@redhat.com>, "Jason Wang" <jasowang@redhat.com>
> Sent: Tuesday, November 28, 2017 8:45:43 PM
> Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> 
> On Tue, Nov 28, 2017 at 01:34:19PM -0500, Victor Kaplansky wrote:
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > To: "Victor Kaplansky" <vkaplans@redhat.com>
> > > Cc: qemu-devel@nongnu.org, "Maxime Coquelin"
> > > <maxime.coquelin@redhat.com>, "Jason Wang" <jasowang@redhat.com>
> > > Sent: Tuesday, November 28, 2017 8:06:52 PM
> > > Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> > > 
> > > On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> > > > From: Victor Kaplansky <vkaplans@dell9020.localdomain>
> > > > 
> > > > If we allow qemu to change logging area after it was already
> > > > established,
> > > > it may require from the backend to acquire a lock on each access to
> > > > the log_base, which has a potential quite a big performance hit.
> > > > 
> > > > Thus we would like to clarify in the spec, that qemu is not expected
> > > > to resize or remap the logging area, and backend implementations
> > > > can safely ignore subsequent requests to log_base modifications.
> > > > 
> > > > Signed-off-by: Victor Kaplansky <vkaplans@redhat.com>
> > > > Suggested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > 
> > > I'm not sure we can do this.
> > > 
> > > 
> > > Log resizing as a result of memory hotplug might force
> > > log base changes.
> > 
> > I agree. Memory hotplug during live migration will cause a
> > bunch of problems not only to dirty page logging, but to a
> > regular backend function, because memory regions have to be
> > redefined before descriptors pointing to the new memory
> > arrive a backend. Which means, that in DPDK we must to
> > guard by some kind of synchronization mechanism all accesses
> > to guest2qemu memory translation. This will have an impact
> > on the backend performace even with light-weighted RCU.
> > 
> > So, I propose to handle memory hot plug by stopping rings,
> > then changing memory regions and log_base, and then re-enabling
> > rings.
> > 
> > What do you say?
> 
> Why not have backend do this internally when it gets
> the log_base message?
> 
> 
> > 
> > > 
> > > Backends need to use something like
> > > rcu to avoid need for locking.
> > > 
> > > Apropos I wonder whether it's a bug that vhost_dev_start
> > > calls vhost_set_log_base after starting rings.
> > 
> > In vhost-user backend before vhost_user_set_log_base is called for first
> > time, log_base is initialized to zero. Which causes backend (DPDK) to
> > skip logging. Thus theoretically setting log_base on an active ring,
> > may cause to skip single (very first) dirty logging. In the reality,
> > it has very small chances to happen, since sending the response from
> > the backend to qemu by a socket takes much longer then a time gap
> > between reading log_base variable and using it.
> 
> I suspect it's worth fixing though.
> 

Yes, this case maybe not worth to bother, but I'm thinking about all
other race conditions between the request handling thread and the data
path in DPDK, and my conclusion is that we will have to add some general
rcu guard between all requests and data path operations.
I'll send a patch to DPDK.

Anyway, it may be worth to clean up things in qemu as well and
avoid sending unnecessary requests to backend slave.

> 
> > 
> > > 
> > > Same question for the iotlb callback.
> > > 
> > > 
> > > 
> > > 
> > > > ---
> > > >  docs/interop/vhost-user.txt | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > > > index 954771d0d8..7ab31e57ef 100644
> > > > --- a/docs/interop/vhost-user.txt
> > > > +++ b/docs/interop/vhost-user.txt
> > > > @@ -257,6 +257,12 @@ Where addr is the guest physical address.
> > > >  
> > > >  Use atomic operations, as the log may be concurrently manipulated.
> > > >  
> > > > +Note that master is not expected to issue more than one
> > > > VHOST_USER_SET_LOG_BASE
> > > > +request before the rings are fully stopped by the master. Thus no
> > > > modifications
> > > > +to log_base address are allowed before the rings are restated and the
> > > > client
> > > > +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the
> > > > log_base
> > > > +address has been established.
> > > > +
> > > >  Note that when logging modifications to the used ring (when
> > > >  VHOST_VRING_F_LOG
> > > >  is set for this ring), log_guest_addr should be used to calculate the
> > > >  log
> > > >  offset: the write to first byte of the used ring is logged at this
> > > >  offset
> > > >  from
> > > > --
> > > > 2.14.2
> > > 
> 

Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base
Posted by Jason Wang 6 years, 3 months ago

On 2017年11月29日 02:06, Michael S. Tsirkin wrote:
> On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
>> From: Victor Kaplansky<vkaplans@dell9020.localdomain>
>>
>> If we allow qemu to change logging area after it was already established,
>> it may require from the backend to acquire a lock on each access to
>> the log_base, which has a potential quite a big performance hit.
>>
>> Thus we would like to clarify in the spec, that qemu is not expected
>> to resize or remap the logging area, and backend implementations
>> can safely ignore subsequent requests to log_base modifications.
>>
>> Signed-off-by: Victor Kaplansky<vkaplans@redhat.com>
>> Suggested-by: Maxime Coquelin<maxime.coquelin@redhat.com>
> I'm not sure we can do this.
>
>
> Log resizing as a result of memory hotplug might force
> log base changes.
>
> Backends need to use something like
> rcu to avoid need for locking.
>
> Apropos I wonder whether it's a bug that vhost_dev_start
> calls vhost_set_log_base after starting rings.
>
> Same question for the iotlb callback.

At least for kernel vhost-net, it doesn't since the backend won't start 
to work until SET_BACKEND which is done in vhost_net_start_one() after 
vhost_dev_start().

Thanks