Linux virtualization list
 help / color / mirror / Atom feed
* [PATCH] tools/virtio: fix byteswap logic
From: Michael S. Tsirkin @ 2015-11-30  8:33 UTC (permalink / raw)
  To: linux-kernel; +Cc: virtualization

commit cf561f0d2eb74574ad9985a2feab134267a9d298 ("virtio: introduce
virtio_is_little_endian() helper") changed byteswap logic to
skip feature bit checks for LE platforms, but didn't
update tools/virtio, so vring_bench started failing.

Update the copy under tools/virtio/ (TODO: find a way to avoid this code
duplication).

Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tools/virtio/linux/virtio_config.h | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/virtio/linux/virtio_config.h b/tools/virtio/linux/virtio_config.h
index 806d683..57a6964 100644
--- a/tools/virtio/linux/virtio_config.h
+++ b/tools/virtio/linux/virtio_config.h
@@ -40,33 +40,39 @@ static inline void __virtio_clear_bit(struct virtio_device *vdev,
 #define virtio_has_feature(dev, feature) \
 	(__virtio_test_bit((dev), feature))
 
+static inline bool virtio_is_little_endian(struct virtio_device *vdev)
+{
+	return virtio_has_feature(vdev, VIRTIO_F_VERSION_1) ||
+		virtio_legacy_is_little_endian();
+}
+
+/* Memory accessors */
 static inline u16 virtio16_to_cpu(struct virtio_device *vdev, __virtio16 val)
 {
-	return __virtio16_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __virtio16_to_cpu(virtio_is_little_endian(vdev), val);
 }
 
 static inline __virtio16 cpu_to_virtio16(struct virtio_device *vdev, u16 val)
 {
-	return __cpu_to_virtio16(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio16(virtio_is_little_endian(vdev), val);
 }
 
 static inline u32 virtio32_to_cpu(struct virtio_device *vdev, __virtio32 val)
 {
-	return __virtio32_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __virtio32_to_cpu(virtio_is_little_endian(vdev), val);
 }
 
 static inline __virtio32 cpu_to_virtio32(struct virtio_device *vdev, u32 val)
 {
-	return __cpu_to_virtio32(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio32(virtio_is_little_endian(vdev), val);
 }
 
 static inline u64 virtio64_to_cpu(struct virtio_device *vdev, __virtio64 val)
 {
-	return __virtio64_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __virtio64_to_cpu(virtio_is_little_endian(vdev), val);
 }
 
 static inline __virtio64 cpu_to_virtio64(struct virtio_device *vdev, u64 val)
 {
-	return __cpu_to_virtio64(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), val);
+	return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
 }
-
-- 
MST

^ permalink raw reply related

* Re: [PATCH net-next 0/3] basic busy polling support for vhost_net
From: Michael S. Tsirkin @ 2015-11-30  8:22 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20151129.223110.1579432138646337508.davem@davemloft.net>

On Sun, Nov 29, 2015 at 10:31:10PM -0500, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Wed, 25 Nov 2015 15:11:26 +0800
> 
> > This series tries to add basic busy polling for vhost net. The idea is
> > simple: at the end of tx/rx processing, busy polling for new tx added
> > descriptor and rx receive socket for a while. The maximum number of
> > time (in us) could be spent on busy polling was specified ioctl.
> > 
> > Test A were done through:
> > 
> > - 50 us as busy loop timeout
> > - Netperf 2.6
> > - Two machines with back to back connected ixgbe
> > - Guest with 1 vcpu and 1 queue
> > 
> > Results:
> > - For stream workload, ioexits were reduced dramatically in medium
> >   size (1024-2048) of tx (at most -43%) and almost all rx (at most
> >   -84%) as a result of polling. This compensate for the possible
> >   wasted cpu cycles more or less. That porbably why we can still see
> >   some increasing in the normalized throughput in some cases.
> > - Throughput of tx were increased (at most 50%) expect for the huge
> >   write (16384). And we can send more packets in the case (+tpkts were
> >   increased).
> > - Very minor rx regression in some cases.
> > - Improvemnt on TCP_RR (at most 17%).
> 
> Michael are you going to take this?  It's touching vhost core as
> much as it is the vhost_net driver.

There's a minor bug there, but once it's fixed - I agree,
it belongs in the vhost tree.

-- 
MST

^ permalink raw reply

* Re: [PATCH net-next 2/3] vhost: introduce vhost_vq_more_avail()
From: Michael S. Tsirkin @ 2015-11-30  8:22 UTC (permalink / raw)
  To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1448435489-5949-3-git-send-email-jasowang@redhat.com>

On Wed, Nov 25, 2015 at 03:11:28PM +0800, Jason Wang wrote:
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vhost/vhost.c | 26 +++++++++++++++++---------
>  drivers/vhost/vhost.h |  1 +
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 163b365..b86c5aa 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1633,10 +1633,25 @@ void vhost_add_used_and_signal_n(struct vhost_dev *dev,
>  }
>  EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
>  
> +bool vhost_vq_more_avail(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> +{
> +	__virtio16 avail_idx;
> +	int r;
> +
> +	r = __get_user(avail_idx, &vq->avail->idx);
> +	if (r) {
> +		vq_err(vq, "Failed to check avail idx at %p: %d\n",
> +		       &vq->avail->idx, r);
> +		return false;

In patch 3 you are calling this under preempt disable,
so this actually can fail and it isn't a VQ error.

> +	}
> +
> +	return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
> +}
> +EXPORT_SYMBOL_GPL(vhost_vq_more_avail);
> +
>  /* OK, now we need to know about added descriptors. */
>  bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>  {
> -	__virtio16 avail_idx;
>  	int r;
>  
>  	if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
> @@ -1660,14 +1675,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>  	/* They could have slipped one in as we were doing that: make
>  	 * sure it's written, then check again. */
>  	smp_mb();
> -	r = __get_user(avail_idx, &vq->avail->idx);
> -	if (r) {
> -		vq_err(vq, "Failed to check avail idx at %p: %d\n",
> -		       &vq->avail->idx, r);
> -		return false;
> -	}
> -
> -	return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
> +	return vhost_vq_more_avail(dev, vq);
>  }
>  EXPORT_SYMBOL_GPL(vhost_enable_notify);
>  

This path does need an error though.
It's probably easier to just leave this call site alone.

> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 43284ad..2f3c57c 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -159,6 +159,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *, struct vhost_virtqueue *,
>  			       struct vring_used_elem *heads, unsigned count);
>  void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
>  void vhost_disable_notify(struct vhost_dev *, struct vhost_virtqueue *);
> +bool vhost_vq_more_avail(struct vhost_dev *, struct vhost_virtqueue *);
>  bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
>  
>  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> -- 
> 2.5.0

^ permalink raw reply

* Re: [PATCH net-next 0/3] basic busy polling support for vhost_net
From: David Miller @ 2015-11-30  3:31 UTC (permalink / raw)
  To: jasowang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1448435489-5949-1-git-send-email-jasowang@redhat.com>

From: Jason Wang <jasowang@redhat.com>
Date: Wed, 25 Nov 2015 15:11:26 +0800

> This series tries to add basic busy polling for vhost net. The idea is
> simple: at the end of tx/rx processing, busy polling for new tx added
> descriptor and rx receive socket for a while. The maximum number of
> time (in us) could be spent on busy polling was specified ioctl.
> 
> Test A were done through:
> 
> - 50 us as busy loop timeout
> - Netperf 2.6
> - Two machines with back to back connected ixgbe
> - Guest with 1 vcpu and 1 queue
> 
> Results:
> - For stream workload, ioexits were reduced dramatically in medium
>   size (1024-2048) of tx (at most -43%) and almost all rx (at most
>   -84%) as a result of polling. This compensate for the possible
>   wasted cpu cycles more or less. That porbably why we can still see
>   some increasing in the normalized throughput in some cases.
> - Throughput of tx were increased (at most 50%) expect for the huge
>   write (16384). And we can send more packets in the case (+tpkts were
>   increased).
> - Very minor rx regression in some cases.
> - Improvemnt on TCP_RR (at most 17%).

Michael are you going to take this?  It's touching vhost core as
much as it is the vhost_net driver.

^ permalink raw reply

* Re: [PATCH 2/2] remoteproc: fix memory leak of remoteproc ida cache layers
From: Ohad Ben-Cohen @ 2015-11-26 15:47 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Suman Anna, linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <20151126103700.GA13200@redhat.com>

On Thu, Nov 26, 2015 at 12:37 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Nov 26, 2015 at 11:38:06AM +0200, Ohad Ben-Cohen wrote:
>> I saw there was some discussion about this between Michael, James and
>> Tejun whether this should be fixed in the IDA core or not.
>>
>> Has it been resolved?
>
> I don't think we reached any conclusions.
> I guess I'll merge the virtio patch as-is then.
> Ohad, would you like to merge 2/2?

Sure, applied 2/2 to remoteproc-fixes. Thanks!

^ permalink raw reply

* Re: [PATCH 2/2] remoteproc: fix memory leak of remoteproc ida cache layers
From: Michael S. Tsirkin @ 2015-11-26 10:37 UTC (permalink / raw)
  To: Ohad Ben-Cohen; +Cc: Suman Anna, linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <CAK=WgbaYdUU8o-iAqJAOuTd_cXzkcYQ0Nt14beuZ=PwtXGtV3A@mail.gmail.com>

On Thu, Nov 26, 2015 at 11:38:06AM +0200, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Thu, Sep 17, 2015 at 3:29 AM, Suman Anna <s-anna@ti.com> wrote:
> > The remoteproc core uses a static ida named rproc_dev_index for
> > assigning an automatic index number to a registered remoteproc.
> > The ida core may allocate some internal idr cache layers and ida
> > bitmap upon any ida allocation, and all these layers are truely
> > freed only upon the ida destruction. The rproc_dev_index ida is
> > not destroyed at present, leading to a memory leak when using the
> > remoteproc core as a module and atleast one rproc device is
> > registered and unregistered.
> >
> > Fix this by invoking ida_destroy() in the remoteproc core module
> > exit.
> 
> I saw there was some discussion about this between Michael, James and
> Tejun whether this should be fixed in the IDA core or not.
> 
> Has it been resolved?
> 
> Thanks,
> Ohad.

I don't think we reached any conclusions.
I guess I'll merge the virtio patch as-is then.
Ohad, would you like to merge 2/2?

-- 
MST

^ permalink raw reply

* Re: [PATCH 2/2] remoteproc: fix memory leak of remoteproc ida cache layers
From: Ohad Ben-Cohen @ 2015-11-26  9:38 UTC (permalink / raw)
  To: Suman Anna
  Cc: virtualization, linux-kernel@vger.kernel.org, Michael S. Tsirkin
In-Reply-To: <1442449758-14594-3-git-send-email-s-anna@ti.com>

Hi Suman,

On Thu, Sep 17, 2015 at 3:29 AM, Suman Anna <s-anna@ti.com> wrote:
> The remoteproc core uses a static ida named rproc_dev_index for
> assigning an automatic index number to a registered remoteproc.
> The ida core may allocate some internal idr cache layers and ida
> bitmap upon any ida allocation, and all these layers are truely
> freed only upon the ida destruction. The rproc_dev_index ida is
> not destroyed at present, leading to a memory leak when using the
> remoteproc core as a module and atleast one rproc device is
> registered and unregistered.
>
> Fix this by invoking ida_destroy() in the remoteproc core module
> exit.

I saw there was some discussion about this between Michael, James and
Tejun whether this should be fixed in the IDA core or not.

Has it been resolved?

Thanks,
Ohad.

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Paolo Bonzini @ 2015-11-25 19:32 UTC (permalink / raw)
  To: Ming Lin; +Cc: virtualization, qemu-devel, linux-nvme, Christoph Hellwig
In-Reply-To: <1448477482.6540.3.camel@ssi>



On 25/11/2015 19:51, Ming Lin wrote:
> > Do you still have a blk_set_aio_context somewhere?  I'm losing track of
> > the changes.
> 
> No.

You'll need it.  That's what causes your error.

> BTW, I'm not sure about qemu upstream policy.
> Do I need to first make the kernel side patch upstream?
> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext

No problem, there's time.  QEMU is in freeze right now, and the 2.6
cycle will last until March or so.

Paolo

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Ming Lin @ 2015-11-25 18:51 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <56559B36.90406@redhat.com>

On Wed, 2015-11-25 at 12:27 +0100, Paolo Bonzini wrote:
> Do you still have a blk_set_aio_context somewhere?  I'm losing track of
> the changes.

No.

> 
> In any case, I think using a separate I/O thread is a bit premature,
> except for benchmarking.  In the meanwhile I think the best option is to
> post a two-patch series with the vendor extension and the ioeventfd
> respectively.

I'll post.

BTW, I'm not sure about qemu upstream policy.
Do I need to first make the kernel side patch upstream?
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext

I think the kernel side patch will take a bit longer time to upstream.

> 
> Paolo

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Paolo Bonzini @ 2015-11-25 11:27 UTC (permalink / raw)
  To: Ming Lin; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <1448393115.2925.2.camel@ssi>



On 24/11/2015 20:25, Ming Lin wrote:
> On Tue, 2015-11-24 at 11:51 +0100, Paolo Bonzini wrote:
>>
>> On 24/11/2015 08:27, Ming Lin wrote:
>>> handle_notify (qemu/hw/block/dataplane/virtio-blk.c:126)
>>> aio_dispatch (qemu/aio-posix.c:329)
>>> aio_poll (qemu/aio-posix.c:474)
>>> iothread_run (qemu/iothread.c:45)
>>> start_thread (pthread_create.c:312)
>>> /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)
>>>
>>> I think I'll have a "nvme_dev_notify" similar as "handle_notify"
>>>
>>> static void nvme_dev_notify(EventNotifier *e)
>>> {
>>>     ....
>>> }
>>>
>>> But then how can I know this notify is for cq or sq?
>>
>> virtio-blk has a single queue, so it has a single EventNotifier.  Your
>> code using multiple EventNotifiers is fine, you can use
>> aio_set_event_notifier multiple times on the same iothread.
> 
> I feel below patch is close to right.
> But I got "Co-routine re-entered recursively".
> 
> Program received signal SIGABRT, Aborted.
> 0x00007ffff4a45cc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> 56	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> (gdb) bt
> #0  0x00007ffff4a45cc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff4a490d8 in __GI_abort () at abort.c:89
> #2  0x0000555555b910d1 in qemu_coroutine_enter (co=0x5555577a9ce0, opaque=0x0) at /home/mlin/qemu/util/qemu-coroutine.c:111
> #3  0x0000555555b282e0 in bdrv_co_io_em_complete (opaque=0x7fffd94e4a30, ret=0) at /home/mlin/qemu/block/io.c:2282
> #4  0x0000555555ab8ba4 in thread_pool_completion_bh (opaque=0x555557d6d440) at /home/mlin/qemu/thread-pool.c:187
> #5  0x0000555555ab7ab2 in aio_bh_call (bh=0x555557648110) at /home/mlin/qemu/async.c:64
> #6  0x0000555555ab7b88 in aio_bh_poll (ctx=0x5555565b28f0) at /home/mlin/qemu/async.c:92
> #7  0x0000555555acb3b6 in aio_dispatch (ctx=0x5555565b28f0) at /home/mlin/qemu/aio-posix.c:305
> #8  0x0000555555ab8013 in aio_ctx_dispatch (source=0x5555565b28f0, callback=0x0, user_data=0x0) at /home/mlin/qemu/async.c:231
> #9  0x00007ffff575ee04 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #10 0x0000555555ac8b35 in glib_pollfds_poll () at /home/mlin/qemu/main-loop.c:211
> #11 0x0000555555ac8c36 in os_host_main_loop_wait (timeout=0) at /home/mlin/qemu/main-loop.c:256
> #12 0x0000555555ac8cff in main_loop_wait (nonblocking=0) at /home/mlin/qemu/main-loop.c:504
> #13 0x00005555558a41d4 in main_loop () at /home/mlin/qemu/vl.c:1920
> #14 0x00005555558ac28a in main (argc=21, argv=0x7fffffffe4a8, envp=0x7fffffffe558) at /home/mlin/qemu/vl.c:4681
> (gdb)
> 
> Would you help to take a look?
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f27fd35..f542740 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -28,6 +28,8 @@
>  #include "sysemu/sysemu.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
> +#include "qom/object_interfaces.h"
>  
>  #include "nvme.h"
>  
> @@ -549,7 +551,10 @@ static void nvme_cq_notifier(EventNotifier *e)
>          container_of(e, NvmeCQueue, notifier);
>  
>      event_notifier_test_and_clear(&cq->notifier);
> +
> +    aio_context_acquire(cq->ctrl->ctx);
>      nvme_post_cqes(cq);
> +    aio_context_release(cq->ctrl->ctx);

This is not yet needed in upstream.

>  }
>  
>  static void nvme_init_cq_eventfd(NvmeCQueue *cq)
> @@ -558,9 +563,12 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
>      uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
>  
>      event_notifier_init(&cq->notifier, 0);
> -    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
>      memory_region_add_eventfd(&n->iomem,
>          0x1000 + offset, 4, false, 0, &cq->notifier);
> +    aio_context_acquire(n->ctx);
> +    aio_set_event_notifier(n->ctx, &cq->notifier, false,

This should be true, but shouldn't affect your bug.

> +                           nvme_cq_notifier);
> +    aio_context_release(n->ctx);
>  }
>  
>  static void nvme_sq_notifier(EventNotifier *e)
> @@ -569,7 +577,9 @@ static void nvme_sq_notifier(EventNotifier *e)
>          container_of(e, NvmeSQueue, notifier);
>  
>      event_notifier_test_and_clear(&sq->notifier);
> +    aio_context_acquire(sq->ctrl->ctx);
>      nvme_process_sq(sq);
> +    aio_context_release(sq->ctrl->ctx);

Same as above.

>  }
>  
>  static void nvme_init_sq_eventfd(NvmeSQueue *sq)
> @@ -578,9 +588,22 @@ static void nvme_init_sq_eventfd(NvmeSQueue *sq)
>      uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
>  
>      event_notifier_init(&sq->notifier, 0);
> -    event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
>      memory_region_add_eventfd(&n->iomem,
>          0x1000 + offset, 4, false, 0, &sq->notifier);
> +    aio_context_acquire(n->ctx);
> +    aio_set_event_notifier(n->ctx, &sq->notifier, false,

Also true.

> +                           nvme_sq_notifier);
> +    aio_context_release(n->ctx);
> +}
> +
> +static void nvme_init_iothread(NvmeCtrl *n)
> +{
> +    object_initialize(&n->internal_iothread_obj,
> +                      sizeof(n->internal_iothread_obj),
> +                      TYPE_IOTHREAD);
> +    user_creatable_complete(OBJECT(&n->internal_iothread_obj), &error_abort);
> +    n->iothread = &n->internal_iothread_obj;
> +    n->ctx = iothread_get_aio_context(n->iothread);

Do you still have a blk_set_aio_context somewhere?  I'm losing track of
the changes.

In any case, I think using a separate I/O thread is a bit premature,
except for benchmarking.  In the meanwhile I think the best option is to
post a two-patch series with the vendor extension and the ioeventfd
respectively.

Paolo

>  }
>  
>  static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
> @@ -595,6 +618,8 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
>          return NVME_INVALID_MEMORY_ADDRESS | NVME_DNR;
>      }
>  
> +    nvme_init_iothread(n);
> +
>      /* This assumes all I/O queues are created before this command is handled.
>       * We skip the admin queues. */
>      for (i = 1; i < n->num_queues; i++) {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 608f202..b53e69d 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -727,6 +727,10 @@ typedef struct NvmeCtrl {
>      NvmeSQueue      admin_sq;
>      NvmeCQueue      admin_cq;
>      NvmeIdCtrl      id_ctrl;
> +
> +    IOThread *iothread;
> +    IOThread internal_iothread_obj;
> +    AioContext *ctx;
>  } NvmeCtrl;
>  
>  #endif /* HW_NVME_H */
> 
> 

^ permalink raw reply

* [PATCH net-next 3/3] vhost_net: basic polling support
From: Jason Wang @ 2015-11-25  7:11 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1448435489-5949-1-git-send-email-jasowang@redhat.com>

This patch tries to poll for new added tx buffer or socket receive
queue for a while at the end of tx/rx processing. The maximum time
spent on polling were specified through a new kind of vring ioctl.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c        | 72 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/vhost/vhost.c      | 15 ++++++++++
 drivers/vhost/vhost.h      |  1 +
 include/uapi/linux/vhost.h | 11 +++++++
 4 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9eda69e..ce6da77 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -287,6 +287,41 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
 	rcu_read_unlock_bh();
 }
 
+static inline unsigned long busy_clock(void)
+{
+	return local_clock() >> 10;
+}
+
+static bool vhost_can_busy_poll(struct vhost_dev *dev,
+				unsigned long endtime)
+{
+	return likely(!need_resched()) &&
+	       likely(!time_after(busy_clock(), endtime)) &&
+	       likely(!signal_pending(current)) &&
+	       !vhost_has_work(dev) &&
+	       single_task_running();
+}
+
+static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
+				    struct vhost_virtqueue *vq,
+				    struct iovec iov[], unsigned int iov_size,
+				    unsigned int *out_num, unsigned int *in_num)
+{
+	unsigned long uninitialized_var(endtime);
+
+	if (vq->busyloop_timeout) {
+		preempt_disable();
+		endtime = busy_clock() + vq->busyloop_timeout;
+		while (vhost_can_busy_poll(vq->dev, endtime) &&
+		       !vhost_vq_more_avail(vq->dev, vq))
+			cpu_relax();
+		preempt_enable();
+	}
+
+	return vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+				 out_num, in_num, NULL, NULL);
+}
+
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
 static void handle_tx(struct vhost_net *net)
@@ -331,10 +366,9 @@ static void handle_tx(struct vhost_net *net)
 			      % UIO_MAXIOV == nvq->done_idx))
 			break;
 
-		head = vhost_get_vq_desc(vq, vq->iov,
-					 ARRAY_SIZE(vq->iov),
-					 &out, &in,
-					 NULL, NULL);
+		head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
+						ARRAY_SIZE(vq->iov),
+						&out, &in);
 		/* On error, stop handling until the next kick. */
 		if (unlikely(head < 0))
 			break;
@@ -435,6 +469,34 @@ static int peek_head_len(struct sock *sk)
 	return len;
 }
 
+static int vhost_net_peek_head_len(struct vhost_net *net, struct sock *sk)
+{
+	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
+	struct vhost_virtqueue *vq = &nvq->vq;
+	unsigned long uninitialized_var(endtime);
+
+	if (vq->busyloop_timeout) {
+		mutex_lock(&vq->mutex);
+		vhost_disable_notify(&net->dev, vq);
+
+		preempt_disable();
+		endtime = busy_clock() + vq->busyloop_timeout;
+
+		while (vhost_can_busy_poll(&net->dev, endtime) &&
+		       skb_queue_empty(&sk->sk_receive_queue) &&
+		       !vhost_vq_more_avail(&net->dev, vq))
+			cpu_relax();
+
+		preempt_enable();
+
+		if (vhost_enable_notify(&net->dev, vq))
+			vhost_poll_queue(&vq->poll);
+		mutex_unlock(&vq->mutex);
+	}
+
+	return peek_head_len(sk);
+}
+
 /* This is a multi-buffer version of vhost_get_desc, that works if
  *	vq has read descriptors only.
  * @vq		- the relevant virtqueue
@@ -553,7 +615,7 @@ static void handle_rx(struct vhost_net *net)
 		vq->log : NULL;
 	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);
 
-	while ((sock_len = peek_head_len(sock->sk))) {
+	while ((sock_len = vhost_net_peek_head_len(net, sock->sk))) {
 		sock_len += sock_hlen;
 		vhost_len = sock_len + vhost_hlen;
 		headcount = get_rx_bufs(vq, vq->heads, vhost_len,
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b86c5aa..857af6c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -285,6 +285,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->memory = NULL;
 	vq->is_le = virtio_legacy_is_little_endian();
 	vhost_vq_reset_user_be(vq);
+	vq->busyloop_timeout = 0;
 }
 
 static int vhost_worker(void *data)
@@ -747,6 +748,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 	struct vhost_vring_state s;
 	struct vhost_vring_file f;
 	struct vhost_vring_addr a;
+	struct vhost_vring_busyloop_timeout t;
 	u32 idx;
 	long r;
 
@@ -919,6 +921,19 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 	case VHOST_GET_VRING_ENDIAN:
 		r = vhost_get_vring_endian(vq, idx, argp);
 		break;
+	case VHOST_SET_VRING_BUSYLOOP_TIMEOUT:
+		if (copy_from_user(&t, argp, sizeof(t))) {
+			r = -EFAULT;
+			break;
+		}
+		vq->busyloop_timeout = t.timeout;
+		break;
+	case VHOST_GET_VRING_BUSYLOOP_TIMEOUT:
+		t.index = idx;
+		t.timeout = vq->busyloop_timeout;
+		if (copy_to_user(argp, &t, sizeof(t)))
+			r = -EFAULT;
+		break;
 	default:
 		r = -ENOIOCTLCMD;
 	}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 2f3c57c..4b7d4fa 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -115,6 +115,7 @@ struct vhost_virtqueue {
 	/* Ring endianness requested by userspace for cross-endian support. */
 	bool user_be;
 #endif
+	u32 busyloop_timeout;
 };
 
 struct vhost_dev {
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index ab373191..eaf6c33 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -27,6 +27,11 @@ struct vhost_vring_file {
 
 };
 
+struct vhost_vring_busyloop_timeout {
+	unsigned int index;
+	unsigned int timeout;
+};
+
 struct vhost_vring_addr {
 	unsigned int index;
 	/* Option flags. */
@@ -126,6 +131,12 @@ struct vhost_memory {
 #define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vring_file)
 /* Set eventfd to signal an error */
 #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
+/* Set busy loop timeout */
+#define VHOST_SET_VRING_BUSYLOOP_TIMEOUT _IOW(VHOST_VIRTIO, 0x23,	\
+					 struct vhost_vring_busyloop_timeout)
+/* Get busy loop timeout */
+#define VHOST_GET_VRING_BUSYLOOP_TIMEOUT _IOW(VHOST_VIRTIO, 0x24,	\
+					 struct vhost_vring_busyloop_timeout)
 
 /* VHOST_NET specific defines */
 
-- 
2.5.0

^ permalink raw reply related

* [PATCH net-next 2/3] vhost: introduce vhost_vq_more_avail()
From: Jason Wang @ 2015-11-25  7:11 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1448435489-5949-1-git-send-email-jasowang@redhat.com>

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/vhost.c | 26 +++++++++++++++++---------
 drivers/vhost/vhost.h |  1 +
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 163b365..b86c5aa 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1633,10 +1633,25 @@ void vhost_add_used_and_signal_n(struct vhost_dev *dev,
 }
 EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
 
+bool vhost_vq_more_avail(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{
+	__virtio16 avail_idx;
+	int r;
+
+	r = __get_user(avail_idx, &vq->avail->idx);
+	if (r) {
+		vq_err(vq, "Failed to check avail idx at %p: %d\n",
+		       &vq->avail->idx, r);
+		return false;
+	}
+
+	return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
+}
+EXPORT_SYMBOL_GPL(vhost_vq_more_avail);
+
 /* OK, now we need to know about added descriptors. */
 bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 {
-	__virtio16 avail_idx;
 	int r;
 
 	if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
@@ -1660,14 +1675,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 	/* They could have slipped one in as we were doing that: make
 	 * sure it's written, then check again. */
 	smp_mb();
-	r = __get_user(avail_idx, &vq->avail->idx);
-	if (r) {
-		vq_err(vq, "Failed to check avail idx at %p: %d\n",
-		       &vq->avail->idx, r);
-		return false;
-	}
-
-	return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
+	return vhost_vq_more_avail(dev, vq);
 }
 EXPORT_SYMBOL_GPL(vhost_enable_notify);
 
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 43284ad..2f3c57c 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -159,6 +159,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *, struct vhost_virtqueue *,
 			       struct vring_used_elem *heads, unsigned count);
 void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
 void vhost_disable_notify(struct vhost_dev *, struct vhost_virtqueue *);
+bool vhost_vq_more_avail(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-- 
2.5.0

^ permalink raw reply related

* [PATCH net-next 1/3] vhost: introduce vhost_has_work()
From: Jason Wang @ 2015-11-25  7:11 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1448435489-5949-1-git-send-email-jasowang@redhat.com>

This path introduces a helper which can give a hint for whether or not
there's a work queued in the work list.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/vhost.c | 7 +++++++
 drivers/vhost/vhost.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index eec2f11..163b365 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -245,6 +245,13 @@ void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
 }
 EXPORT_SYMBOL_GPL(vhost_work_queue);
 
+/* A lockless hint for busy polling code to exit the loop */
+bool vhost_has_work(struct vhost_dev *dev)
+{
+	return !list_empty(&dev->work_list);
+}
+EXPORT_SYMBOL_GPL(vhost_has_work);
+
 void vhost_poll_queue(struct vhost_poll *poll)
 {
 	vhost_work_queue(poll->dev, &poll->work);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d3f7674..43284ad 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -37,6 +37,7 @@ struct vhost_poll {
 
 void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
 void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work);
+bool vhost_has_work(struct vhost_dev *dev);
 
 void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
 		     unsigned long mask, struct vhost_dev *dev);
-- 
2.5.0

^ permalink raw reply related

* [PATCH net-next 0/3] basic busy polling support for vhost_net
From: Jason Wang @ 2015-11-25  7:11 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel

Hi all:

This series tries to add basic busy polling for vhost net. The idea is
simple: at the end of tx/rx processing, busy polling for new tx added
descriptor and rx receive socket for a while. The maximum number of
time (in us) could be spent on busy polling was specified ioctl.

Test A were done through:

- 50 us as busy loop timeout
- Netperf 2.6
- Two machines with back to back connected ixgbe
- Guest with 1 vcpu and 1 queue

Results:
- For stream workload, ioexits were reduced dramatically in medium
  size (1024-2048) of tx (at most -43%) and almost all rx (at most
  -84%) as a result of polling. This compensate for the possible
  wasted cpu cycles more or less. That porbably why we can still see
  some increasing in the normalized throughput in some cases.
- Throughput of tx were increased (at most 50%) expect for the huge
  write (16384). And we can send more packets in the case (+tpkts were
  increased).
- Very minor rx regression in some cases.
- Improvemnt on TCP_RR (at most 17%).

Guest TX:
size/session/+thu%/+normalize%/+tpkts%/+rpkts%/+ioexits%/
   64/     1/  +18%/  -10%/   +7%/  +11%/    0%
   64/     2/  +14%/  -13%/   +7%/  +10%/    0%
   64/     4/   +8%/  -17%/   +7%/   +9%/    0%
   64/     8/  +11%/  -15%/   +7%/  +10%/    0%
  256/     1/  +35%/   +9%/  +21%/  +12%/  -11%
  256/     2/  +26%/   +2%/  +20%/   +9%/  -10%
  256/     4/  +23%/    0%/  +21%/  +10%/   -9%
  256/     8/  +23%/    0%/  +21%/   +9%/   -9%
  512/     1/  +31%/   +9%/  +23%/  +18%/  -12%
  512/     2/  +30%/   +8%/  +24%/  +15%/  -10%
  512/     4/  +26%/   +5%/  +24%/  +14%/  -11%
  512/     8/  +32%/   +9%/  +23%/  +15%/  -11%
 1024/     1/  +39%/  +16%/  +29%/  +22%/  -26%
 1024/     2/  +35%/  +14%/  +30%/  +21%/  -22%
 1024/     4/  +34%/  +13%/  +32%/  +21%/  -25%
 1024/     8/  +36%/  +14%/  +32%/  +19%/  -26%
 2048/     1/  +50%/  +27%/  +34%/  +26%/  -42%
 2048/     2/  +43%/  +21%/  +36%/  +25%/  -43%
 2048/     4/  +41%/  +20%/  +37%/  +27%/  -43%
 2048/     8/  +40%/  +18%/  +35%/  +25%/  -42%
16384/     1/    0%/  -12%/   -1%/   +8%/  +15%
16384/     2/    0%/  -10%/   +1%/   +4%/   +5%
16384/     4/    0%/  -11%/   -3%/    0%/   +3%
16384/     8/    0%/  -10%/   -4%/    0%/   +1%

Guest RX:
size/session/+thu%/+normalize%/+tpkts%/+rpkts%/+ioexits%/
   64/     1/   -2%/  -21%/   +1%/   +2%/  -75%
   64/     2/   +1%/   -9%/  +12%/    0%/  -55%
   64/     4/    0%/   -6%/   +5%/   -1%/  -44%
   64/     8/   -5%/   -5%/   +7%/  -23%/  -50%
  256/     1/   -8%/  -18%/  +16%/  +15%/  -63%
  256/     2/    0%/   -8%/   +9%/   -2%/  -26%
  256/     4/    0%/   -7%/   -8%/  +20%/  -41%
  256/     8/   -8%/  -11%/   -9%/  -24%/  -78%
  512/     1/   -6%/  -19%/  +20%/  +18%/  -29%
  512/     2/    0%/  -10%/  -14%/   -8%/  -31%
  512/     4/   -1%/   -5%/  -11%/   -9%/  -38%
  512/     8/   -7%/   -9%/  -17%/  -22%/  -81%
 1024/     1/    0%/  -16%/  +12%/   +9%/  -11%
 1024/     2/    0%/  -11%/    0%/   +3%/  -30%
 1024/     4/    0%/   -4%/   +2%/   +6%/  -15%
 1024/     8/   -3%/   -4%/   -8%/   -8%/  -70%
 2048/     1/   -8%/  -23%/  +36%/  +22%/  -11%
 2048/     2/    0%/  -12%/   +1%/   +3%/  -29%
 2048/     4/    0%/   -3%/  -17%/  -15%/  -84%
 2048/     8/    0%/   -3%/   +1%/   -3%/  +10%
16384/     1/    0%/  -11%/   +4%/   +7%/  -22%
16384/     2/    0%/   -7%/   +4%/   +4%/  -33%
16384/     4/    0%/   -2%/   -2%/   -4%/  -23%
16384/     8/   -1%/   -2%/   +1%/  -22%/  -40%

TCP_RR:
size/session/+thu%/+normalize%/+tpkts%/+rpkts%/+ioexits%/
    1/     1/  +11%/  -26%/  +11%/  +11%/  +10%
    1/    25/  +11%/  -15%/  +11%/  +11%/    0%
    1/    50/   +9%/  -16%/  +10%/  +10%/    0%
    1/   100/   +9%/  -15%/   +9%/   +9%/    0%
   64/     1/  +11%/  -31%/  +11%/  +11%/  +11%
   64/    25/  +12%/  -14%/  +12%/  +12%/    0%
   64/    50/  +11%/  -14%/  +12%/  +12%/    0%
   64/   100/  +11%/  -15%/  +11%/  +11%/    0%
  256/     1/  +11%/  -27%/  +11%/  +11%/  +10%
  256/    25/  +17%/  -11%/  +16%/  +16%/   -1%
  256/    50/  +16%/  -11%/  +17%/  +17%/   +1%
  256/   100/  +17%/  -11%/  +18%/  +18%/   +1%

Test B were done through:

- 50us as busy loop timeout
- Netperf 2.6
- Two machines with back to back connected ixgbe
- Two guests each wich 1 vcpu and 1 queue
- pin two vhost threads to the same cpu on host to simulate the cpu
  contending

Results:
- In this radical case, we can still get at most 14% improvement on
  TCP_RR.
- For guest tx stream, minor improvemnt with at most 5% regression in
  one byte case. For guest rx stream, at most 5% regression were seen.

Guest TX:
size /-+%   /
1    /-5.55%/
64   /+1.11%/
256  /+2.33%/
512  /-0.03%/
1024 /+1.14%/
4096 /+0.00%/
16384/+0.00%/

Guest RX:
size /-+%   /
1    /-5.11%/
64   /-0.55%/
256  /-2.35%/
512  /-3.39%/
1024 /+6.8% /
4096 /-0.01%/
16384/+0.00%/

TCP_RR:
size /-+%    /
1    /+9.79% /
64   /+4.51% /
256  /+6.47% /
512  /-3.37% /
1024 /+6.15% /
4096 /+14.88%/
16384/-2.23% /

Changes from RFC V3:
- small tweak on the code to avoid multiple duplicate conditions in
  critical path when busy loop is not enabled.
- Add the test result of multiple VMs

Changes from RFC V2:
- poll also at the end of rx handling
- factor out the polling logic and optimize the code a little bit
- add two ioctls to get and set the busy poll timeout
- test on ixgbe (which can give more stable and reproducable numbers)
  instead of mlx4.

Changes from RFC V1:
- Add a comment for vhost_has_work() to explain why it could be
  lockless
- Add param description for busyloop_timeout
- Split out the busy polling logic into a new helper
- Check and exit the loop when there's a pending signal
- Disable preemption during busy looping to make sure lock_clock() was
  correctly used.

Jason Wang (3):
  vhost: introduce vhost_has_work()
  vhost: introduce vhost_vq_more_avail()
  vhost_net: basic polling support

 drivers/vhost/net.c        | 72 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/vhost/vhost.c      | 48 +++++++++++++++++++++++++------
 drivers/vhost/vhost.h      |  3 ++
 include/uapi/linux/vhost.h | 11 +++++++
 4 files changed, 120 insertions(+), 14 deletions(-)

-- 
2.5.0

^ permalink raw reply

* Re: [PATCH] paravirt: remove paravirt ops pmd_update[_defer] and pte_update_defer
From: Rusty Russell @ 2015-11-25  3:34 UTC (permalink / raw)
  To: Juergen Gross, linux-kernel, x86, hpa, tglx, mingo, jeremy,
	chrisw, akataria, virtualization, xen-devel, konrad.wilk,
	david.vrabel, boris.ostrovsky
In-Reply-To: <56545147.1000100@suse.com>

Juergen Gross <jgross@suse.com> writes:
> Ping?

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Cheers,
Rusty.

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Ming Lin @ 2015-11-24 19:25 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <56544122.3010300@redhat.com>

On Tue, 2015-11-24 at 11:51 +0100, Paolo Bonzini wrote:
> 
> On 24/11/2015 08:27, Ming Lin wrote:
> > handle_notify (qemu/hw/block/dataplane/virtio-blk.c:126)
> > aio_dispatch (qemu/aio-posix.c:329)
> > aio_poll (qemu/aio-posix.c:474)
> > iothread_run (qemu/iothread.c:45)
> > start_thread (pthread_create.c:312)
> > /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)
> > 
> > I think I'll have a "nvme_dev_notify" similar as "handle_notify"
> > 
> > static void nvme_dev_notify(EventNotifier *e)
> > {
> >     ....
> > }
> > 
> > But then how can I know this notify is for cq or sq?
> 
> virtio-blk has a single queue, so it has a single EventNotifier.  Your
> code using multiple EventNotifiers is fine, you can use
> aio_set_event_notifier multiple times on the same iothread.

I feel below patch is close to right.
But I got "Co-routine re-entered recursively".

Program received signal SIGABRT, Aborted.
0x00007ffff4a45cc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56	../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007ffff4a45cc9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff4a490d8 in __GI_abort () at abort.c:89
#2  0x0000555555b910d1 in qemu_coroutine_enter (co=0x5555577a9ce0, opaque=0x0) at /home/mlin/qemu/util/qemu-coroutine.c:111
#3  0x0000555555b282e0 in bdrv_co_io_em_complete (opaque=0x7fffd94e4a30, ret=0) at /home/mlin/qemu/block/io.c:2282
#4  0x0000555555ab8ba4 in thread_pool_completion_bh (opaque=0x555557d6d440) at /home/mlin/qemu/thread-pool.c:187
#5  0x0000555555ab7ab2 in aio_bh_call (bh=0x555557648110) at /home/mlin/qemu/async.c:64
#6  0x0000555555ab7b88 in aio_bh_poll (ctx=0x5555565b28f0) at /home/mlin/qemu/async.c:92
#7  0x0000555555acb3b6 in aio_dispatch (ctx=0x5555565b28f0) at /home/mlin/qemu/aio-posix.c:305
#8  0x0000555555ab8013 in aio_ctx_dispatch (source=0x5555565b28f0, callback=0x0, user_data=0x0) at /home/mlin/qemu/async.c:231
#9  0x00007ffff575ee04 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#10 0x0000555555ac8b35 in glib_pollfds_poll () at /home/mlin/qemu/main-loop.c:211
#11 0x0000555555ac8c36 in os_host_main_loop_wait (timeout=0) at /home/mlin/qemu/main-loop.c:256
#12 0x0000555555ac8cff in main_loop_wait (nonblocking=0) at /home/mlin/qemu/main-loop.c:504
#13 0x00005555558a41d4 in main_loop () at /home/mlin/qemu/vl.c:1920
#14 0x00005555558ac28a in main (argc=21, argv=0x7fffffffe4a8, envp=0x7fffffffe558) at /home/mlin/qemu/vl.c:4681
(gdb)

Would you help to take a look?

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f27fd35..f542740 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -28,6 +28,8 @@
 #include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
+#include "qom/object_interfaces.h"
 
 #include "nvme.h"
 
@@ -549,7 +551,10 @@ static void nvme_cq_notifier(EventNotifier *e)
         container_of(e, NvmeCQueue, notifier);
 
     event_notifier_test_and_clear(&cq->notifier);
+
+    aio_context_acquire(cq->ctrl->ctx);
     nvme_post_cqes(cq);
+    aio_context_release(cq->ctrl->ctx);
 }
 
 static void nvme_init_cq_eventfd(NvmeCQueue *cq)
@@ -558,9 +563,12 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
     uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
 
     event_notifier_init(&cq->notifier, 0);
-    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
     memory_region_add_eventfd(&n->iomem,
         0x1000 + offset, 4, false, 0, &cq->notifier);
+    aio_context_acquire(n->ctx);
+    aio_set_event_notifier(n->ctx, &cq->notifier, false,
+                           nvme_cq_notifier);
+    aio_context_release(n->ctx);
 }
 
 static void nvme_sq_notifier(EventNotifier *e)
@@ -569,7 +577,9 @@ static void nvme_sq_notifier(EventNotifier *e)
         container_of(e, NvmeSQueue, notifier);
 
     event_notifier_test_and_clear(&sq->notifier);
+    aio_context_acquire(sq->ctrl->ctx);
     nvme_process_sq(sq);
+    aio_context_release(sq->ctrl->ctx);
 }
 
 static void nvme_init_sq_eventfd(NvmeSQueue *sq)
@@ -578,9 +588,22 @@ static void nvme_init_sq_eventfd(NvmeSQueue *sq)
     uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
 
     event_notifier_init(&sq->notifier, 0);
-    event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
     memory_region_add_eventfd(&n->iomem,
         0x1000 + offset, 4, false, 0, &sq->notifier);
+    aio_context_acquire(n->ctx);
+    aio_set_event_notifier(n->ctx, &sq->notifier, false,
+                           nvme_sq_notifier);
+    aio_context_release(n->ctx);
+}
+
+static void nvme_init_iothread(NvmeCtrl *n)
+{
+    object_initialize(&n->internal_iothread_obj,
+                      sizeof(n->internal_iothread_obj),
+                      TYPE_IOTHREAD);
+    user_creatable_complete(OBJECT(&n->internal_iothread_obj), &error_abort);
+    n->iothread = &n->internal_iothread_obj;
+    n->ctx = iothread_get_aio_context(n->iothread);
 }
 
 static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
@@ -595,6 +618,8 @@ static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
         return NVME_INVALID_MEMORY_ADDRESS | NVME_DNR;
     }
 
+    nvme_init_iothread(n);
+
     /* This assumes all I/O queues are created before this command is handled.
      * We skip the admin queues. */
     for (i = 1; i < n->num_queues; i++) {
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 608f202..b53e69d 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -727,6 +727,10 @@ typedef struct NvmeCtrl {
     NvmeSQueue      admin_sq;
     NvmeCQueue      admin_cq;
     NvmeIdCtrl      id_ctrl;
+
+    IOThread *iothread;
+    IOThread internal_iothread_obj;
+    AioContext *ctx;
 } NvmeCtrl;
 
 #endif /* HW_NVME_H */

^ permalink raw reply related

* Re: [PATCH] paravirt: remove paravirt ops pmd_update[_defer] and pte_update_defer
From: Juergen Gross @ 2015-11-24 12:00 UTC (permalink / raw)
  To: linux-kernel, x86, hpa, tglx, mingo, jeremy, chrisw, akataria,
	rusty, virtualization, xen-devel, konrad.wilk, david.vrabel,
	boris.ostrovsky
In-Reply-To: <1447771879-1806-1-git-send-email-jgross@suse.com>

Ping?

On 17/11/15 15:51, Juergen Gross wrote:
> pte_update_defer can be removed as it is always set to the same
> function as pte_update. So any usage of pte_update_defer() can be
> replaced by pte_update().
> 
> pmd_update and pmd_update_defer are always set to paravirt_nop, so they
> can just be nuked.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/include/asm/paravirt.h       | 17 -----------------
>  arch/x86/include/asm/paravirt_types.h |  6 ------
>  arch/x86/include/asm/pgtable.h        | 15 ++-------------
>  arch/x86/kernel/paravirt.c            |  3 ---
>  arch/x86/lguest/boot.c                |  1 -
>  arch/x86/mm/pgtable.c                 |  7 +------
>  arch/x86/xen/mmu.c                    |  1 -
>  7 files changed, 3 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> index 10d0596..398f068 100644
> --- a/arch/x86/include/asm/paravirt.h
> +++ b/arch/x86/include/asm/paravirt.h
> @@ -375,23 +375,6 @@ static inline void pte_update(struct mm_struct *mm, unsigned long addr,
>  {
>  	PVOP_VCALL3(pv_mmu_ops.pte_update, mm, addr, ptep);
>  }
> -static inline void pmd_update(struct mm_struct *mm, unsigned long addr,
> -			      pmd_t *pmdp)
> -{
> -	PVOP_VCALL3(pv_mmu_ops.pmd_update, mm, addr, pmdp);
> -}
> -
> -static inline void pte_update_defer(struct mm_struct *mm, unsigned long addr,
> -				    pte_t *ptep)
> -{
> -	PVOP_VCALL3(pv_mmu_ops.pte_update_defer, mm, addr, ptep);
> -}
> -
> -static inline void pmd_update_defer(struct mm_struct *mm, unsigned long addr,
> -				    pmd_t *pmdp)
> -{
> -	PVOP_VCALL3(pv_mmu_ops.pmd_update_defer, mm, addr, pmdp);
> -}
>  
>  static inline pte_t __pte(pteval_t val)
>  {
> diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
> index 31247b5..6418541 100644
> --- a/arch/x86/include/asm/paravirt_types.h
> +++ b/arch/x86/include/asm/paravirt_types.h
> @@ -274,12 +274,6 @@ struct pv_mmu_ops {
>  			   pmd_t *pmdp, pmd_t pmdval);
>  	void (*pte_update)(struct mm_struct *mm, unsigned long addr,
>  			   pte_t *ptep);
> -	void (*pte_update_defer)(struct mm_struct *mm,
> -				 unsigned long addr, pte_t *ptep);
> -	void (*pmd_update)(struct mm_struct *mm, unsigned long addr,
> -			   pmd_t *pmdp);
> -	void (*pmd_update_defer)(struct mm_struct *mm,
> -				 unsigned long addr, pmd_t *pmdp);
>  
>  	pte_t (*ptep_modify_prot_start)(struct mm_struct *mm, unsigned long addr,
>  					pte_t *ptep);
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 6ec0c8b..d3eee66 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -69,9 +69,6 @@ extern struct mm_struct *pgd_page_get_mm(struct page *page);
>  #define pmd_clear(pmd)			native_pmd_clear(pmd)
>  
>  #define pte_update(mm, addr, ptep)              do { } while (0)
> -#define pte_update_defer(mm, addr, ptep)        do { } while (0)
> -#define pmd_update(mm, addr, ptep)              do { } while (0)
> -#define pmd_update_defer(mm, addr, ptep)        do { } while (0)
>  
>  #define pgd_val(x)	native_pgd_val(x)
>  #define __pgd(x)	native_make_pgd(x)
> @@ -731,14 +728,9 @@ static inline void native_set_pmd_at(struct mm_struct *mm, unsigned long addr,
>   * updates should either be sets, clears, or set_pte_atomic for P->P
>   * transitions, which means this hook should only be called for user PTEs.
>   * This hook implies a P->P protection or access change has taken place, which
> - * requires a subsequent TLB flush.  The notification can optionally be delayed
> - * until the TLB flush event by using the pte_update_defer form of the
> - * interface, but care must be taken to assure that the flush happens while
> - * still holding the same page table lock so that the shadow and primary pages
> - * do not become out of sync on SMP.
> + * requires a subsequent TLB flush.
>   */
>  #define pte_update(mm, addr, ptep)		do { } while (0)
> -#define pte_update_defer(mm, addr, ptep)	do { } while (0)
>  #endif
>  
>  /*
> @@ -830,9 +822,7 @@ static inline int pmd_write(pmd_t pmd)
>  static inline pmd_t pmdp_huge_get_and_clear(struct mm_struct *mm, unsigned long addr,
>  				       pmd_t *pmdp)
>  {
> -	pmd_t pmd = native_pmdp_get_and_clear(pmdp);
> -	pmd_update(mm, addr, pmdp);
> -	return pmd;
> +	return native_pmdp_get_and_clear(pmdp);
>  }
>  
>  #define __HAVE_ARCH_PMDP_SET_WRPROTECT
> @@ -840,7 +830,6 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
>  				      unsigned long addr, pmd_t *pmdp)
>  {
>  	clear_bit(_PAGE_BIT_RW, (unsigned long *)pmdp);
> -	pmd_update(mm, addr, pmdp);
>  }
>  
>  /*
> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> index c2130ae..f601250 100644
> --- a/arch/x86/kernel/paravirt.c
> +++ b/arch/x86/kernel/paravirt.c
> @@ -444,9 +444,6 @@ struct pv_mmu_ops pv_mmu_ops = {
>  	.set_pmd = native_set_pmd,
>  	.set_pmd_at = native_set_pmd_at,
>  	.pte_update = paravirt_nop,
> -	.pte_update_defer = paravirt_nop,
> -	.pmd_update = paravirt_nop,
> -	.pmd_update_defer = paravirt_nop,
>  
>  	.ptep_modify_prot_start = __ptep_modify_prot_start,
>  	.ptep_modify_prot_commit = __ptep_modify_prot_commit,
> diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
> index a0d09f6..a1900d4 100644
> --- a/arch/x86/lguest/boot.c
> +++ b/arch/x86/lguest/boot.c
> @@ -1472,7 +1472,6 @@ __init void lguest_init(void)
>  	pv_mmu_ops.lazy_mode.leave = lguest_leave_lazy_mmu_mode;
>  	pv_mmu_ops.lazy_mode.flush = paravirt_flush_lazy_mmu;
>  	pv_mmu_ops.pte_update = lguest_pte_update;
> -	pv_mmu_ops.pte_update_defer = lguest_pte_update;
>  
>  #ifdef CONFIG_X86_LOCAL_APIC
>  	/* APIC read/write intercepts */
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index fb0a9dd..ee9c2e3 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -414,7 +414,7 @@ int ptep_set_access_flags(struct vm_area_struct *vma,
>  
>  	if (changed && dirty) {
>  		*ptep = entry;
> -		pte_update_defer(vma->vm_mm, address, ptep);
> +		pte_update(vma->vm_mm, address, ptep);
>  	}
>  
>  	return changed;
> @@ -431,7 +431,6 @@ int pmdp_set_access_flags(struct vm_area_struct *vma,
>  
>  	if (changed && dirty) {
>  		*pmdp = entry;
> -		pmd_update_defer(vma->vm_mm, address, pmdp);
>  		/*
>  		 * We had a write-protection fault here and changed the pmd
>  		 * to to more permissive. No need to flush the TLB for that,
> @@ -469,9 +468,6 @@ int pmdp_test_and_clear_young(struct vm_area_struct *vma,
>  		ret = test_and_clear_bit(_PAGE_BIT_ACCESSED,
>  					 (unsigned long *)pmdp);
>  
> -	if (ret)
> -		pmd_update(vma->vm_mm, addr, pmdp);
> -
>  	return ret;
>  }
>  #endif
> @@ -518,7 +514,6 @@ void pmdp_splitting_flush(struct vm_area_struct *vma,
>  	set = !test_and_set_bit(_PAGE_BIT_SPLITTING,
>  				(unsigned long *)pmdp);
>  	if (set) {
> -		pmd_update(vma->vm_mm, address, pmdp);
>  		/* need tlb flush only to serialize against gup-fast */
>  		flush_tlb_range(vma, address, address + HPAGE_PMD_SIZE);
>  	}
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index ac161db..896dc14 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -2436,7 +2436,6 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
>  	.flush_tlb_others = xen_flush_tlb_others,
>  
>  	.pte_update = paravirt_nop,
> -	.pte_update_defer = paravirt_nop,
>  
>  	.pgd_alloc = xen_pgd_alloc,
>  	.pgd_free = xen_pgd_free,
> 

^ permalink raw reply

* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Paolo Bonzini @ 2015-11-24 11:01 UTC (permalink / raw)
  To: Ming Lin; +Cc: qemu block, qemu-devel, linux-nvme, virtualization
In-Reply-To: <1448346548.5392.4.camel@hasee>

On 24/11/2015 07:29, Ming Lin wrote:
>> Here is new performance number:
>>
>> qemu-nvme + google-ext + eventfd: 294MB/s
>> virtio-blk: 344MB/s
>> virtio-scsi: 296MB/s
>>
>> It's almost same as virtio-scsi. Nice.

Pretty good indeed.

> Looks like "regular MMIO" runs in vcpu thread, while "eventfd MMIO" runs
> in the main loop thread.
> 
> Could you help to explain why eventfd MMIO gets better performance?

Because VCPU latency is really everything if the I/O is very fast _or_
if the queue depth is high; signaling an eventfd is cheap enough to give
a noticeable boost in VCPU latency. Waking up a sleeping process is a
bit expensive, but if you manage to keep the iothread close to 100% CPU,
the main loop thread's poll() is usually quite cheap too.

> call stack: regular MMIO
> ========================
> nvme_mmio_write (qemu/hw/block/nvme.c:921)
> memory_region_write_accessor (qemu/memory.c:451)
> access_with_adjusted_size (qemu/memory.c:506)
> memory_region_dispatch_write (qemu/memory.c:1158)
> address_space_rw (qemu/exec.c:2547)
> kvm_cpu_exec (qemu/kvm-all.c:1849)
> qemu_kvm_cpu_thread_fn (qemu/cpus.c:1050)
> start_thread (pthread_create.c:312)
> clone
> 
> call stack: eventfd MMIO
> =========================
> nvme_sq_notifier (qemu/hw/block/nvme.c:598)
> aio_dispatch (qemu/aio-posix.c:329)
> aio_ctx_dispatch (qemu/async.c:232)
> g_main_context_dispatch
> glib_pollfds_poll (qemu/main-loop.c:213)
> os_host_main_loop_wait (qemu/main-loop.c:257)
> main_loop_wait (qemu/main-loop.c:504)
> main_loop (qemu/vl.c:1920)
> main (qemu/vl.c:4682)
> __libc_start_main

For comparison, here is the "iothread+eventfd MMIO" stack

nvme_sq_notifier (qemu/hw/block/nvme.c:598)
aio_dispatch (qemu/aio-posix.c:329)
aio_poll (qemu/aio-posix.c:474)
iothread_run (qemu/iothread.c:170)
__libc_start_main

aio_poll is much more specialized than the main thread (which uses glib
and thus wraps aio_poll with a GSource adapter), and can be faster too.
 (That said, things are still a bit in flux here.  2.6 will have pretty
heavy changes in this area, but the API will be the same).

Even more performance can be squeezed by adding a little bit of busy
waiting to aio_poll() before going to the blocking poll(). This avoids
very short idling and can improve things even more.

BTW, you may want to Cc qemu-block@nongnu.org in addition to
qemu-devel@nongnu.org.  Most people are on both lists, but some notice
things faster if you write to the lower-traffic qemu-block mailing list.

Paolo

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Paolo Bonzini @ 2015-11-24 10:51 UTC (permalink / raw)
  To: Ming Lin; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <1448350074.6696.3.camel@hasee>



On 24/11/2015 08:27, Ming Lin wrote:
> handle_notify (qemu/hw/block/dataplane/virtio-blk.c:126)
> aio_dispatch (qemu/aio-posix.c:329)
> aio_poll (qemu/aio-posix.c:474)
> iothread_run (qemu/iothread.c:45)
> start_thread (pthread_create.c:312)
> /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)
> 
> I think I'll have a "nvme_dev_notify" similar as "handle_notify"
> 
> static void nvme_dev_notify(EventNotifier *e)
> {
>     ....
> }
> 
> But then how can I know this notify is for cq or sq?

virtio-blk has a single queue, so it has a single EventNotifier.  Your
code using multiple EventNotifiers is fine, you can use
aio_set_event_notifier multiple times on the same iothread.

Paolo

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Ming Lin @ 2015-11-24  8:23 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <1448350074.6696.3.camel@hasee>

On Mon, 2015-11-23 at 23:27 -0800, Ming Lin wrote:
> On Mon, 2015-11-23 at 15:14 +0100, Paolo Bonzini wrote:
> > 
> > On 23/11/2015 09:17, Ming Lin wrote:
> > > On Sat, 2015-11-21 at 14:11 +0100, Paolo Bonzini wrote:
> > >>
> > >> On 20/11/2015 01:20, Ming Lin wrote:
> > >>> One improvment could be to use google's NVMe vendor extension that
> > >>> I send in another thread, aslo here:
> > >>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
> > >>>
> > >>> Qemu side:
> > >>> http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
> > >>> Kernel side also here:
> > >>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
> > >>
> > >> How much do you get with vhost-nvme plus vendor extension, compared to
> > >> 190 MB/s for QEMU?
> > > 
> > > There is still some bug. I'll update.
> > 
> > Sure.
> > 
> > >> Note that in all likelihood, QEMU can actually do better than 190 MB/s,
> > >> and gain more parallelism too, by moving the processing of the
> > >> ioeventfds to a separate thread.  This is similar to
> > >> hw/block/dataplane/virtio-blk.c.
> > >>
> > >> It's actually pretty easy to do.  Even though
> > >> hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
> > >> access in QEMU is now thread-safe.  I have pending patches for 2.6 that
> > >> cut that file down to a mere 200 lines of code, NVMe would probably be
> > >> about the same.
> > > 
> > > Is there a git tree for your patches?
> > 
> > No, not yet.  I'll post them today or tomorrow, will make sure to Cc you.
> > 
> > > Did you mean some pseduo code as below?
> > > 1. need a iothread for each cq/sq?
> > > 2. need a AioContext for each cq/sq?
> > > 
> > >  hw/block/nvme.c | 32 ++++++++++++++++++++++++++++++--
> > >  hw/block/nvme.h |  8 ++++++++
> > >  2 files changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index f27fd35..fed4827 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -28,6 +28,8 @@
> > >  #include "sysemu/sysemu.h"
> > >  #include "qapi/visitor.h"
> > >  #include "sysemu/block-backend.h"
> > > +#include "sysemu/iothread.h"
> > > +#include "qom/object_interfaces.h"
> > >  
> > >  #include "nvme.h"
> > >  
> > > @@ -558,9 +560,22 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
> > >      uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
> > >  
> > >      event_notifier_init(&cq->notifier, 0);
> > > -    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
> > >      memory_region_add_eventfd(&n->iomem,
> > >          0x1000 + offset, 4, false, 0, &cq->notifier);
> > > +
> > > +    object_initialize(&cq->internal_iothread_obj,
> > > +                      sizeof(cq->internal_iothread_obj),
> > > +                      TYPE_IOTHREAD);
> > > +    user_creatable_complete(OBJECT(&cq->internal_iothread_obj), &error_abort);
> > 
> > For now, you have to use one iothread for all cq/sq of a single NVMe
> > device; multiqueue block layer is planned for 2.7 or 2.8.  Otherwise
> > yes, it's very close to just these changes.
> 
> Here is the call stack of iothread for virtio-blk-dataplane.
> 
> handle_notify (qemu/hw/block/dataplane/virtio-blk.c:126)
> aio_dispatch (qemu/aio-posix.c:329)
> aio_poll (qemu/aio-posix.c:474)
> iothread_run (qemu/iothread.c:45)
> start_thread (pthread_create.c:312)
> /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)
> 
> I think I'll have a "nvme_dev_notify" similar as "handle_notify"
> 
> static void nvme_dev_notify(EventNotifier *e)
> {
>     ....
> }
> 
> But then how can I know this notify is for cq or sq?

Did you mean patch as below?
But it doesn't work yet.

 hw/block/nvme.c | 64 +++++++++++++++++++++++++++++++++++++++------------------
 hw/block/nvme.h |  5 +++++
 2 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f27fd35..a8c9914 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -28,6 +28,8 @@
 #include "sysemu/sysemu.h"
 #include "qapi/visitor.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/iothread.h"
+#include "qom/object_interfaces.h"
 
 #include "nvme.h"
 
@@ -543,42 +545,22 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
     return NVME_SUCCESS;
 }
 
-static void nvme_cq_notifier(EventNotifier *e)
-{
-    NvmeCQueue *cq =
-        container_of(e, NvmeCQueue, notifier);
-
-    event_notifier_test_and_clear(&cq->notifier);
-    nvme_post_cqes(cq);
-}
-
 static void nvme_init_cq_eventfd(NvmeCQueue *cq)
 {
     NvmeCtrl *n = cq->ctrl;
     uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
 
     event_notifier_init(&cq->notifier, 0);
-    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
     memory_region_add_eventfd(&n->iomem,
         0x1000 + offset, 4, false, 0, &cq->notifier);
 }
 
-static void nvme_sq_notifier(EventNotifier *e)
-{
-    NvmeSQueue *sq =
-        container_of(e, NvmeSQueue, notifier);
-
-    event_notifier_test_and_clear(&sq->notifier);
-    nvme_process_sq(sq);
-}
-
 static void nvme_init_sq_eventfd(NvmeSQueue *sq)
 {
     NvmeCtrl *n = sq->ctrl;
     uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
 
     event_notifier_init(&sq->notifier, 0);
-    event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
     memory_region_add_eventfd(&n->iomem,
         0x1000 + offset, 4, false, 0, &sq->notifier);
 }
@@ -900,6 +882,45 @@ static const MemoryRegionOps nvme_mmio_ops = {
     },
 };
 
+static void nvme_dev_notify(EventNotifier *e)
+{
+    NvmeCtrl *n =
+        container_of(e, NvmeCtrl, notifier);
+    int i;
+
+    event_notifier_test_and_clear(e);
+
+    for (i = 1; i < n->num_queues; i++) {
+        NvmeSQueue *sq = n->sq[i];
+        NvmeCQueue *cq = n->cq[i];
+
+        if (sq != NULL) {
+            event_notifier_test_and_clear(&sq->notifier);
+            nvme_process_sq(sq);
+        }
+
+        if (cq != NULL) {
+            event_notifier_test_and_clear(&cq->notifier);
+            nvme_post_cqes(cq);
+        }
+    }
+}
+
+static void nvme_init_iothread(NvmeCtrl *n)
+{
+    object_initialize(&n->internal_iothread_obj,
+                      sizeof(n->internal_iothread_obj),
+                      TYPE_IOTHREAD);
+    user_creatable_complete(OBJECT(&n->internal_iothread_obj), &error_abort);
+    n->iothread = &n->internal_iothread_obj;
+    n->ctx = iothread_get_aio_context(n->iothread);
+
+    aio_context_acquire(n->ctx);
+    aio_set_event_notifier(n->ctx, &n->notifier, true,
+                           nvme_dev_notify);
+    aio_context_release(n->ctx);
+}
+
 static int nvme_init(PCIDevice *pci_dev)
 {
     NvmeCtrl *n = NVME(pci_dev);
@@ -995,6 +1016,9 @@ static int nvme_init(PCIDevice *pci_dev)
             cpu_to_le64(n->ns_size >>
                 id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
     }
+
+    nvme_init_iothread(n);
+
     return 0;
 }
 
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 608f202..089af35 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -727,6 +727,11 @@ typedef struct NvmeCtrl {
     NvmeSQueue      admin_sq;
     NvmeCQueue      admin_cq;
     NvmeIdCtrl      id_ctrl;
+
+    IOThread *iothread;
+    IOThread internal_iothread_obj;
+    AioContext *ctx;
+    EventNotifier notifier;
 } NvmeCtrl;
 
 #endif /* HW_NVME_H */

^ permalink raw reply related

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Ming Lin @ 2015-11-24  7:27 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <56531F5F.3050709@redhat.com>

On Mon, 2015-11-23 at 15:14 +0100, Paolo Bonzini wrote:
> 
> On 23/11/2015 09:17, Ming Lin wrote:
> > On Sat, 2015-11-21 at 14:11 +0100, Paolo Bonzini wrote:
> >>
> >> On 20/11/2015 01:20, Ming Lin wrote:
> >>> One improvment could be to use google's NVMe vendor extension that
> >>> I send in another thread, aslo here:
> >>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
> >>>
> >>> Qemu side:
> >>> http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
> >>> Kernel side also here:
> >>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
> >>
> >> How much do you get with vhost-nvme plus vendor extension, compared to
> >> 190 MB/s for QEMU?
> > 
> > There is still some bug. I'll update.
> 
> Sure.
> 
> >> Note that in all likelihood, QEMU can actually do better than 190 MB/s,
> >> and gain more parallelism too, by moving the processing of the
> >> ioeventfds to a separate thread.  This is similar to
> >> hw/block/dataplane/virtio-blk.c.
> >>
> >> It's actually pretty easy to do.  Even though
> >> hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
> >> access in QEMU is now thread-safe.  I have pending patches for 2.6 that
> >> cut that file down to a mere 200 lines of code, NVMe would probably be
> >> about the same.
> > 
> > Is there a git tree for your patches?
> 
> No, not yet.  I'll post them today or tomorrow, will make sure to Cc you.
> 
> > Did you mean some pseduo code as below?
> > 1. need a iothread for each cq/sq?
> > 2. need a AioContext for each cq/sq?
> > 
> >  hw/block/nvme.c | 32 ++++++++++++++++++++++++++++++--
> >  hw/block/nvme.h |  8 ++++++++
> >  2 files changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index f27fd35..fed4827 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -28,6 +28,8 @@
> >  #include "sysemu/sysemu.h"
> >  #include "qapi/visitor.h"
> >  #include "sysemu/block-backend.h"
> > +#include "sysemu/iothread.h"
> > +#include "qom/object_interfaces.h"
> >  
> >  #include "nvme.h"
> >  
> > @@ -558,9 +560,22 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
> >      uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
> >  
> >      event_notifier_init(&cq->notifier, 0);
> > -    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
> >      memory_region_add_eventfd(&n->iomem,
> >          0x1000 + offset, 4, false, 0, &cq->notifier);
> > +
> > +    object_initialize(&cq->internal_iothread_obj,
> > +                      sizeof(cq->internal_iothread_obj),
> > +                      TYPE_IOTHREAD);
> > +    user_creatable_complete(OBJECT(&cq->internal_iothread_obj), &error_abort);
> 
> For now, you have to use one iothread for all cq/sq of a single NVMe
> device; multiqueue block layer is planned for 2.7 or 2.8.  Otherwise
> yes, it's very close to just these changes.

Here is the call stack of iothread for virtio-blk-dataplane.

handle_notify (qemu/hw/block/dataplane/virtio-blk.c:126)
aio_dispatch (qemu/aio-posix.c:329)
aio_poll (qemu/aio-posix.c:474)
iothread_run (qemu/iothread.c:45)
start_thread (pthread_create.c:312)
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)

I think I'll have a "nvme_dev_notify" similar as "handle_notify"

static void nvme_dev_notify(EventNotifier *e)
{
    ....
}

But then how can I know this notify is for cq or sq?

^ permalink raw reply

* Re: [PATCH -qemu] nvme: support Google vendor extension
From: Ming Lin @ 2015-11-24  6:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, linux-nvme, virtualization
In-Reply-To: <1448178345.7480.2.camel@hasee>

On Sat, 2015-11-21 at 23:45 -0800, Ming Lin wrote:
> On Sat, 2015-11-21 at 13:56 +0100, Paolo Bonzini wrote:
> > 
> > On 21/11/2015 00:05, Ming Lin wrote:
> > > [    1.752129] Freeing unused kernel memory: 420K (ffff880001b97000 - ffff880001c00000)
> > > [    1.986573] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x30e5c9bbf83, max_idle_ns: 440795378954 ns
> > > [    1.988187] clocksource: Switched to clocksource tsc
> > > [    3.235423] clocksource: timekeeping watchdog: Marking clocksource 'tsc' as unstable because the skew is too large:
> > > [    3.358713] clocksource:                       'refined-jiffies' wd_now: fffeddf3 wd_last: fffedd76 mask: ffffffff
> > > [    3.410013] clocksource:                       'tsc' cs_now: 3c121d4ec cs_last: 340888eb7 mask: ffffffffffffffff
> > > [    3.450026] clocksource: Switched to clocksource refined-jiffies
> > > [    7.696769] Adding 392188k swap on /dev/vda5.  Priority:-1 extents:1 across:392188k 
> > > [    7.902174] EXT4-fs (vda1): re-mounted. Opts: (null)
> > > [    8.734178] EXT4-fs (vda1): re-mounted. Opts: errors=remount-ro
> > > 
> > > Then it doesn't response input for almost 1 minute.
> > > Without this patch, kernel loads quickly.
> > 
> > Interesting.  I guess there's time to debug it, since QEMU 2.6 is still 
> > a few months away.  In the meanwhile we can apply your patch as is, 
> > apart from disabling the "if (new_head >= cq->size)" and the similar 
> > one for "if (new_ tail >= sq->size".
> > 
> > But, I have a possible culprit.  In your nvme_cq_notifier you are not doing the 
> > equivalent of:
> > 
> > 	start_sqs = nvme_cq_full(cq) ? 1 : 0;
> >         cq->head = new_head;
> >         if (start_sqs) {
> >             NvmeSQueue *sq;
> >             QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
> >                 timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> >             }
> >             timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> >         }
> > 
> > Instead, you are just calling nvme_post_cqes, which is the equivalent of
> > 
> > 	timer_mod(cq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> > 
> > Adding a loop to nvme_cq_notifier, and having it call nvme_process_sq, might
> > fix the weird 1-minute delay.
> 
> I found it.
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 31572f2..f27fd35 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -548,6 +548,7 @@ static void nvme_cq_notifier(EventNotifier *e)
>      NvmeCQueue *cq =
>          container_of(e, NvmeCQueue, notifier);
>  
> +    event_notifier_test_and_clear(&cq->notifier);
>      nvme_post_cqes(cq);
>  }
>  
> @@ -567,6 +568,7 @@ static void nvme_sq_notifier(EventNotifier *e)
>      NvmeSQueue *sq =
>          container_of(e, NvmeSQueue, notifier);
>  
> +    event_notifier_test_and_clear(&sq->notifier);
>      nvme_process_sq(sq);
>  }
>  
> Here is new performance number:
> 
> qemu-nvme + google-ext + eventfd: 294MB/s
> virtio-blk: 344MB/s
> virtio-scsi: 296MB/s
> 
> It's almost same as virtio-scsi. Nice.

(strip CC)

Looks like "regular MMIO" runs in vcpu thread, while "eventfd MMIO" runs
in the main loop thread.

Could you help to explain why eventfd MMIO gets better performance?

call stack: regular MMIO
========================
nvme_mmio_write (qemu/hw/block/nvme.c:921)
memory_region_write_accessor (qemu/memory.c:451)
access_with_adjusted_size (qemu/memory.c:506)
memory_region_dispatch_write (qemu/memory.c:1158)
address_space_rw (qemu/exec.c:2547)
kvm_cpu_exec (qemu/kvm-all.c:1849)
qemu_kvm_cpu_thread_fn (qemu/cpus.c:1050)
start_thread (pthread_create.c:312)
clone

call stack: eventfd MMIO
=========================
nvme_sq_notifier (qemu/hw/block/nvme.c:598)
aio_dispatch (qemu/aio-posix.c:329)
aio_ctx_dispatch (qemu/async.c:232)
g_main_context_dispatch
glib_pollfds_poll (qemu/main-loop.c:213)
os_host_main_loop_wait (qemu/main-loop.c:257)
main_loop_wait (qemu/main-loop.c:504)
main_loop (qemu/vl.c:1920)
main (qemu/vl.c:4682)
__libc_start_main

^ permalink raw reply

* Re: [PATCH] virtio_ring: Shadow available ring flags & index
From: Xie, Huawei @ 2015-11-23 16:46 UTC (permalink / raw)
  To: Venkatesh Srinivas
  Cc: Abhinkar, Bindiya S, KVM list, Michael S. Tsirkin,
	virtualization@lists.linux-foundation.org, Venkatesh Srinivas,
	Lu, Patrick, luto@kernel.org, Wang, Zhihong, David Matlack,
	Paolo Bonzini
In-Reply-To: <20151120183011.GA24228@google.com>

On 11/21/2015 2:30 AM, Venkatesh Srinivas wrote:
> On Thu, Nov 19, 2015 at 04:15:48PM +0000, Xie, Huawei wrote:
>> On 11/18/2015 12:28 PM, Venkatesh Srinivas wrote:
>>> On Tue, Nov 17, 2015 at 08:08:18PM -0800, Venkatesh Srinivas wrote:
>>>> On Mon, Nov 16, 2015 at 7:46 PM, Xie, Huawei <huawei.xie@intel.com> wrote:
>>>>
>>>>> On 11/14/2015 7:41 AM, Venkatesh Srinivas wrote:
>>>>>> On Wed, Nov 11, 2015 at 02:34:33PM +0200, Michael S. Tsirkin wrote:
>>>>>>> On Tue, Nov 10, 2015 at 04:21:07PM -0800, Venkatesh Srinivas wrote:
>>>>>>>> Improves cacheline transfer flow of available ring header.
>>>>>>>>
>>>>>>>> Virtqueues are implemented as a pair of rings, one producer->consumer
>>>>>>>> avail ring and one consumer->producer used ring; preceding the
>>>>>>>> avail ring in memory are two contiguous u16 fields -- avail->flags
>>>>>>>> and avail->idx. A producer posts work by writing to avail->idx and
>>>>>>>> a consumer reads avail->idx.
>>>>>>>>
>>>>>>>> The flags and idx fields only need to be written by a producer CPU
>>>>>>>> and only read by a consumer CPU; when the producer and consumer are
>>>>>>>> running on different CPUs and the virtio_ring code is structured to
>>>>>>>> only have source writes/sink reads, we can continuously transfer the
>>>>>>>> avail header cacheline between 'M' states between cores. This flow
>>>>>>>> optimizes core -> core bandwidth on certain CPUs.
>>>>>>>>
>>>>>>>> (see: "Software Optimization Guide for AMD Family 15h Processors",
>>>>>>>> Section 11.6; similar language appears in the 10h guide and should
>>>>>>>> apply to CPUs w/ exclusive caches, using LLC as a transfer cache)
>>>>>>>>
>>>>>>>> Unfortunately the existing virtio_ring code issued reads to the
>>>>>>>> avail->idx and read-modify-writes to avail->flags on the producer.
>>>>>>>>
>>>>>>>> This change shadows the flags and index fields in producer memory;
>>>>>>>> the vring code now reads from the shadows and only ever writes to
>>>>>>>> avail->flags and avail->idx, allowing the cacheline to transfer
>>>>>>>> core -> core optimally.
>>>>>>> Sounds logical, I'll apply this after a  bit of testing
>>>>>>> of my own, thanks!
>>>>>> Thanks!
>>>>> Venkatesh:
>>>>> Is it that your patch only applies to CPUs w/ exclusive caches?
>>>> No --- it applies when the inter-cache coherence flow is optimized by
>>>> 'M' -> 'M' transfers and when producer reads might interfere w/
>>>> consumer prefetchw/reads. The AMD Optimization guides have specific
>>>> language on this subject, but other platforms may benefit.
>>>> (see Intel #'s below)
>> For core2core case(not HT paire), after consumer reads that M cache line
>> for avail_idx, is that line still in the producer core's L1 data cache
>> with state changing from M->O state?
> Textbook MOESI would not allow that state combination -- when the consumer
> gets the line in 'M' state, the producer cannot hold it in 'O' state.
Hi Venkatesh:
On consumer core, you are using (prefetchw + load) to get the cache line
anyway, even it doesn't mean to write, right? That makes sense for your
cache line transfer.
If using load only, the cache line on producer core should be changed
from M -> O, meaning dirty sharing, and the consumer gets the line with
S state.

I might miss something important in your case. Could you give more
detailed description?
For non-shadow case,
1) Producer updates flags or idx, cache line is set to be M state.
2) When consumer reads the idx or flags, cache line is set to be S state
on consumer core, while the cache line on producer is set to be O state.
What is the problem reading avail idx/flag whose cache line is either M
or O state on producer core? What is the benefit with and without prefetchw?

>
> On the AMD Piledriver, per the Optimization guide, I use PREFETCHW/Load to
> get the line in 'M' state on the consumer (invalidating it in the Producer's
> cache):
>
> "* Use PREFETCHW on the consumer side, even if the consumer will not modify
>    the data"
>
> That, plus the "Optimizing Inter-Core Data Transfer" section imply that
> PREFETCHW + MOV will cause the consumer to load the line into 'M' state.
>
> PREFETCHW was not available on Intel CPUs pre-Broadwell; from the public
> documentation alone, I don't think we can tell what transition the producer's
> cacheline undergoes on these cores. For that matter, the latest documentation
> I can find (for Nehalem), indicated there was no 'O' state -- Nehalem
> implemented MESIF, not MOESI.
By O, i mean AMD MOESI, and i thought you were using only load to load
the cache line on the consumer core. If you are using prefetchw + load,
that makes sense for the state transfer.
>
> HTH,
> -- vs;
>

^ permalink raw reply

* Re: [RFC PATCH 0/9] vhost-nvme: new qemu nvme backend using nvme target
From: Paolo Bonzini @ 2015-11-23 14:14 UTC (permalink / raw)
  To: Ming Lin; +Cc: qemu-devel, Christoph Hellwig, linux-nvme, virtualization
In-Reply-To: <1448266667.18175.5.camel@hasee>



On 23/11/2015 09:17, Ming Lin wrote:
> On Sat, 2015-11-21 at 14:11 +0100, Paolo Bonzini wrote:
>>
>> On 20/11/2015 01:20, Ming Lin wrote:
>>> One improvment could be to use google's NVMe vendor extension that
>>> I send in another thread, aslo here:
>>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=nvme-google-ext
>>>
>>> Qemu side:
>>> http://www.minggr.net/cgit/cgit.cgi/qemu/log/?h=vhost-nvme.0
>>> Kernel side also here:
>>> https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=vhost-nvme.0
>>
>> How much do you get with vhost-nvme plus vendor extension, compared to
>> 190 MB/s for QEMU?
> 
> There is still some bug. I'll update.

Sure.

>> Note that in all likelihood, QEMU can actually do better than 190 MB/s,
>> and gain more parallelism too, by moving the processing of the
>> ioeventfds to a separate thread.  This is similar to
>> hw/block/dataplane/virtio-blk.c.
>>
>> It's actually pretty easy to do.  Even though
>> hw/block/dataplane/virtio-blk.c is still using some old APIs, all memory
>> access in QEMU is now thread-safe.  I have pending patches for 2.6 that
>> cut that file down to a mere 200 lines of code, NVMe would probably be
>> about the same.
> 
> Is there a git tree for your patches?

No, not yet.  I'll post them today or tomorrow, will make sure to Cc you.

> Did you mean some pseduo code as below?
> 1. need a iothread for each cq/sq?
> 2. need a AioContext for each cq/sq?
> 
>  hw/block/nvme.c | 32 ++++++++++++++++++++++++++++++--
>  hw/block/nvme.h |  8 ++++++++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f27fd35..fed4827 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -28,6 +28,8 @@
>  #include "sysemu/sysemu.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/iothread.h"
> +#include "qom/object_interfaces.h"
>  
>  #include "nvme.h"
>  
> @@ -558,9 +560,22 @@ static void nvme_init_cq_eventfd(NvmeCQueue *cq)
>      uint16_t offset = (cq->cqid*2+1) * (4 << NVME_CAP_DSTRD(n->bar.cap));
>  
>      event_notifier_init(&cq->notifier, 0);
> -    event_notifier_set_handler(&cq->notifier, nvme_cq_notifier);
>      memory_region_add_eventfd(&n->iomem,
>          0x1000 + offset, 4, false, 0, &cq->notifier);
> +
> +    object_initialize(&cq->internal_iothread_obj,
> +                      sizeof(cq->internal_iothread_obj),
> +                      TYPE_IOTHREAD);
> +    user_creatable_complete(OBJECT(&cq->internal_iothread_obj), &error_abort);

For now, you have to use one iothread for all cq/sq of a single NVMe
device; multiqueue block layer is planned for 2.7 or 2.8.  Otherwise
yes, it's very close to just these changes.

If you use "-object iothread,id=NN" and a iothread property, you can
also use an N:M model with multiple disks attached to the same iothread.
 Defining the iothread property is like

	object_property_add_link(obj, "iothread", TYPE_IOTHREAD,
				(Object **)&s->conf.iothread,
				qdev_prop_allow_set_link_before_realize,
				OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);

Thanks,

Paolo

> +    cq->iothread = &cq->internal_iothread_obj;
> +    cq->ctx = iothread_get_aio_context(cq->iothread);
> +    //Question: Need a conf.blk for each cq/sq???
> +    //blk_set_aio_context(cq->conf->conf.blk, cq->ctx);
> +    aio_context_acquire(cq->ctx);
> +    aio_set_event_notifier(cq->ctx, &cq->notifier, true,
> +                           nvme_cq_notifier);
> +    aio_context_release(cq->ctx);
>  }
>  
>  static void nvme_sq_notifier(EventNotifier *e)
> @@ -578,9 +593,22 @@ static void nvme_init_sq_eventfd(NvmeSQueue *sq)
>      uint16_t offset = sq->sqid * 2 * (4 << NVME_CAP_DSTRD(n->bar.cap));
>  
>      event_notifier_init(&sq->notifier, 0);
> -    event_notifier_set_handler(&sq->notifier, nvme_sq_notifier);
>      memory_region_add_eventfd(&n->iomem,
>          0x1000 + offset, 4, false, 0, &sq->notifier);
> +
> +    object_initialize(&sq->internal_iothread_obj,
> +                      sizeof(sq->internal_iothread_obj),
> +                      TYPE_IOTHREAD);
> +    user_creatable_complete(OBJECT(&sq->internal_iothread_obj), &error_abort);
> +    sq->iothread = &sq->internal_iothread_obj;
> +    sq->ctx = iothread_get_aio_context(sq->iothread);
> +    //Question: Need a conf.blk for each cq/sq???
> +    //blk_set_aio_context(sq->conf->conf.blk, sq->ctx);
> +
> +    aio_context_acquire(sq->ctx);
> +    aio_set_event_notifier(sq->ctx, &sq->notifier, true,
> +                           nvme_sq_notifier);
> +    aio_context_release(sq->ctx);
>  }
>  
>  static uint16_t nvme_set_db_memory(NvmeCtrl *n, const NvmeCmd *cmd)
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 608f202..171ee0b 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -667,6 +667,10 @@ typedef struct NvmeSQueue {
>       * do not go over this value will not result in MMIO writes (but will
>       * still write the tail pointer to the "db_addr" location above). */
>      uint64_t    eventidx_addr;
> +
> +    IOThread *iothread;
> +    IOThread internal_iothread_obj;
> +    AioContext *ctx;
>      EventNotifier notifier;
>  } NvmeSQueue;
>  
> @@ -690,6 +694,10 @@ typedef struct NvmeCQueue {
>       * do not go over this value will not result in MMIO writes (but will
>       * still write the head pointer to the "db_addr" location above). */
>      uint64_t    eventidx_addr;
> +
> +    IOThread *iothread;
> +    IOThread internal_iothread_obj;
> +    AioContext *ctx;
>      EventNotifier notifier;
>  } NvmeCQueue;
>  
> 
>>
>> Paolo

^ permalink raw reply

* Re: vhost-blk and qemu
From: Stefan Hajnoczi @ 2015-11-23  8:39 UTC (permalink / raw)
  To: Mohan G; +Cc: virtualization@lists.linux-foundation.org
In-Reply-To: <1872664481.6220477.1447766581884.JavaMail.yahoo@mail.yahoo.com>

On Tue, Nov 17, 2015 at 9:23 PM, Mohan G via Virtualization
<virtualization@lists.linux-foundation.org> wrote:
> I am looking to experiment the vhost-blk stack. Can some one point me to the
> latest code version and the corresponding qemu version location.
> I am on centos 7 (3.10 ) kernel. I am hoping using the vhost-blk.ko and
> corresponding qemu version will get me started to measure some numbers.

vhost-blk is not under active development.  New revisions of the
patches have not been posted for some time.

You may find the recent vhost-nvme patches from Ming Lin interesting instead:
http://permalink.gmane.org/gmane.linux.kernel.virtualization/26254

Stefan

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox