* Re: Re: [RFC PATCH 0/6] virtio-trace: Support virtio-trace
From: Amit Shah @ 2012-07-27 9:43 UTC (permalink / raw)
To: Yoshihiro YUNOMAE
Cc: Arnd Bergmann, Greg Kroah-Hartman, Frederic Weisbecker,
qemu-devel, Borislav Petkov, linux-kernel, Herbert Xu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, yrl.pp-manager.tt, Masami Hiramatsu,
virtualization
In-Reply-To: <5012576F.7040500@hitachi.com>
On (Fri) 27 Jul 2012 [17:55:11], Yoshihiro YUNOMAE wrote:
> Hi Amit,
>
> Thank you for commenting on our work.
>
> (2012/07/26 20:35), Amit Shah wrote:
> >On (Tue) 24 Jul 2012 [11:36:57], Yoshihiro YUNOMAE wrote:
>
> [...]
>
> >>
> >>Therefore, we propose a new system "virtio-trace", which uses enhanced
> >>virtio-serial and existing ring-buffer of ftrace, for collecting guest kernel
> >>tracing data. In this system, there are 5 main components:
> >> (1) Ring-buffer of ftrace in a guest
> >> - When trace agent reads ring-buffer, a page is removed from ring-buffer.
> >> (2) Trace agent in the guest
> >> - Splice the page of ring-buffer to read_pipe using splice() without
> >> memory copying. Then, the page is spliced from write_pipe to virtio
> >> without memory copying.
> >
> >I really like the splicing idea.
>
> Thanks. We will improve this patch set.
>
> >> (3) Virtio-console driver in the guest
> >> - Pass the page to virtio-ring
> >> (4) Virtio-serial bus in QEMU
> >> - Copy the page to kernel pipe
> >> (5) Reader in the host
> >> - Read guest tracing data via FIFO(named pipe)
> >
> >So will this be useful only if guest and host run the same kernel?
> >
> >I'd like to see the host kernel not being used at all -- collect all
> >relevant info from the guest and send it out to qemu, where it can be
> >consumed directly by apps driving the tracing.
>
> No, this patch set is used only for guest kernels, so guest and host
> don't need to run the same kernel.
OK - that's good to know.
> >>***Evaluation***
> >>When a host collects tracing data of a guest, the performance of using
> >>virtio-trace is compared with that of using native(just running ftrace),
> >>IVRing, and virtio-serial(normal method of read/write).
> >
> >Why is tracing performance-sensitive? i.e. why try to optimise this
> >at all?
>
> To minimize effects for applications on guests when a host collects
> tracing data of guests.
> For example, we assume the situation where guests A and B are running
> on a host sharing I/O device. An I/O delay problem occur in guest A,
> but it doesn't for the requirement in guest B. In this case, we need to
> collect tracing data of guests A and B, but a usual method using
> network takes high load for applications of guest B even if guest B is
> normally running. Therefore, we try to decrease the load on guests.
> We also use this feature for performance analysis on production
> virtualization systems.
OK, got it.
>
> [...]
>
> >>
> >>***Just enhancement ideas***
> >> - Support for trace-cmd
> >> - Support for 9pfs protocol
> >> - Support for non-blocking mode in QEMU
> >
> >There were patches long back (by me) to make chardevs non-blocking but
> >they didn't make it upstream. Fedora carries them, if you want to try
> >out. Though we want to converge on a reasonable solution that's
> >acceptable upstream as well. Just that no one's working on it
> >currently. Any help here will be appreciated.
>
> Thanks! In this case, since a guest will stop to run when host reads
> trace data of the guest, char device is needed to add a non-blocking
> mode. I'll read your patch series. Is the latest version 8?
> http://lists.gnu.org/archive/html/qemu-devel/2010-12/msg00035.html
I suppose the latest version on-list is what you quote above. The
objections to the patch series are mentioned in Anthony's mails.
Hans maintains a rebased version of the patches in his tree at
http://cgit.freedesktop.org/~jwrdegoede/qemu/
those patches are included in Fedora's qemu-kvm, so you can try that
out if it improves performance for you.
> >> - Make "vhost-serial"
> >
> >I need to understand a) why it's perf-critical, and b) why should the
> >host be involved at all, to comment on these.
>
> a) To make collecting overhead decrease for application on a guest.
> (see above)
> b) Trace data of host kernel is not involved even if we introduce this
> patch set.
I see, so you suggested vhost-serial only because you saw the guest
stopping problem due to the absence of non-blocking code? If so, it
now makes sense. I don't think we need vhost-serial in any way yet.
BTW where do you parse the trace data obtained from guests? On a
remote host?
Thanks,
Amit
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Alan Cox @ 2012-07-27 9:53 UTC (permalink / raw)
To: Andrew Stiegmann (stieg)
Cc: pv-drivers, gregkh, linux-kernel, virtualization, vm-crosstalk,
cschamp
In-Reply-To: <1343345980-32397-12-git-send-email-astiegmann@vmware.com>
> +enum {
> + VMCI_SUCCESS_QUEUEPAIR_ATTACH = 5,
> + VMCI_SUCCESS_QUEUEPAIR_CREATE = 4,
> + VMCI_SUCCESS_LAST_DETACH = 3,
> + VMCI_SUCCESS_ACCESS_GRANTED = 2,
> + VMCI_SUCCESS_ENTRY_DEAD = 1,
We've got a nice collection of Linux error codes than you, and it would
make the driver enormously more readable on the Linux side if as low
level as possible it started using Linux error codes.
> + VMCI_SUCCESS = 0,
> + VMCI_ERROR_INVALID_RESOURCE = (-1),
> + VMCI_ERROR_INVALID_ARGS = (-2),
> + VMCI_ERROR_NO_MEM = (-3),
> + VMCI_ERROR_DATAGRAM_FAILED = (-4),
> + VMCI_ERROR_MORE_DATA = (-5),
> + VMCI_ERROR_NO_MORE_DATAGRAMS = (-6),
> + VMCI_ERROR_NO_ACCESS = (-7),
> + VMCI_ERROR_NO_HANDLE = (-8),
> + VMCI_ERROR_DUPLICATE_ENTRY = (-9),
> + VMCI_ERROR_DST_UNREACHABLE = (-10),
> + VMCI_ERROR_PAYLOAD_TOO_LARGE = (-11),
> + VMCI_ERROR_INVALID_PRIV = (-12),
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Sam Ravnborg @ 2012-07-27 10:34 UTC (permalink / raw)
To: Andrew Stiegmann (stieg)
Cc: pv-drivers, gregkh, linux-kernel, virtualization, vm-crosstalk,
cschamp
In-Reply-To: <1343345980-32397-12-git-send-email-astiegmann@vmware.com>
Hi Andrew.
A few things noted in the following..
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 2661f6e..fe38c7a 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -517,4 +517,5 @@ source "drivers/misc/lis3lv02d/Kconfig"
> source "drivers/misc/carma/Kconfig"
> source "drivers/misc/altera-stapl/Kconfig"
> source "drivers/misc/mei/Kconfig"
> +source "drivers/misc/vmw_vmci/Kconfig"
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 456972f..af9e413 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -51,3 +51,4 @@ obj-y += carma/
> obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
> obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/
> obj-$(CONFIG_INTEL_MEI) += mei/
> +obj-y += vmw_vmci/
Please use obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
like we do in the other cases. This prevents us from visiting the directory
when this feature is not enabled.
> +++ b/drivers/misc/vmw_vmci/Makefile
> @@ -0,0 +1,43 @@
> +################################################################################
> +#
> +# Linux driver for VMware's VMCI device.
> +#
> +# Copyright (C) 2007-2012, VMware, Inc. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License as published by the
> +# Free Software Foundation; version 2 of the License and no later version.
> +#
> +# This program is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> +# NON INFRINGEMENT. See the GNU General Public License for more
> +# details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".
> +#
> +# Maintained by: Andrew Stiegmann <pv-drivers@vmware.com>
> +#
> +################################################################################
Lot's of boilerplate noise for such a simple file...
> +
> +#
> +# Makefile for the VMware VMCI
> +#
> +
> +obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci.o
> +
> +vmw_vmci-objs += vmci_context.o
> +vmw_vmci-objs += vmci_datagram.o
> +vmw_vmci-objs += vmci_doorbell.o
> +vmw_vmci-objs += vmci_driver.o
> +vmw_vmci-objs += vmci_event.o
> +vmw_vmci-objs += vmci_handle_array.o
> +vmw_vmci-objs += vmci_hash_table.o
> +vmw_vmci-objs += vmci_queue_pair.o
> +vmw_vmci-objs += vmci_resource.o
> +vmw_vmci-objs += vmci_route.o
please use:
vmw_vmci-y += vmci_context.o
vmw_vmci-y += vmci_datagram.o
vmw_vmci-y += vmci_doorbell.o
This is recommended these days and allows you to enable/disable
single files later using a config option.
> diff --git a/drivers/misc/vmw_vmci/vmci_common_int.h b/drivers/misc/vmw_vmci/vmci_common_int.h
> +
> +#ifndef _VMCI_COMMONINT_H_
> +#define _VMCI_COMMONINT_H_
> +
> +#include <linux/printk.h>
> +#include <linux/vmw_vmci_defs.h>
Use inverse chrismas tree here.
Longer include lines first, and soret alphabetically when
lines are of the same length.
This applies likely in many cases.
> +#include "vmci_handle_array.h"
> +
> +#define ASSERT(cond) BUG_ON(!(cond))
> +
> +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK))
> +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED)
Looks like poor obscufation.
Use a statis inline function if you need a helper for this.
> +
> +/*
> + * Utilility function that checks whether two entities are allowed
> + * to interact. If one of them is restricted, the other one must
> + * be trusted.
> + */
> +static inline bool vmci_deny_interaction(uint32_t partOne,
> + uint32_t partTwo)
The kernel types are u32 not uint32_t - these types belongs in user-space.
> +++ b/include/linux/vmw_vmci_api.h
> +
> +#ifndef __VMW_VMCI_API_H__
> +#define __VMW_VMCI_API_H__
> +
> +#include <linux/vmw_vmci_defs.h>
> +
> +#undef VMCI_KERNEL_API_VERSION
> +#define VMCI_KERNEL_API_VERSION_2 2
> +#define VMCI_KERNEL_API_VERSION VMCI_KERNEL_API_VERSION_2
> +
> +typedef void (VMCI_DeviceShutdownFn) (void *deviceRegistration, void *userData);
> +
> +bool VMCI_DeviceGet(uint32_t *apiVersion,
> + VMCI_DeviceShutdownFn *deviceShutdownCB,
> + void *userData, void **deviceRegistration);
The kernel style is to use lower_case for everything.
So this would become:
vmci_device_get()
This is obviously a very general comment and applies everywhere.
Sam
^ permalink raw reply
* Re: [net-next RFC V5 3/5] virtio: intorduce an API to set affinity for a virtqueue
From: Paolo Bonzini @ 2012-07-27 14:38 UTC (permalink / raw)
To: Jason Wang, mst, Nicholas A. Bellinger
Cc: krkumar2, habanero, mashirle, kvm, netdev, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <1341484194-8108-4-git-send-email-jasowang@redhat.com>
Il 05/07/2012 12:29, Jason Wang ha scritto:
> Sometimes, virtio device need to configure irq affiniry hint to maximize the
> performance. Instead of just exposing the irq of a virtqueue, this patch
> introduce an API to set the affinity for a virtqueue.
>
> The api is best-effort, the affinity hint may not be set as expected due to
> platform support, irq sharing or irq type. Currently, only pci method were
> implemented and we set the affinity according to:
>
> - if device uses INTX, we just ignore the request
> - if device has per vq vector, we force the affinity hint
> - if the virtqueues share MSI, make the affinity OR over all affinities
> requested
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Hmm, I don't see any benefit from this patch, I need to use
irq_set_affinity (which however is not exported) to actually bind IRQs
to CPUs. Example:
with irq_set_affinity_hint:
43: 89 107 100 97 PCI-MSI-edge virtio0-request
44: 178 195 268 199 PCI-MSI-edge virtio0-request
45: 97 100 97 155 PCI-MSI-edge virtio0-request
46: 234 261 213 218 PCI-MSI-edge virtio0-request
with irq_set_affinity:
43: 721 0 0 1 PCI-MSI-edge virtio0-request
44: 0 746 0 1 PCI-MSI-edge virtio0-request
45: 0 0 658 0 PCI-MSI-edge virtio0-request
46: 0 0 1 547 PCI-MSI-edge virtio0-request
I gathered these quickly after boot, but real benchmarks show the same
behavior, and performance gets actually worse with virtio-scsi
multiqueue+irq_set_affinity_hint than with irq_set_affinity.
I also tried adding IRQ_NO_BALANCING, but the only effect is that I
cannot set the affinity
The queue steering algorithm I use in virtio-scsi is extremely simple
and based on your tx code. See how my nice pinning is destroyed:
# taskset -c 0 dd if=/dev/sda bs=1M count=1000 of=/dev/null iflag=direct
# cat /proc/interrupts
43: 2690 2709 2691 2696 PCI-MSI-edge virtio0-request
44: 109 122 199 124 PCI-MSI-edge virtio0-request
45: 170 183 170 237 PCI-MSI-edge virtio0-request
46: 143 166 125 125 PCI-MSI-edge virtio0-request
All my requests come from CPU#0 and thus go to the first virtqueue, but
the interrupts are serviced all over the place.
Did you set the affinity manually in your experiments, or perhaps there
is a difference between scsi and networking... (interrupt mitigation?)
Paolo
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Andrew Stiegmann @ 2012-07-27 17:20 UTC (permalink / raw)
To: Sam Ravnborg
Cc: pv-drivers, gregkh, linux-kernel, virtualization, vm-crosstalk,
cschamp
In-Reply-To: <20120727103455.GA4639@merkur.ravnborg.org>
Hi Sam,
----- Original Message -----
> From: "Sam Ravnborg" <sam@ravnborg.org>
> To: "Andrew Stiegmann (stieg)" <astiegmann@vmware.com>
> Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, pv-drivers@vmware.com,
> vm-crosstalk@vmware.com, cschamp@vmware.com, gregkh@linuxfoundation.org
> Sent: Friday, July 27, 2012 3:34:55 AM
> Subject: Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
>
> Hi Andrew.
>
> A few things noted in the following..
>
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 2661f6e..fe38c7a 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -517,4 +517,5 @@ source "drivers/misc/lis3lv02d/Kconfig"
> > source "drivers/misc/carma/Kconfig"
> > source "drivers/misc/altera-stapl/Kconfig"
> > source "drivers/misc/mei/Kconfig"
> > +source "drivers/misc/vmw_vmci/Kconfig"
> > endmenu
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 456972f..af9e413 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -51,3 +51,4 @@ obj-y += carma/
> > obj-$(CONFIG_USB_SWITCH_FSA9480) += fsa9480.o
> > obj-$(CONFIG_ALTERA_STAPL) +=altera-stapl/
> > obj-$(CONFIG_INTEL_MEI) += mei/
> > +obj-y += vmw_vmci/
>
> Please use obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
>
> like we do in the other cases. This prevents us from visiting the
> directory
> when this feature is not enabled.
Ok.
> > +++ b/drivers/misc/vmw_vmci/Makefile
> > @@ -0,0 +1,43 @@
> > +################################################################################
> > +#
> > +# Linux driver for VMware's VMCI device.
> > +#
> > +# Copyright (C) 2007-2012, VMware, Inc. All Rights Reserved.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > modify it
> > +# under the terms of the GNU General Public License as published
> > by the
> > +# Free Software Foundation; version 2 of the License and no later
> > version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > but
> > +# WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE
> > or
> > +# NON INFRINGEMENT. See the GNU General Public License for more
> > +# details.
> > +#
> > +# You should have received a copy of the GNU General Public
> > License
> > +# along with this program; if not, write to the Free Software
> > +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > 02110-1301 USA.
> > +#
> > +# The full GNU General Public License is included in this
> > distribution in
> > +# the file called "COPYING".
> > +#
> > +# Maintained by: Andrew Stiegmann <pv-drivers@vmware.com>
> > +#
> > +################################################################################
> Lot's of boilerplate noise for such a simple file...
I removed the section containing FSF address and section below it as well per Greg KH's request.
> > +
> > +#
> > +# Makefile for the VMware VMCI
> > +#
> > +
> > +obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci.o
> > +
> > +vmw_vmci-objs += vmci_context.o
> > +vmw_vmci-objs += vmci_datagram.o
> > +vmw_vmci-objs += vmci_doorbell.o
> > +vmw_vmci-objs += vmci_driver.o
> > +vmw_vmci-objs += vmci_event.o
> > +vmw_vmci-objs += vmci_handle_array.o
> > +vmw_vmci-objs += vmci_hash_table.o
> > +vmw_vmci-objs += vmci_queue_pair.o
> > +vmw_vmci-objs += vmci_resource.o
> > +vmw_vmci-objs += vmci_route.o
>
> please use:
> vmw_vmci-y += vmci_context.o
> vmw_vmci-y += vmci_datagram.o
> vmw_vmci-y += vmci_doorbell.o
>
> This is recommended these days and allows you to enable/disable
> single files later using a config option.
Ok.
> > diff --git a/drivers/misc/vmw_vmci/vmci_common_int.h
> > b/drivers/misc/vmw_vmci/vmci_common_int.h
> > +
> > +#ifndef _VMCI_COMMONINT_H_
> > +#define _VMCI_COMMONINT_H_
> > +
> > +#include <linux/printk.h>
> > +#include <linux/vmw_vmci_defs.h>
>
> Use inverse chrismas tree here.
> Longer include lines first, and soret alphabetically when
> lines are of the same length.
> This applies likely in many cases.
>
> > +#include "vmci_handle_array.h"
> > +
> > +#define ASSERT(cond) BUG_ON(!(cond))
> > +
> > +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK))
> > +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED)
>
> Looks like poor obscufation.
> Use a statis inline function if you need a helper for this.
These definitions are intended more as a helper to make reading the code easier. IMHO ts a lot easier to read
if (CAN_BLOCK(flags))
compared to
if (!(flags & VMCI_QPFLAG_NONBLOCK))
Wouldn't you agree? I'm not sure something this simple warrants a static inline function but I don't see any harm in converting it over to that.
> > +
> > +/*
> > + * Utilility function that checks whether two entities are allowed
> > + * to interact. If one of them is restricted, the other one must
> > + * be trusted.
> > + */
> > +static inline bool vmci_deny_interaction(uint32_t partOne,
> > + uint32_t partTwo)
>
> The kernel types are u32 not uint32_t - these types belongs in
> user-space.
Ok.
> > +++ b/include/linux/vmw_vmci_api.h
> > +
> > +#ifndef __VMW_VMCI_API_H__
> > +#define __VMW_VMCI_API_H__
> > +
> > +#include <linux/vmw_vmci_defs.h>
> > +
> > +#undef VMCI_KERNEL_API_VERSION
> > +#define VMCI_KERNEL_API_VERSION_2 2
> > +#define VMCI_KERNEL_API_VERSION VMCI_KERNEL_API_VERSION_2
> > +
> > +typedef void (VMCI_DeviceShutdownFn) (void *deviceRegistration,
> > void *userData);
> > +
> > +bool VMCI_DeviceGet(uint32_t *apiVersion,
> > + VMCI_DeviceShutdownFn *deviceShutdownCB,
> > + void *userData, void **deviceRegistration);
>
> The kernel style is to use lower_case for everything.
> So this would become:
>
> vmci_device_get()
>
> This is obviously a very general comment and applies everywhere.
I wish I could lower case these symbols but VMCI has already existed outside the mainline Linux tree for some time now and changing these exported symbols would mean that other drivers that depend on VMCI (vSock, vmhgfs) would need to change as well. One thought that did come to mind was exporting both VMCI_Device_Get and vmci_device_get but that would likely just confuse people. So in short I have made function names lower case where possible, but exported symbols could not be changed.
> Sam
>
^ permalink raw reply
* Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Dmitry Torokhov @ 2012-07-27 18:04 UTC (permalink / raw)
To: Alan Cox
Cc: pv-drivers, gregkh, linux-kernel, virtualization, vm-crosstalk,
Andrew Stiegmann (stieg)
In-Reply-To: <20120727105357.2e40dd10@pyramind.ukuu.org.uk>
Hi Alan,
On Fri, Jul 27, 2012 at 10:53:57AM +0100, Alan Cox wrote:
> > +enum {
> > + VMCI_SUCCESS_QUEUEPAIR_ATTACH = 5,
> > + VMCI_SUCCESS_QUEUEPAIR_CREATE = 4,
> > + VMCI_SUCCESS_LAST_DETACH = 3,
> > + VMCI_SUCCESS_ACCESS_GRANTED = 2,
> > + VMCI_SUCCESS_ENTRY_DEAD = 1,
>
> We've got a nice collection of Linux error codes than you, and it would
> make the driver enormously more readable on the Linux side if as low
> level as possible it started using Linux error codes.
If VMCI was only used on Linux we'd definitely do that; however VMCI
core is shared among several operating systems (much like ACPI is) and
we'd like to limit divergencies between them while conforming to the
kernel coding style as much as possible.
We'll make sure that we will not leak VMCI-specific errors to the
standard kernel APIs.
Thanks,
Dmitry
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Greg KH @ 2012-07-27 18:16 UTC (permalink / raw)
To: Andrew Stiegmann
Cc: pv-drivers, linux-kernel, virtualization, vm-crosstalk,
Sam Ravnborg, cschamp
In-Reply-To: <1693394101.5749000.1343409643934.JavaMail.root@vmware.com>
On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
> > The kernel style is to use lower_case for everything.
> > So this would become:
> >
> > vmci_device_get()
> >
> > This is obviously a very general comment and applies everywhere.
>
> I wish I could lower case these symbols but VMCI has already existed
> outside the mainline Linux tree for some time now and changing these
> exported symbols would mean that other drivers that depend on VMCI
> (vSock, vmhgfs) would need to change as well. One thought that did
> come to mind was exporting both VMCI_Device_Get and vmci_device_get
> but that would likely just confuse people. So in short I have made
> function names lower case where possible, but exported symbols could
> not be changed.
Not true at all. You want those drivers to be merged as well, right?
So they will need to have their functions changed, and their code as
well.
Just wait until we get to the "change your functionality around"
requests, those will require those drivers to change. Right now we are
at the "silly and obvious things you did wrong" stage of the review
process :)
So please fix these, and also, post these drivers as well, so we can see
how they interact with the core code.
Actually, if you are going to need lots of refactoring for these
drivers, and the core, I would recommend putting this all in the staging
tree, to allow that to happen over time. That would ensure that your
users keep having working systems, and let you modify the interfaces
better and easier, than having to keep it all out-of-tree.
What do you think?
greg k-h
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Andrew Stiegmann @ 2012-07-27 18:39 UTC (permalink / raw)
To: Greg KH
Cc: pv-drivers, linux-kernel, virtualization, vm-crosstalk,
Sam Ravnborg, cschamp
In-Reply-To: <20120727181639.GA23564@kroah.com>
----- Original Message -----
> From: "Greg KH" <gregkh@linuxfoundation.org>
> To: "Andrew Stiegmann" <astiegmann@vmware.com>
> Cc: "Sam Ravnborg" <sam@ravnborg.org>, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org,
> pv-drivers@vmware.com, vm-crosstalk@vmware.com, cschamp@vmware.com
> Sent: Friday, July 27, 2012 11:16:39 AM
> Subject: Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
>
> On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
> > > The kernel style is to use lower_case for everything.
> > > So this would become:
> > >
> > > vmci_device_get()
> > >
> > > This is obviously a very general comment and applies everywhere.
> >
> > I wish I could lower case these symbols but VMCI has already
> > existed
> > outside the mainline Linux tree for some time now and changing
> > these
> > exported symbols would mean that other drivers that depend on VMCI
> > (vSock, vmhgfs) would need to change as well. One thought that
> > did
> > come to mind was exporting both VMCI_Device_Get and vmci_device_get
> > but that would likely just confuse people. So in short I have made
> > function names lower case where possible, but exported symbols
> > could
> > not be changed.
>
> Not true at all. You want those drivers to be merged as well, right?
> So they will need to have their functions changed, and their code as
> well.
As previously mentioned VMware is working on upstreaming our vSock driver (one of a few drivers that uses vmw_vmci). However there are no plans to upstream the other drivers that depend on vmw_vmci. Because of this these symbols can not change.
> Just wait until we get to the "change your functionality around"
> requests, those will require those drivers to change. Right now we
> are
> at the "silly and obvious things you did wrong" stage of the review
> process :)
>
> So please fix these, and also, post these drivers as well, so we can
> see
> how they interact with the core code.
>
> Actually, if you are going to need lots of refactoring for these
> drivers, and the core, I would recommend putting this all in the
> staging
> tree, to allow that to happen over time. That would ensure that your
> users keep having working systems, and let you modify the interfaces
> better and easier, than having to keep it all out-of-tree.
>
> What do you think?
We will discuss this internally and let you know.
> greg k-h
>
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Greg KH @ 2012-07-27 18:52 UTC (permalink / raw)
To: Andrew Stiegmann
Cc: pv-drivers, linux-kernel, virtualization, vm-crosstalk,
Sam Ravnborg, cschamp
In-Reply-To: <1258715407.5847225.1343414363506.JavaMail.root@vmware.com>
On Fri, Jul 27, 2012 at 11:39:23AM -0700, Andrew Stiegmann wrote:
>
>
> ----- Original Message -----
> > From: "Greg KH" <gregkh@linuxfoundation.org>
> > To: "Andrew Stiegmann" <astiegmann@vmware.com>
> > Cc: "Sam Ravnborg" <sam@ravnborg.org>, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org,
> > pv-drivers@vmware.com, vm-crosstalk@vmware.com, cschamp@vmware.com
> > Sent: Friday, July 27, 2012 11:16:39 AM
> > Subject: Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
> >
> > On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
> > > > The kernel style is to use lower_case for everything.
> > > > So this would become:
> > > >
> > > > vmci_device_get()
> > > >
> > > > This is obviously a very general comment and applies everywhere.
> > >
> > > I wish I could lower case these symbols but VMCI has already
> > > existed
> > > outside the mainline Linux tree for some time now and changing
> > > these
> > > exported symbols would mean that other drivers that depend on VMCI
> > > (vSock, vmhgfs) would need to change as well. One thought that
> > > did
> > > come to mind was exporting both VMCI_Device_Get and vmci_device_get
> > > but that would likely just confuse people. So in short I have made
> > > function names lower case where possible, but exported symbols
> > > could
> > > not be changed.
> >
> > Not true at all. You want those drivers to be merged as well, right?
> > So they will need to have their functions changed, and their code as
> > well.
>
> As previously mentioned VMware is working on upstreaming our vSock
> driver (one of a few drivers that uses vmw_vmci).
Great.
> However there are no plans to upstream the other drivers that depend
> on vmw_vmci.
Why not? That seems quite short-sighted.
> Because of this these symbols can not change.
Then I would argue that we can not accept this code at all, because it
will change over time, both symbol names, and functionality (see my
previous comment about how that is going to have to change.)
sorry,
greg k-h
^ permalink raw reply
* Re: Re: [Qemu-devel] [RFC PATCH 0/6] virtio-trace: Support virtio-trace
From: Blue Swirl @ 2012-07-27 18:58 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Herbert Xu, Arnd Bergmann, Greg Kroah-Hartman,
Frederic Weisbecker, linux-kernel, Borislav Petkov, qemu-devel,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers, Steven Rostedt,
Anthony Liguori, yrl.pp-manager.tt, Amit Shah, virtualization
In-Reply-To: <500FAB33.4060301@hitachi.com>
On Wed, Jul 25, 2012 at 8:15 AM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (2012/07/25 5:26), Blue Swirl wrote:>
>>> The following patch set provides a low-overhead system for collecting kernel
>>> tracing data of guests by a host in a virtualization environment.
>>>
>>> A guest OS generally shares some devices with other guests or a host, so
>>> reasons of any problems occurring in a guest may be from other guests or a
>>> host.
>>> Then, to collect some tracing data of a number of guests and a host is needed
>>> when some problems occur in a virtualization environment. One of methods to
>>> realize that is to collect tracing data of guests in a host. To do this,
>>> network
>>> is generally used. However, high load will be taken to applications on guests
>>> using network I/O because there are many network stack layers. Therefore,
>>> a communication method for collecting the data without using network is
>>> needed.
>>
>> I implemented something similar earlier by passing trace data from
>> OpenBIOS to QEMU using the firmware configuration device. The data
>> format was the same as QEMU used for simpletrace event structure
>> instead of ftrace. I didn't commit it because of a few problems.
>
> Sounds interesting :)
> I guess you traced BIOS events, right?
Yes, I converted a few DPRINTFs to tracepoints as a proof of concept.
>
>> I'm not familiar with ftrace, is it possible to trace two guest
>> applications (BIOS and kernel) at the same time?
>
> Since ftrace itself is a tracing feature in the linux kernel, it
> can trace two or more applications (processes) if those run on linux
> kernel. However, I think OpenBIOS runs *under* the guest kernel.
> If so, ftrace currently can't trace OpenBIOS from guest side.
No, OpenBIOS boots the machine and then passes control to boot loader
and that to kernel. The kernel will make a few calls to OpenBIOS at
start but not later. OpenBIOS is used by QEMU as Sparc and PowerPC
BIOS.
>
> I think it may need another enhancement on both OpenBIOS and linux
> kernel to trace BIOS event from linux kernel.
>
Ideally both OpenBIOS and Linux should be able to feed trace events
back to QEMU independently.
>> Or could this be
>> handled by opening two different virtio-serial pipes, one for BIOS and
>> the other for the kernel?
>
> Of course, virtio-serial itself can open multiple channels, thus, if
> OpenBIOS can handle virtio, it can pass trace data via another
> channel.
Currently OpenBIOS probes the PCI bus and identifies virtio devices
but ignores them, adding virtio-serial support shouldn't be too hard.
There's a time window between CPU boot and PCI probe when the the
device will not be available though.
>
>> In my version, the tracepoint ID would have been used to demultiplex
>> QEMU tracepoints from BIOS tracepoints, but something like separate ID
>> spaces would have been better.
>
> I guess your feature notifies events to QEMU and QEMU records that in
> their own buffer. Therefore it must have different tracepoint IDs.
> On the other hand, with this feature, QEMU just passes trace-data to
> host-side pipe. Since outer tracing tool separately collects trace
> data, we don't need to demultiplex the data.
>
> Perhaps, in the analyzing phase (after tracing), we have to mix events
> again. At that time, we'll add some guest-ID for each event-ID, but
> it can be done offline.
Yes, the multiplexing/demultiplexing is only needed in my version
because the feeds are not independent.
>
> Best Regards,
>
> --
> Masami HIRAMATSU
> Software Platform Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Sam Ravnborg @ 2012-07-27 19:53 UTC (permalink / raw)
To: Andrew Stiegmann
Cc: pv-drivers, gregkh, linux-kernel, virtualization, vm-crosstalk,
cschamp
In-Reply-To: <1693394101.5749000.1343409643934.JavaMail.root@vmware.com>
> > > +
> > > +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK))
> > > +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED)
> >
> > Looks like poor obscufation.
> > Use a statis inline function if you need a helper for this.
>
> These definitions are intended more as a helper to make reading the code easier. IMHO ts a lot easier to read
>
> if (CAN_BLOCK(flags))
>
> compared to
>
> if (!(flags & VMCI_QPFLAG_NONBLOCK))
>
> Wouldn't you agree? I'm not sure something this simple warrants a static inline
> function but I don't see any harm in converting it over to that.
I would put it the other way around. I cannot see that such simple stuff warrants a #define.
A static inline is (almost) always preferable to hide code in a macro.
For once you get better type-checks.
And semantics are also much simpler. With a macro you can do so many silly things.
Sam
^ permalink raw reply
* Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Andrew Stiegmann @ 2012-07-27 20:07 UTC (permalink / raw)
To: Sam Ravnborg
Cc: pv-drivers, gregkh, linux-kernel, virtualization, vm-crosstalk,
cschamp
In-Reply-To: <20120727195320.GA5822@merkur.ravnborg.org>
----- Original Message -----
> From: "Sam Ravnborg" <sam@ravnborg.org>
> To: "Andrew Stiegmann" <astiegmann@vmware.com>
> Cc: linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, pv-drivers@vmware.com,
> vm-crosstalk@vmware.com, cschamp@vmware.com, gregkh@linuxfoundation.org
> Sent: Friday, July 27, 2012 12:53:20 PM
> Subject: Re: [vmw_vmci 11/11] Apply the header code to make VMCI build
>
> > > > +
> > > > +#define CAN_BLOCK(_f) (!((_f) & VMCI_QPFLAG_NONBLOCK))
> > > > +#define QP_PINNED(_f) ((_f) & VMCI_QPFLAG_PINNED)
> > >
> > > Looks like poor obscufation.
> > > Use a statis inline function if you need a helper for this.
> >
> > These definitions are intended more as a helper to make reading the
> > code easier. IMHO ts a lot easier to read
> >
> > if (CAN_BLOCK(flags))
> >
> > compared to
> >
> > if (!(flags & VMCI_QPFLAG_NONBLOCK))
> >
> > Wouldn't you agree? I'm not sure something this simple warrants a
> > static inline
> > function but I don't see any harm in converting it over to that.
>
> I would put it the other way around. I cannot see that such simple
> stuff warrants a #define.
> A static inline is (almost) always preferable to hide code in a
> macro.
>
> For once you get better type-checks.
> And semantics are also much simpler. With a macro you can do so many
> silly things.
Fair enough. I'll make them into static inline functions.
> Sam
>
^ permalink raw reply
* Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Dmitry Torokhov @ 2012-07-27 20:29 UTC (permalink / raw)
To: Greg KH
Cc: pv-drivers, linux-kernel, virtualization, vm-crosstalk,
Andrew Stiegmann, Sam Ravnborg
In-Reply-To: <20120727181639.GA23564@kroah.com>
On Fri, Jul 27, 2012 at 11:16:39AM -0700, Greg KH wrote:
> On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
> > > The kernel style is to use lower_case for everything.
> > > So this would become:
> > >
> > > vmci_device_get()
> > >
> > > This is obviously a very general comment and applies everywhere.
> >
> > I wish I could lower case these symbols but VMCI has already existed
> > outside the mainline Linux tree for some time now and changing these
> > exported symbols would mean that other drivers that depend on VMCI
> > (vSock, vmhgfs) would need to change as well. One thought that did
> > come to mind was exporting both VMCI_Device_Get and vmci_device_get
> > but that would likely just confuse people. So in short I have made
> > function names lower case where possible, but exported symbols could
> > not be changed.
>
> Not true at all. You want those drivers to be merged as well, right?
> So they will need to have their functions changed, and their code as
> well.
>
> Just wait until we get to the "change your functionality around"
> requests, those will require those drivers to change. Right now we are
> at the "silly and obvious things you did wrong" stage of the review
> process :)
>
> So please fix these, and also, post these drivers as well, so we can see
> how they interact with the core code.
>
> Actually, if you are going to need lots of refactoring for these
> drivers, and the core, I would recommend putting this all in the staging
> tree, to allow that to happen over time. That would ensure that your
> users keep having working systems, and let you modify the interfaces
> better and easier, than having to keep it all out-of-tree.
>
> What do you think?
Actually I think that we'd prefer to keep this in a patch-based form, at
least for now, because majority of our users get these drivers with
VMware Tools and will continue doing so until ditsributions start
enabling VMCI in their kernels. Which they probably won't until VMCI
moves form staging. We'd also have to constantly adjust drivers that we
are not working on getting upstream at this time to work with the
rapidly changing version of VMCI in staging, which will just add work
for us.
So we'd like to get more feedback and have a chance to address issues
and then decide whether staying in staging makes sense or not.
Thanks.
--
Dmitry
^ permalink raw reply
* KVM Forum 2012 Call For Participation
From: KVM Forum 2012 Program Committee @ 2012-07-27 23:31 UTC (permalink / raw)
To: kvm, libvir-list, qemu-devel, virtualization; +Cc: kvm-forum-2012-pc
=================================================================
KVM Forum 2012: Call For Participation
November 7-9, 2012 - Hotel Fira Palace - Barcelona, Spain
(All submissions must be received before midnight Aug 31st, 2012)
=================================================================
KVM is an industry leading open source hypervisor that provides
an ideal platform for datacenter virtualization, virtual desktop
infrastructure, and cloud computing. Once again, it's time to bring
together the community of developers and users that define the KVM
ecosystem for our annual technical conference. We will discuss the
current state of affairs and plan for the future of KVM, its surrounding
infrastructure, and management tools. We are also excited to announce
the oVirt Workshop will run in parallel with the KVM Forum, bringing in
a community focused on enterprise datacenter virtualization management
built on KVM. For topics which overlap we will have shared sessions.
So mark your calendar and join us in advancing KVM.
http://events.linuxfoundation.org/events/kvm-forum/
Once again we are colocated with The Linux Foundation's LinuxCon,
Based on feedback from last year, this time it's LinuxCon Europe!
KVM Forum attendees will be able to attend oVirt Workshop sessions and
are eligible to attend LinuxCon Europe for a discounted rate.
http://events.linuxfoundation.org/events/kvm-forum/register
We invite you to lead part of the discussion by submitting a speaking
proposal for KVM Forum 2012.
http://events.linuxfoundation.org/cfp
Suggested topics:
KVM
- Scaling and performance
- Nested virtualization
- I/O improvements
- PCI device assignment
- Driver domains
- Time keeping
- Resource management (cpu, memory, i/o)
- Memory management (page sharing, swapping, huge pages, etc)
- VEPA, VN-Link, vswitch
- Security
- Architecture ports
QEMU
- Device model improvements
- New devices and chipsets
- Scaling and performance
- Desktop virtualization
- Spice
- Increasing robustness and hardening
- Security model
- Management interfaces
- QMP protocol and implementation
- Image formats
- Firmware (SeaBIOS, OVMF, UEFI, etc)
- Live migration
- Live snapshots and merging
- Fault tolerance, high availability, continuous backup
- Real-time guest support
Virtio
- Speeding up existing devices
- Alternatives
- Virtio on non-Linux or non-virtualized
Management infrastructure
- oVirt (shared track w/ oVirt Workshop)
- Libvirt
- KVM autotest
- OpenStack
- Network virtualization management
- Enterprise storage management
Cloud computing
- Scalable storage
- Virtual networking
- Security
- Provisioning
SUBMISSION REQUIREMENTS
Abstracts due: Aug 31st, 2012
Notification: Sep 14th, 2012
Please submit a short abstract (~150 words) describing your presentation
proposal. In your submission please note how long your talk will take.
Slots vary in length up to 45 minutes. Also include in your proposal
the proposal type -- one of:
- technical talk
- end-user talk
- birds of a feather (BOF) session
Submit your proposal here:
http://events.linuxfoundation.org/cfp
You will receive a notification whether or not your presentation proposal
was accepted by Sep 14th.
END-USER COLLABORATION
One of the big challenges as developers is to know what, where and how
people actually use our software. We will reserve a few slots for end
users talking about their deployment challenges and achievements.
If you are using KVM in production you are encouraged submit a speaking
proposal. Simply mark it as an end-user collaboration proposal. As an
end user, this is a unique opportunity to get your input to developers.
BOF SESSION
We will reserve some slots in the evening after the main conference
tracks, for birds of a feather (BOF) sessions. These sessions will be
less formal than presentation tracks and targetted for people who would
like to discuss specific issues with other developers and/or users.
If you are interested in getting developers and/or uses together to
discuss a specific problem, please submit a BOF proposal.
LIGHTNING TALKS
In addition to submitted talks we will also have some room for lightning
talks. These are short (5 minute) discussions to highlight new work or
ideas that aren't complete enough to warrant a full presentation slot.
Lightning talk submissions and scheduling will be handled on-site at
KVM Forum.
HOTEL / TRAVEL
The KVM Forum 2012 will be held in Barcelona, Spain at the Hotel Fira Palace.
http://events.linuxfoundation.org/events/kvm-forum/hotel
Thank you for your interest in KVM. We're looking forward to your
submissions and seeing you at the KVM Forum 2012 in November!
Thanks,
your KVM Forum 2012 Program Commitee
Please contact us with any questions or comments.
KVM-Forum-2012-PC@redhat.com
^ permalink raw reply
* [PATCH V4 0/3] Improve virtio-blk performance
From: Asias He @ 2012-07-28 2:21 UTC (permalink / raw)
To: linux-kernel
Cc: Jens Axboe, kvm, Michael S. Tsirkin, virtualization, Tejun Heo,
Shaohua Li
Hi, Jens & Rusty
This version is rebased against linux-next which resolves the conflict with
Paolo Bonzini's 'virtio-blk: allow toggling host cache between writeback and
writethrough' patch.
Patch 1/3 and 2/3 applies on linus's master as well. Since Rusty will pick up
patch 3/3 so the changes to block core (adding blk_bio_map_sg()) will have a
user.
Jens, could you please consider picking up the dependencies 1/3 and 2/3 in your
tree. Thanks!
This patchset implements bio-based IO path for virito-blk to improve
performance.
Fio test shows bio-based IO path gives the following performance improvement:
1) Ramdisk device
With bio-based IO path, sequential read/write, random read/write
IOPS boost : 28%, 24%, 21%, 16%
Latency improvement: 32%, 17%, 21%, 16%
2) Fusion IO device
With bio-based IO path, sequential read/write, random read/write
IOPS boost : 11%, 11%, 13%, 10%
Latency improvement: 10%, 10%, 12%, 10%
Asias He (3):
block: Introduce __blk_segment_map_sg() helper
block: Add blk_bio_map_sg() helper
virtio-blk: Add bio-based IO path for virtio-blk
block/blk-merge.c | 117 +++++++++++++++++--------
drivers/block/virtio_blk.c | 203 +++++++++++++++++++++++++++++++++++---------
include/linux/blkdev.h | 2 +
3 files changed, 247 insertions(+), 75 deletions(-)
--
1.7.10.4
^ permalink raw reply
* [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Asias He @ 2012-07-28 2:21 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, Michael S. Tsirkin, virtualization, Christoph Hellwig
In-Reply-To: <1343442065-15646-1-git-send-email-asias@redhat.com>
This patch introduces bio-based IO path for virtio-blk.
Compared to request-based IO path, bio-based IO path uses driver
provided ->make_request_fn() method to bypasses the IO scheduler. It
handles the bio to device directly without allocating a request in block
layer. This reduces the IO path in guest kernel to achieve high IOPS
and lower latency. The downside is that guest can not use the IO
scheduler to merge and sort requests. However, this is not a big problem
if the backend disk in host side uses faster disk device.
When the bio-based IO path is not enabled, virtio-blk still uses the
original request-based IO path, no performance difference is observed.
Performance evaluation:
-----------------------------
1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
kvm tool.
Short version:
With bio-based IO path, sequential read/write, random read/write
IOPS boost : 28%, 24%, 21%, 16%
Latency improvement: 32%, 17%, 21%, 16%
Long version:
With bio-based IO path:
seq-read : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
rand-write: io=3095.7MB, bw=96198KB/s, iops=192396 , runt= 32952msec
clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
With request-based IO path:
seq-read : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985
2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
kvm tool.
Short version:
With bio-based IO path, sequential read/write, random read/write
IOPS boost : 11%, 11%, 13%, 10%
Latency improvement: 10%, 10%, 12%, 10%
Long Version:
With bio-based IO path:
read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
With request-based IO path:
read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409
How to use:
-----------------------------
Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
use_bio=1' to enable ->make_request_fn() based I/O path.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
Signed-off-by: Asias He <asias@redhat.com>
---
drivers/block/virtio_blk.c | 203 +++++++++++++++++++++++++++++++++++---------
1 file changed, 163 insertions(+), 40 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index c0bbeb4..95cfeed 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -14,6 +14,9 @@
#define PART_BITS 4
+static bool use_bio;
+module_param(use_bio, bool, S_IRUGO);
+
static int major;
static DEFINE_IDA(vd_index_ida);
@@ -23,6 +26,7 @@ struct virtio_blk
{
struct virtio_device *vdev;
struct virtqueue *vq;
+ wait_queue_head_t queue_wait;
/* The disk structure for the kernel. */
struct gendisk *disk;
@@ -51,53 +55,87 @@ struct virtio_blk
struct virtblk_req
{
struct request *req;
+ struct bio *bio;
struct virtio_blk_outhdr out_hdr;
struct virtio_scsi_inhdr in_hdr;
u8 status;
+ struct scatterlist sg[];
};
-static void blk_done(struct virtqueue *vq)
+static inline int virtblk_result(struct virtblk_req *vbr)
+{
+ switch (vbr->status) {
+ case VIRTIO_BLK_S_OK:
+ return 0;
+ case VIRTIO_BLK_S_UNSUPP:
+ return -ENOTTY;
+ default:
+ return -EIO;
+ }
+}
+
+static inline void virtblk_request_done(struct virtio_blk *vblk,
+ struct virtblk_req *vbr)
+{
+ struct request *req = vbr->req;
+ int error = virtblk_result(vbr);
+
+ if (req->cmd_type == REQ_TYPE_BLOCK_PC) {
+ req->resid_len = vbr->in_hdr.residual;
+ req->sense_len = vbr->in_hdr.sense_len;
+ req->errors = vbr->in_hdr.errors;
+ } else if (req->cmd_type == REQ_TYPE_SPECIAL) {
+ req->errors = (error != 0);
+ }
+
+ __blk_end_request_all(req, error);
+ mempool_free(vbr, vblk->pool);
+}
+
+static inline void virtblk_bio_done(struct virtio_blk *vblk,
+ struct virtblk_req *vbr)
+{
+ bio_endio(vbr->bio, virtblk_result(vbr));
+ mempool_free(vbr, vblk->pool);
+}
+
+static void virtblk_done(struct virtqueue *vq)
{
struct virtio_blk *vblk = vq->vdev->priv;
+ unsigned long bio_done = 0, req_done = 0;
struct virtblk_req *vbr;
- unsigned int len;
unsigned long flags;
+ unsigned int len;
spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
- int error;
-
- switch (vbr->status) {
- case VIRTIO_BLK_S_OK:
- error = 0;
- break;
- case VIRTIO_BLK_S_UNSUPP:
- error = -ENOTTY;
- break;
- default:
- error = -EIO;
- break;
- }
-
- switch (vbr->req->cmd_type) {
- case REQ_TYPE_BLOCK_PC:
- vbr->req->resid_len = vbr->in_hdr.residual;
- vbr->req->sense_len = vbr->in_hdr.sense_len;
- vbr->req->errors = vbr->in_hdr.errors;
- break;
- case REQ_TYPE_SPECIAL:
- vbr->req->errors = (error != 0);
- break;
- default:
- break;
+ if (vbr->bio) {
+ virtblk_bio_done(vblk, vbr);
+ bio_done++;
+ } else {
+ virtblk_request_done(vblk, vbr);
+ req_done++;
}
-
- __blk_end_request_all(vbr->req, error);
- mempool_free(vbr, vblk->pool);
}
/* In case queue is stopped waiting for more buffers. */
- blk_start_queue(vblk->disk->queue);
+ if (req_done)
+ blk_start_queue(vblk->disk->queue);
spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
+
+ if (bio_done)
+ wake_up(&vblk->queue_wait);
+}
+
+static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
+ gfp_t gfp_mask)
+{
+ struct virtblk_req *vbr;
+
+ vbr = mempool_alloc(vblk->pool, gfp_mask);
+ if (vbr && use_bio)
+ sg_init_table(vbr->sg, vblk->sg_elems);
+
+ return vbr;
}
static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
@@ -106,13 +144,13 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
unsigned long num, out = 0, in = 0;
struct virtblk_req *vbr;
- vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
+ vbr = virtblk_alloc_req(vblk, GFP_ATOMIC);
if (!vbr)
/* When another request finishes we'll try again. */
return false;
vbr->req = req;
-
+ vbr->bio = NULL;
if (req->cmd_flags & REQ_FLUSH) {
vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
vbr->out_hdr.sector = 0;
@@ -172,7 +210,8 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
}
}
- if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr, GFP_ATOMIC)<0) {
+ if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr,
+ GFP_ATOMIC) < 0) {
mempool_free(vbr, vblk->pool);
return false;
}
@@ -180,7 +219,7 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
return true;
}
-static void do_virtblk_request(struct request_queue *q)
+static void virtblk_request(struct request_queue *q)
{
struct virtio_blk *vblk = q->queuedata;
struct request *req;
@@ -203,6 +242,82 @@ static void do_virtblk_request(struct request_queue *q)
virtqueue_kick(vblk->vq);
}
+static void virtblk_add_buf_wait(struct virtio_blk *vblk,
+ struct virtblk_req *vbr,
+ unsigned long out,
+ unsigned long in)
+{
+ DEFINE_WAIT(wait);
+
+ for (;;) {
+ prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
+ TASK_UNINTERRUPTIBLE);
+
+ spin_lock_irq(vblk->disk->queue->queue_lock);
+ if (virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
+ GFP_ATOMIC) < 0) {
+ spin_unlock_irq(vblk->disk->queue->queue_lock);
+ io_schedule();
+ } else {
+ virtqueue_kick(vblk->vq);
+ spin_unlock_irq(vblk->disk->queue->queue_lock);
+ break;
+ }
+
+ }
+
+ finish_wait(&vblk->queue_wait, &wait);
+}
+
+static void virtblk_make_request(struct request_queue *q, struct bio *bio)
+{
+ struct virtio_blk *vblk = q->queuedata;
+ unsigned int num, out = 0, in = 0;
+ struct virtblk_req *vbr;
+
+ BUG_ON(bio->bi_phys_segments + 2 > vblk->sg_elems);
+ BUG_ON(bio->bi_rw & (REQ_FLUSH | REQ_FUA));
+
+ vbr = virtblk_alloc_req(vblk, GFP_NOIO);
+ if (!vbr) {
+ bio_endio(bio, -ENOMEM);
+ return;
+ }
+
+ vbr->bio = bio;
+ vbr->req = NULL;
+ vbr->out_hdr.type = 0;
+ vbr->out_hdr.sector = bio->bi_sector;
+ vbr->out_hdr.ioprio = bio_prio(bio);
+
+ sg_set_buf(&vbr->sg[out++], &vbr->out_hdr, sizeof(vbr->out_hdr));
+
+ num = blk_bio_map_sg(q, bio, vbr->sg + out);
+
+ sg_set_buf(&vbr->sg[num + out + in++], &vbr->status,
+ sizeof(vbr->status));
+
+ if (num) {
+ if (bio->bi_rw & REQ_WRITE) {
+ vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
+ out += num;
+ } else {
+ vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
+ in += num;
+ }
+ }
+
+ spin_lock_irq(vblk->disk->queue->queue_lock);
+ if (unlikely(virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
+ GFP_ATOMIC) < 0)) {
+ spin_unlock_irq(vblk->disk->queue->queue_lock);
+ virtblk_add_buf_wait(vblk, vbr, out, in);
+ return;
+ }
+ virtqueue_kick(vblk->vq);
+ spin_unlock_irq(vblk->disk->queue->queue_lock);
+}
+
/* return id (s/n) string for *disk to *id_str
*/
static int virtblk_get_id(struct gendisk *disk, char *id_str)
@@ -360,7 +475,7 @@ static int init_vq(struct virtio_blk *vblk)
int err = 0;
/* We expect one virtqueue, for output. */
- vblk->vq = virtio_find_single_vq(vblk->vdev, blk_done, "requests");
+ vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
if (IS_ERR(vblk->vq))
err = PTR_ERR(vblk->vq);
@@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device *vdev)
u8 writeback = virtblk_get_cache_mode(vdev);
struct virtio_blk *vblk = vdev->priv;
- if (writeback)
+ if (writeback && !use_bio)
blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
else
blk_queue_flush(vblk->disk->queue, 0);
@@ -477,6 +592,8 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
struct virtio_blk *vblk;
struct request_queue *q;
int err, index;
+ int pool_size;
+
u64 cap;
u32 v, blk_size, sg_elems, opt_io_size;
u16 min_io_size;
@@ -506,10 +623,12 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
goto out_free_index;
}
+ init_waitqueue_head(&vblk->queue_wait);
vblk->vdev = vdev;
vblk->sg_elems = sg_elems;
sg_init_table(vblk->sg, vblk->sg_elems);
mutex_init(&vblk->config_lock);
+
INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
vblk->config_enable = true;
@@ -517,7 +636,10 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
if (err)
goto out_free_vblk;
- vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
+ pool_size = sizeof(struct virtblk_req);
+ if (use_bio)
+ pool_size += sizeof(struct scatterlist) * sg_elems;
+ vblk->pool = mempool_create_kmalloc_pool(1, pool_size);
if (!vblk->pool) {
err = -ENOMEM;
goto out_free_vq;
@@ -530,12 +652,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
goto out_mempool;
}
- q = vblk->disk->queue = blk_init_queue(do_virtblk_request, NULL);
+ q = vblk->disk->queue = blk_init_queue(virtblk_request, NULL);
if (!q) {
err = -ENOMEM;
goto out_put_disk;
}
+ if (use_bio)
+ blk_queue_make_request(q, virtblk_make_request);
q->queuedata = vblk;
virtblk_name_format("vd", index, vblk->disk->disk_name, DISK_NAME_LEN);
@@ -620,7 +744,6 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
if (!err && opt_io_size)
blk_queue_io_opt(q, blk_size * opt_io_size);
-
add_disk(vblk->disk);
err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial);
if (err)
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Asias He @ 2012-07-28 2:38 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Paolo Bonzini, Christoph Hellwig
In-Reply-To: <87d33initv.fsf@rustcorp.com.au>
On 07/27/2012 08:33 AM, Rusty Russell wrote:
> On Fri, 13 Jul 2012 16:38:51 +0800, Asias He <asias@redhat.com> wrote:
>> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
>> use_bio=1' to enable ->make_request_fn() based I/O path.
>
> This patch conflicts with Paolo's Bonzini's 'virtio-blk: allow toggling
> host cache between writeback and writethrough' which is also queued (see
> linux-next).
Rebased against Paolo's patch in V4.
> I'm not sure what the correct behavior for bio & cacheflush is, if any.
REQ_FLUSH is not supported in the bio path.
> But as to the patch itself: it's a hack.
>
> 1) Leaving the guest's admin to turn on the switch is a terrible choice.
> 2) The block layer should stop merging and sorting when a device is
> fast, not the driver.
> 3) I pointed out that slow disks have low IOPS, so why is this
> conditional? Sure, more guest exits, but it's still a small number
> for a slow device.
> 4) The only case where we want merging is on a slow device when the host
> isn't doing it.
>
> Now, despite this, I'm prepared to commit it. But in my mind it's a
> hack: we should aim for use_bio to be based on a feature bit fed from
> the host, and use the module parameter only if we want to override it.
OK. A feature bit from host sound like a choice but a switch is also
needed on host side. And for other OS, e.g. Windows, the bio thing does
not apply at all.
Anyway, I have to admit that adding a module parameter here is not the
best choice. Let's think more.
--
Asias
^ permalink raw reply
* Re: [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Sasha Levin @ 2012-07-28 6:35 UTC (permalink / raw)
To: Asias He
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <1343442065-15646-4-git-send-email-asias@redhat.com>
On 07/28/2012 04:21 AM, Asias He wrote:
> This patch introduces bio-based IO path for virtio-blk.
>
> Compared to request-based IO path, bio-based IO path uses driver
> provided ->make_request_fn() method to bypasses the IO scheduler. It
> handles the bio to device directly without allocating a request in block
> layer. This reduces the IO path in guest kernel to achieve high IOPS
> and lower latency. The downside is that guest can not use the IO
> scheduler to merge and sort requests. However, this is not a big problem
> if the backend disk in host side uses faster disk device.
>
> When the bio-based IO path is not enabled, virtio-blk still uses the
> original request-based IO path, no performance difference is observed.
>
> Performance evaluation:
> -----------------------------
> 1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
> kvm tool.
>
> Short version:
> With bio-based IO path, sequential read/write, random read/write
> IOPS boost : 28%, 24%, 21%, 16%
> Latency improvement: 32%, 17%, 21%, 16%
>
> Long version:
> With bio-based IO path:
> seq-read : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
> seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
> rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
> rand-write: io=3095.7MB, bw=96198KB/s, iops=192396 , runt= 32952msec
> clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
> clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
> clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
> clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
> cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
> cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
> cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
> cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
> With request-based IO path:
> seq-read : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
> seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
> rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
> rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
> clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
> clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
> clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
> clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
> cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
> cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
> cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
> cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985
>
> 2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
> kvm tool.
>
> Short version:
> With bio-based IO path, sequential read/write, random read/write
> IOPS boost : 11%, 11%, 13%, 10%
> Latency improvement: 10%, 10%, 12%, 10%
> Long Version:
> With bio-based IO path:
> read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
> write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
> read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
> write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
> clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
> clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
> clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
> clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
> cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
> cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
> cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
> cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
> With request-based IO path:
> read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
> write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
> read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
> write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
> clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
> clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
> clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
> clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
> cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
> cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
> cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
> cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409
What are the cases where we'll see a performance degradation with using the bio path? Could we measure performance for those as well?
> How to use:
> -----------------------------
> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
> use_bio=1' to enable ->make_request_fn() based I/O path.
If there are, in fact, no cases where performance is degraded, can use_bio=1 be the default?
^ permalink raw reply
* Re: [PATCH V3 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Paolo Bonzini @ 2012-07-28 6:42 UTC (permalink / raw)
To: Asias He
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Christoph Hellwig
In-Reply-To: <501350B1.7080908@redhat.com>
> > I'm not sure what the correct behavior for bio & cacheflush is, if
> > any.
>
> REQ_FLUSH is not supported in the bio path.
Ouch, that's correct:
@@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device *vdev)
u8 writeback = virtblk_get_cache_mode(vdev);
struct virtio_blk *vblk = vdev->priv;
- if (writeback)
+ if (writeback && !use_bio)
blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
else
blk_queue_flush(vblk->disk->queue, 0);
then it is not safe against power losses.
Paolo
^ permalink raw reply
* Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Greg KH @ 2012-07-28 19:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: pv-drivers, linux-kernel, virtualization, vm-crosstalk,
Andrew Stiegmann, Sam Ravnborg
In-Reply-To: <20120727202926.GC8262@dtor-ws.eng.vmware.com>
On Fri, Jul 27, 2012 at 01:29:27PM -0700, Dmitry Torokhov wrote:
> On Fri, Jul 27, 2012 at 11:16:39AM -0700, Greg KH wrote:
> > On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
> > > > The kernel style is to use lower_case for everything.
> > > > So this would become:
> > > >
> > > > vmci_device_get()
> > > >
> > > > This is obviously a very general comment and applies everywhere.
> > >
> > > I wish I could lower case these symbols but VMCI has already existed
> > > outside the mainline Linux tree for some time now and changing these
> > > exported symbols would mean that other drivers that depend on VMCI
> > > (vSock, vmhgfs) would need to change as well. One thought that did
> > > come to mind was exporting both VMCI_Device_Get and vmci_device_get
> > > but that would likely just confuse people. So in short I have made
> > > function names lower case where possible, but exported symbols could
> > > not be changed.
> >
> > Not true at all. You want those drivers to be merged as well, right?
> > So they will need to have their functions changed, and their code as
> > well.
> >
> > Just wait until we get to the "change your functionality around"
> > requests, those will require those drivers to change. Right now we are
> > at the "silly and obvious things you did wrong" stage of the review
> > process :)
> >
> > So please fix these, and also, post these drivers as well, so we can see
> > how they interact with the core code.
> >
> > Actually, if you are going to need lots of refactoring for these
> > drivers, and the core, I would recommend putting this all in the staging
> > tree, to allow that to happen over time. That would ensure that your
> > users keep having working systems, and let you modify the interfaces
> > better and easier, than having to keep it all out-of-tree.
> >
> > What do you think?
>
> Actually I think that we'd prefer to keep this in a patch-based form, at
> least for now, because majority of our users get these drivers with
> VMware Tools and will continue doing so until ditsributions start
> enabling VMCI in their kernels. Which they probably won't until VMCI
> moves form staging. We'd also have to constantly adjust drivers that we
> are not working on getting upstream at this time to work with the
> rapidly changing version of VMCI in staging, which will just add work
> for us.
That wouldn't be an issue if you just include all of the drivers in the
tree at the same time, right?
Just like what the hyper-v developers did.
greg
^ permalink raw reply
* Re: [Pv-drivers] [vmw_vmci 11/11] Apply the header code to make VMCI build
From: Dmitry Torokhov @ 2012-07-28 21:10 UTC (permalink / raw)
To: Greg KH
Cc: pv-drivers, linux-kernel, virtualization, vm-crosstalk,
Andrew Stiegmann, Sam Ravnborg
In-Reply-To: <20120728195535.GC4762@kroah.com>
On Sat, Jul 28, 2012 at 12:55:35PM -0700, Greg KH wrote:
> On Fri, Jul 27, 2012 at 01:29:27PM -0700, Dmitry Torokhov wrote:
> > On Fri, Jul 27, 2012 at 11:16:39AM -0700, Greg KH wrote:
> > > On Fri, Jul 27, 2012 at 10:20:43AM -0700, Andrew Stiegmann wrote:
> > > > > The kernel style is to use lower_case for everything.
> > > > > So this would become:
> > > > >
> > > > > vmci_device_get()
> > > > >
> > > > > This is obviously a very general comment and applies everywhere.
> > > >
> > > > I wish I could lower case these symbols but VMCI has already existed
> > > > outside the mainline Linux tree for some time now and changing these
> > > > exported symbols would mean that other drivers that depend on VMCI
> > > > (vSock, vmhgfs) would need to change as well. One thought that did
> > > > come to mind was exporting both VMCI_Device_Get and vmci_device_get
> > > > but that would likely just confuse people. So in short I have made
> > > > function names lower case where possible, but exported symbols could
> > > > not be changed.
> > >
> > > Not true at all. You want those drivers to be merged as well, right?
> > > So they will need to have their functions changed, and their code as
> > > well.
> > >
> > > Just wait until we get to the "change your functionality around"
> > > requests, those will require those drivers to change. Right now we are
> > > at the "silly and obvious things you did wrong" stage of the review
> > > process :)
> > >
> > > So please fix these, and also, post these drivers as well, so we can see
> > > how they interact with the core code.
> > >
> > > Actually, if you are going to need lots of refactoring for these
> > > drivers, and the core, I would recommend putting this all in the staging
> > > tree, to allow that to happen over time. That would ensure that your
> > > users keep having working systems, and let you modify the interfaces
> > > better and easier, than having to keep it all out-of-tree.
> > >
> > > What do you think?
> >
> > Actually I think that we'd prefer to keep this in a patch-based form, at
> > least for now, because majority of our users get these drivers with
> > VMware Tools and will continue doing so until ditsributions start
> > enabling VMCI in their kernels. Which they probably won't until VMCI
> > moves form staging. We'd also have to constantly adjust drivers that we
> > are not working on getting upstream at this time to work with the
> > rapidly changing version of VMCI in staging, which will just add work
> > for us.
>
> That wouldn't be an issue if you just include all of the drivers in the
> tree at the same time, right?
Maybe it wouldn't, however at this time we have not scheduled any
resources for upstreaming vmhgfs driver. We however do seek feedback on
vmci driver (and later vsock) for which we did schedule resources.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Michael S. Tsirkin @ 2012-07-29 9:44 UTC (permalink / raw)
To: Sasha Levin
Cc: krkumar2, habanero, kvm, netdev, mashirle, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <500A9A72.20507@gmail.com>
On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote:
> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
> >> - err = init_vqs(vi);
> >> > + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
> >> > + vi->has_cvq = true;
> >> > +
> > How about we disable multiqueue if there's no cvq?
> > Will make logic a bit simpler, won't it?
>
> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
Well !cvq support is a legacy feature: the reason we support it
in driver is to avoid breaking on old hosts. Adding more code to that
path just doesn't make much sense since old hosts won't have mq.
--
MST
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Michael S. Tsirkin @ 2012-07-29 9:50 UTC (permalink / raw)
To: Jason Wang
Cc: krkumar2, habanero, mashirle, kvm, netdev, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <500CE5B3.3040404@redhat.com>
On Mon, Jul 23, 2012 at 01:48:35PM +0800, Jason Wang wrote:
> >>+ }
> >>
> >>- if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
> >>+ ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
> >>+ (const char **)names);
> >>+ if (ret)
> >>+ goto err;
> >>+
> >>+ if (vi->has_cvq)
> >> vi->cvq = vqs[2];
> >>
> >>- if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
> >>- vi->dev->features |= NETIF_F_HW_VLAN_FILTER;
> >>+ for (i = 0; i< vi->num_queue_pairs * 2; i += 2) {
> >>+ int j = i == 0 ? i : i + vi->has_cvq;
> >>+ vi->rq[i / 2]->vq = vqs[j];
> >>+ vi->sq[i / 2]->vq = vqs[j + 1];
> >Same here.
>
> Consider the code is really simple, seem no need to use helpers.
Well it was not simple to at least one reader :)
The problem is not this logic is complex,
it is that it is spread all over the code.
If we had e.g. vnet_tx_vqn_to_queuenum vnet_tx_queuenum_to_vqn
and same for rx, then the logic would all be
in one place, and have a tidy comment on top explaining
the VQ numbering scheme.
--
MST
^ permalink raw reply
* Re: [PATCH V4 3/3] virtio-blk: Add bio-based IO path for virtio-blk
From: Michael S. Tsirkin @ 2012-07-29 11:11 UTC (permalink / raw)
To: Asias He; +Cc: kvm, linux-kernel, virtualization, Christoph Hellwig
In-Reply-To: <1343442065-15646-4-git-send-email-asias@redhat.com>
On Sat, Jul 28, 2012 at 10:21:05AM +0800, Asias He wrote:
> This patch introduces bio-based IO path for virtio-blk.
>
> Compared to request-based IO path, bio-based IO path uses driver
> provided ->make_request_fn() method to bypasses the IO scheduler. It
> handles the bio to device directly without allocating a request in block
> layer. This reduces the IO path in guest kernel to achieve high IOPS
> and lower latency. The downside is that guest can not use the IO
> scheduler to merge and sort requests. However, this is not a big problem
> if the backend disk in host side uses faster disk device.
If this optimization depends on the host, then it
should be reported to the guest using a feature bit,
as opposed to being guest driven.
> When the bio-based IO path is not enabled, virtio-blk still uses the
> original request-based IO path, no performance difference is observed.
>
> Performance evaluation:
> -----------------------------
> 1) Fio test is performed in a 8 vcpu guest with ramdisk based guest using
> kvm tool.
>
> Short version:
> With bio-based IO path, sequential read/write, random read/write
> IOPS boost : 28%, 24%, 21%, 16%
> Latency improvement: 32%, 17%, 21%, 16%
>
> Long version:
> With bio-based IO path:
> seq-read : io=2048.0MB, bw=116996KB/s, iops=233991 , runt= 17925msec
> seq-write : io=2048.0MB, bw=100829KB/s, iops=201658 , runt= 20799msec
> rand-read : io=3095.7MB, bw=112134KB/s, iops=224268 , runt= 28269msec
> rand-write: io=3095.7MB, bw=96198KB/s, iops=192396 , runt= 32952msec
> clat (usec): min=0 , max=2631.6K, avg=58716.99, stdev=191377.30
> clat (usec): min=0 , max=1753.2K, avg=66423.25, stdev=81774.35
> clat (usec): min=0 , max=2915.5K, avg=61685.70, stdev=120598.39
> clat (usec): min=0 , max=1933.4K, avg=76935.12, stdev=96603.45
> cpu : usr=74.08%, sys=703.84%, ctx=29661403, majf=21354, minf=22460954
> cpu : usr=70.92%, sys=702.81%, ctx=77219828, majf=13980, minf=27713137
> cpu : usr=72.23%, sys=695.37%, ctx=88081059, majf=18475, minf=28177648
> cpu : usr=69.69%, sys=654.13%, ctx=145476035, majf=15867, minf=26176375
> With request-based IO path:
> seq-read : io=2048.0MB, bw=91074KB/s, iops=182147 , runt= 23027msec
> seq-write : io=2048.0MB, bw=80725KB/s, iops=161449 , runt= 25979msec
> rand-read : io=3095.7MB, bw=92106KB/s, iops=184211 , runt= 34416msec
> rand-write: io=3095.7MB, bw=82815KB/s, iops=165630 , runt= 38277msec
> clat (usec): min=0 , max=1932.4K, avg=77824.17, stdev=170339.49
> clat (usec): min=0 , max=2510.2K, avg=78023.96, stdev=146949.15
> clat (usec): min=0 , max=3037.2K, avg=74746.53, stdev=128498.27
> clat (usec): min=0 , max=1363.4K, avg=89830.75, stdev=114279.68
> cpu : usr=53.28%, sys=724.19%, ctx=37988895, majf=17531, minf=23577622
> cpu : usr=49.03%, sys=633.20%, ctx=205935380, majf=18197, minf=27288959
> cpu : usr=55.78%, sys=722.40%, ctx=101525058, majf=19273, minf=28067082
> cpu : usr=56.55%, sys=690.83%, ctx=228205022, majf=18039, minf=26551985
So bio based causes cpu to jump up by some 30%?
What happens if you divide IOPS/CPU?
Any improvement that comes from increasing the cpu share
of the given guest on the host will not scale well on
a typical overcommitted host.
> 2) Fio test is performed in a 8 vcpu guest with Fusion-IO based guest using
> kvm tool.
>
> Short version:
> With bio-based IO path, sequential read/write, random read/write
> IOPS boost : 11%, 11%, 13%, 10%
> Latency improvement: 10%, 10%, 12%, 10%
> Long Version:
> With bio-based IO path:
> read : io=2048.0MB, bw=58920KB/s, iops=117840 , runt= 35593msec
> write: io=2048.0MB, bw=64308KB/s, iops=128616 , runt= 32611msec
> read : io=3095.7MB, bw=59633KB/s, iops=119266 , runt= 53157msec
> write: io=3095.7MB, bw=62993KB/s, iops=125985 , runt= 50322msec
> clat (usec): min=0 , max=1284.3K, avg=128109.01, stdev=71513.29
> clat (usec): min=94 , max=962339 , avg=116832.95, stdev=65836.80
> clat (usec): min=0 , max=1846.6K, avg=128509.99, stdev=89575.07
> clat (usec): min=0 , max=2256.4K, avg=121361.84, stdev=82747.25
> cpu : usr=56.79%, sys=421.70%, ctx=147335118, majf=21080, minf=19852517
> cpu : usr=61.81%, sys=455.53%, ctx=143269950, majf=16027, minf=24800604
> cpu : usr=63.10%, sys=455.38%, ctx=178373538, majf=16958, minf=24822612
> cpu : usr=62.04%, sys=453.58%, ctx=226902362, majf=16089, minf=23278105
> With request-based IO path:
> read : io=2048.0MB, bw=52896KB/s, iops=105791 , runt= 39647msec
> write: io=2048.0MB, bw=57856KB/s, iops=115711 , runt= 36248msec
> read : io=3095.7MB, bw=52387KB/s, iops=104773 , runt= 60510msec
> write: io=3095.7MB, bw=57310KB/s, iops=114619 , runt= 55312msec
> clat (usec): min=0 , max=1532.6K, avg=142085.62, stdev=109196.84
> clat (usec): min=0 , max=1487.4K, avg=129110.71, stdev=114973.64
> clat (usec): min=0 , max=1388.6K, avg=145049.22, stdev=107232.55
> clat (usec): min=0 , max=1465.9K, avg=133585.67, stdev=110322.95
> cpu : usr=44.08%, sys=590.71%, ctx=451812322, majf=14841, minf=17648641
> cpu : usr=48.73%, sys=610.78%, ctx=418953997, majf=22164, minf=26850689
> cpu : usr=45.58%, sys=581.16%, ctx=714079216, majf=21497, minf=22558223
> cpu : usr=48.40%, sys=599.65%, ctx=656089423, majf=16393, minf=23824409
Is this host or guest cpu? We should probably measure host cpu
as this includes device overhead which could vary by load.
> How to use:
> -----------------------------
> Add 'virtio_blk.use_bio=1' to kernel cmdline or 'modprobe virtio_blk
> use_bio=1' to enable ->make_request_fn() based I/O path.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
> Signed-off-by: Asias He <asias@redhat.com>
> ---
> drivers/block/virtio_blk.c | 203 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 163 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index c0bbeb4..95cfeed 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -14,6 +14,9 @@
>
> #define PART_BITS 4
>
> +static bool use_bio;
> +module_param(use_bio, bool, S_IRUGO);
> +
> static int major;
> static DEFINE_IDA(vd_index_ida);
>
> @@ -23,6 +26,7 @@ struct virtio_blk
> {
> struct virtio_device *vdev;
> struct virtqueue *vq;
> + wait_queue_head_t queue_wait;
>
> /* The disk structure for the kernel. */
> struct gendisk *disk;
> @@ -51,53 +55,87 @@ struct virtio_blk
> struct virtblk_req
> {
> struct request *req;
> + struct bio *bio;
> struct virtio_blk_outhdr out_hdr;
> struct virtio_scsi_inhdr in_hdr;
> u8 status;
> + struct scatterlist sg[];
> };
>
> -static void blk_done(struct virtqueue *vq)
> +static inline int virtblk_result(struct virtblk_req *vbr)
> +{
> + switch (vbr->status) {
> + case VIRTIO_BLK_S_OK:
> + return 0;
> + case VIRTIO_BLK_S_UNSUPP:
> + return -ENOTTY;
> + default:
> + return -EIO;
> + }
> +}
> +
> +static inline void virtblk_request_done(struct virtio_blk *vblk,
> + struct virtblk_req *vbr)
> +{
> + struct request *req = vbr->req;
> + int error = virtblk_result(vbr);
> +
> + if (req->cmd_type == REQ_TYPE_BLOCK_PC) {
> + req->resid_len = vbr->in_hdr.residual;
> + req->sense_len = vbr->in_hdr.sense_len;
> + req->errors = vbr->in_hdr.errors;
> + } else if (req->cmd_type == REQ_TYPE_SPECIAL) {
> + req->errors = (error != 0);
> + }
> +
> + __blk_end_request_all(req, error);
> + mempool_free(vbr, vblk->pool);
> +}
> +
> +static inline void virtblk_bio_done(struct virtio_blk *vblk,
> + struct virtblk_req *vbr)
> +{
> + bio_endio(vbr->bio, virtblk_result(vbr));
> + mempool_free(vbr, vblk->pool);
> +}
> +
> +static void virtblk_done(struct virtqueue *vq)
> {
> struct virtio_blk *vblk = vq->vdev->priv;
> + unsigned long bio_done = 0, req_done = 0;
> struct virtblk_req *vbr;
> - unsigned int len;
> unsigned long flags;
> + unsigned int len;
>
> spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
> while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
> - int error;
> -
> - switch (vbr->status) {
> - case VIRTIO_BLK_S_OK:
> - error = 0;
> - break;
> - case VIRTIO_BLK_S_UNSUPP:
> - error = -ENOTTY;
> - break;
> - default:
> - error = -EIO;
> - break;
> - }
> -
> - switch (vbr->req->cmd_type) {
> - case REQ_TYPE_BLOCK_PC:
> - vbr->req->resid_len = vbr->in_hdr.residual;
> - vbr->req->sense_len = vbr->in_hdr.sense_len;
> - vbr->req->errors = vbr->in_hdr.errors;
> - break;
> - case REQ_TYPE_SPECIAL:
> - vbr->req->errors = (error != 0);
> - break;
> - default:
> - break;
> + if (vbr->bio) {
> + virtblk_bio_done(vblk, vbr);
> + bio_done++;
> + } else {
> + virtblk_request_done(vblk, vbr);
> + req_done++;
> }
> -
> - __blk_end_request_all(vbr->req, error);
> - mempool_free(vbr, vblk->pool);
> }
> /* In case queue is stopped waiting for more buffers. */
> - blk_start_queue(vblk->disk->queue);
> + if (req_done)
> + blk_start_queue(vblk->disk->queue);
> spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
> +
> + if (bio_done)
> + wake_up(&vblk->queue_wait);
> +}
> +
> +static inline struct virtblk_req *virtblk_alloc_req(struct virtio_blk *vblk,
> + gfp_t gfp_mask)
> +{
> + struct virtblk_req *vbr;
> +
> + vbr = mempool_alloc(vblk->pool, gfp_mask);
> + if (vbr && use_bio)
> + sg_init_table(vbr->sg, vblk->sg_elems);
> +
> + return vbr;
> }
>
> static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> @@ -106,13 +144,13 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> unsigned long num, out = 0, in = 0;
> struct virtblk_req *vbr;
>
> - vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
> + vbr = virtblk_alloc_req(vblk, GFP_ATOMIC);
> if (!vbr)
> /* When another request finishes we'll try again. */
> return false;
>
> vbr->req = req;
> -
> + vbr->bio = NULL;
> if (req->cmd_flags & REQ_FLUSH) {
> vbr->out_hdr.type = VIRTIO_BLK_T_FLUSH;
> vbr->out_hdr.sector = 0;
> @@ -172,7 +210,8 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> }
> }
>
> - if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr, GFP_ATOMIC)<0) {
> + if (virtqueue_add_buf(vblk->vq, vblk->sg, out, in, vbr,
> + GFP_ATOMIC) < 0) {
> mempool_free(vbr, vblk->pool);
> return false;
> }
> @@ -180,7 +219,7 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> return true;
> }
>
> -static void do_virtblk_request(struct request_queue *q)
> +static void virtblk_request(struct request_queue *q)
> {
> struct virtio_blk *vblk = q->queuedata;
> struct request *req;
> @@ -203,6 +242,82 @@ static void do_virtblk_request(struct request_queue *q)
> virtqueue_kick(vblk->vq);
> }
>
> +static void virtblk_add_buf_wait(struct virtio_blk *vblk,
> + struct virtblk_req *vbr,
> + unsigned long out,
> + unsigned long in)
> +{
> + DEFINE_WAIT(wait);
> +
> + for (;;) {
> + prepare_to_wait_exclusive(&vblk->queue_wait, &wait,
> + TASK_UNINTERRUPTIBLE);
> +
> + spin_lock_irq(vblk->disk->queue->queue_lock);
> + if (virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
> + GFP_ATOMIC) < 0) {
> + spin_unlock_irq(vblk->disk->queue->queue_lock);
> + io_schedule();
> + } else {
> + virtqueue_kick(vblk->vq);
> + spin_unlock_irq(vblk->disk->queue->queue_lock);
> + break;
> + }
> +
> + }
> +
> + finish_wait(&vblk->queue_wait, &wait);
> +}
> +
> +static void virtblk_make_request(struct request_queue *q, struct bio *bio)
> +{
> + struct virtio_blk *vblk = q->queuedata;
> + unsigned int num, out = 0, in = 0;
> + struct virtblk_req *vbr;
> +
> + BUG_ON(bio->bi_phys_segments + 2 > vblk->sg_elems);
> + BUG_ON(bio->bi_rw & (REQ_FLUSH | REQ_FUA));
> +
> + vbr = virtblk_alloc_req(vblk, GFP_NOIO);
> + if (!vbr) {
> + bio_endio(bio, -ENOMEM);
> + return;
> + }
> +
> + vbr->bio = bio;
> + vbr->req = NULL;
> + vbr->out_hdr.type = 0;
> + vbr->out_hdr.sector = bio->bi_sector;
> + vbr->out_hdr.ioprio = bio_prio(bio);
> +
> + sg_set_buf(&vbr->sg[out++], &vbr->out_hdr, sizeof(vbr->out_hdr));
> +
> + num = blk_bio_map_sg(q, bio, vbr->sg + out);
> +
> + sg_set_buf(&vbr->sg[num + out + in++], &vbr->status,
> + sizeof(vbr->status));
> +
> + if (num) {
> + if (bio->bi_rw & REQ_WRITE) {
> + vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
> + out += num;
> + } else {
> + vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
> + in += num;
> + }
> + }
> +
> + spin_lock_irq(vblk->disk->queue->queue_lock);
> + if (unlikely(virtqueue_add_buf(vblk->vq, vbr->sg, out, in, vbr,
> + GFP_ATOMIC) < 0)) {
> + spin_unlock_irq(vblk->disk->queue->queue_lock);
> + virtblk_add_buf_wait(vblk, vbr, out, in);
> + return;
> + }
> + virtqueue_kick(vblk->vq);
> + spin_unlock_irq(vblk->disk->queue->queue_lock);
> +}
> +
> /* return id (s/n) string for *disk to *id_str
> */
> static int virtblk_get_id(struct gendisk *disk, char *id_str)
> @@ -360,7 +475,7 @@ static int init_vq(struct virtio_blk *vblk)
> int err = 0;
>
> /* We expect one virtqueue, for output. */
> - vblk->vq = virtio_find_single_vq(vblk->vdev, blk_done, "requests");
> + vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
> if (IS_ERR(vblk->vq))
> err = PTR_ERR(vblk->vq);
>
> @@ -414,7 +529,7 @@ static void virtblk_update_cache_mode(struct virtio_device *vdev)
> u8 writeback = virtblk_get_cache_mode(vdev);
> struct virtio_blk *vblk = vdev->priv;
>
> - if (writeback)
> + if (writeback && !use_bio)
> blk_queue_flush(vblk->disk->queue, REQ_FLUSH);
> else
> blk_queue_flush(vblk->disk->queue, 0);
> @@ -477,6 +592,8 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> struct virtio_blk *vblk;
> struct request_queue *q;
> int err, index;
> + int pool_size;
> +
> u64 cap;
> u32 v, blk_size, sg_elems, opt_io_size;
> u16 min_io_size;
> @@ -506,10 +623,12 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> goto out_free_index;
> }
>
> + init_waitqueue_head(&vblk->queue_wait);
> vblk->vdev = vdev;
> vblk->sg_elems = sg_elems;
> sg_init_table(vblk->sg, vblk->sg_elems);
> mutex_init(&vblk->config_lock);
> +
> INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
> vblk->config_enable = true;
>
> @@ -517,7 +636,10 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vblk;
>
> - vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
> + pool_size = sizeof(struct virtblk_req);
> + if (use_bio)
> + pool_size += sizeof(struct scatterlist) * sg_elems;
> + vblk->pool = mempool_create_kmalloc_pool(1, pool_size);
> if (!vblk->pool) {
> err = -ENOMEM;
> goto out_free_vq;
> @@ -530,12 +652,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> goto out_mempool;
> }
>
> - q = vblk->disk->queue = blk_init_queue(do_virtblk_request, NULL);
> + q = vblk->disk->queue = blk_init_queue(virtblk_request, NULL);
> if (!q) {
> err = -ENOMEM;
> goto out_put_disk;
> }
>
> + if (use_bio)
> + blk_queue_make_request(q, virtblk_make_request);
> q->queuedata = vblk;
>
> virtblk_name_format("vd", index, vblk->disk->disk_name, DISK_NAME_LEN);
> @@ -620,7 +744,6 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> if (!err && opt_io_size)
> blk_queue_io_opt(q, blk_size * opt_io_size);
>
> -
> add_disk(vblk->disk);
> err = device_create_file(disk_to_dev(vblk->disk), &dev_attr_serial);
> if (err)
> --
> 1.7.10.4
^ permalink raw reply
* Re: [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION
From: Michael S. Tsirkin @ 2012-07-29 12:27 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, linux-scsi, kvm-devel, Greg Kroah-Hartman, qemu-devel,
lf-virt, Anthony Liguori, target-devel, Avi Kivity, Paolo Bonzini,
Zhi Yong Wu, Christoph Hellwig, Stefan Hajnoczi
In-Reply-To: <1343250648.1813.169.camel@haakon2.linux-iscsi.org>
On Wed, Jul 25, 2012 at 02:10:48PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2012-07-25 at 16:10 +0300, Avi Kivity wrote:
> > On 07/24/2012 11:45 PM, Nicholas A. Bellinger wrote:
> >
> > >> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > >> > index e942df9..3d5378f 100644
> > >> > --- a/drivers/vhost/tcm_vhost.h
> > >> > +++ b/drivers/vhost/tcm_vhost.h
> > >> > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> > >> >
> > >> > #include <linux/vhost.h>
> > >> >
> > >> > +/*
> > >> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > >> > + *
> > >> > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > >> > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > >> > + */
> >
> >
> > If it's out of tree, why consider it at all? Put a stable ABI in tree
> > and extend it in compatible ways.
> >
> >
>
> This comment was supposed to convey that ABI=0 vhost-scsi userspace code
> is not supported with tcm_vhost mainline code.
>
> But obviously that was not clear enough here. Updating the comment to
> reflect to make this clear.
>
> So the main question here was if it's fine to start with ABI=1, and
> require >= ABI=1 for all vhost-scsi userspace code to function with
> tcm_vhost.
>
> The idea was to avoid confusion for the ABI=0 vhost-scsi code that's
> been floating around for the last 2 years.
>
> --nab
>
>
>
>
>
>
Yes but I am guessing that code does not invoke the ABI ioctl?
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox