* [PATCH 0/5] trace-cmd: Add a recorder readable feature for virtio-trace
From: Yoshihiro YUNOMAE @ 2012-08-22 8:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
Hi Steven,
The following patch set provides a feature which can read trace data of a guest
using virtio-trace (https://lkml.org/lkml/2012/8/9/210) for a recorder
function of trace-cmd. This patch set depends on the trace-agent running on a
guest in the virtio-trace system.
To translate raw data of a guest to text data on a host, information of debugfs
in the guest is also needed on the host. In other words, the guest's debugfs
must be exposed (mounted) on the host via other serial line (we don't like to
depend on network connection). For this purpose, we'll use DIOD 9pfs server
(http://code.google.com/p/diod/) as below.
***HOW TO USE***
We explain about how to translate raw data to text data on a host using
trace-cmd applied this patch set and virtio-trace.
- Preparation
1. Make FIFO in a host
virtio-trace uses virtio-serial pipe as trace data paths as to the number
of CPUs and a control path, so FIFO (named pipe) should be created as follows:
# mkdir /tmp/virtio-trace/
# mkfifo /tmp/virtio-trace/trace-path-cpu{0,1,2,...,X}.{in,out}
# mkfifo /tmp/virtio-trace/agent-ctl-path.{in,out}
Here, if we assign 1VCPU for a guest, then we set as follows:
trace-path-cpu0.{in.out}
and
agent-ctl-path.{in,out}.
2. Set up of virtio-serial pipe and unix in a host
Add qemu option to use virtio-serial pipe for tracing and unix for debugfs.
##virtio-serial device##
-device virtio-serial-pci,id=virtio-serial0\
##control path##
-chardev pipe,id=charchannel0,path=/tmp/virtio-trace/agent-ctl-path\
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
id=channel0,name=agent-ctl-path\
##data path##
-chardev pipe,id=charchannel1,path=/tmp/virtio-trace/trace-path-cpu0\
-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\
id=channel1,name=trace-path-cpu0\
##9pfs path##
-device virtio-serial \
-chardev socket,path=/tmp/virtio-trace/trace-9pfs,server,nowait, \
id=trace-9pfs \
-device virtserialport,chardev=trace-9pfs,name=virtioserial
If you manage guests with libvirt, add the following tags to domain XML files.
Then, libvirt passes the same command option to qemu.
<channel type='pipe'>
<source path='/tmp/virtio-trace/agent-ctl-path'/>
<target type='virtio' name='agent-ctl-path'/>
<address type='virtio-serial' controller='0' bus='0' port='0'/>
</channel>
<channel type='pipe'>
<source path='/tmp/virtio-trace/trace-path-cpu0'/>
<target type='virtio' name='trace-path-cpu0'/>
<address type='virtio-serial' controller='0' bus='0' port='1'/>
</channel>
<channel type='unix'>
<source mode='bind' path='/tmp/virtio-trace/trace-9pfs'/>
<target type='virtio' name='trace-9pfs'/>
<address type='virtio-serial' controller='0' bus='0' port='2'/>
</channel>
Here, chardev names are restricted to trace-path-cpu0 and agent-ctl-path. UNIX
domain socket is automatically created on a host.
3. Boot the guest
You can find some chardev in /dev/virtio-ports/ in the guest.
4. Create symbolic link for trace-cmd on the host
# ln -s /tmp/virtio-trace/trace-path-cpu0.out \
/tmp/virtio-tracing/tracing/per_cpu/cpu0/trace_pipe_raw
5. Wait for 9pfs server on the host
# mount -t 9p -o trans=unix,access=any,uname=root, \
aname=/sys/kernel/debug,version=9p2000.L \
/tmp/virtio-trace/trace-9pfs /tmp/virtio-trace/debugfs
6. Run DIOD on the guest
# diod -E -Nn -u 0
7. Connect DIOD to virtio-console on the guest
# socat TCP4:127.0.0.1:564 /dev/virtio-ports/trace-9pfs
- Execution
1. Run trace-agent on the guest
# ./trace-agent
2. Execute trace-cmd on the host
# AGENT_READ_DIR=/tmp/virtio-trace/tracing \
AGENT_CTL=/tmp/virtio-trace/agent-ctl-path.in \
TRACE_DIR=/tmp/virtio-trace/debugfs/tracing \
./trace-cmd record -e "sched:*
3. Translate raw data to text data on the host
# ./trace-cmd report trace.dat
***Just enhancement ideas***
- Support for trace-cmd => done
- Support for 9pfs protocol
- Support for non-blocking mode in QEMU
Thank you,
---
Masami Hiramatsu (2):
trace-cmd: Use tracing directory to count CPUs
trace-cmd: Use TRACE_DIR envrionment variable if defined
Yoshihiro YUNOMAE (3):
trace-cmd: Use polling function
trace-cmd: Add non-blocking option for open() and splice_read()
trace-cmd: Support trace-agent of virtio-trace
trace-cmd.h | 1
trace-record.c | 41 ++++++++++++++++++++
trace-recorder.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++------
trace-util.c | 27 +++++++++++++
4 files changed, 169 insertions(+), 12 deletions(-)
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Stefan Hajnoczi @ 2012-08-22 8:25 UTC (permalink / raw)
To: Blue Swirl
Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization, Cong Meng,
Paolo Bonzini
In-Reply-To: <CAAu8pHvN+mSoqDuug0iODipDMQ7rMPy6FEFbQqhsR5ZS9RQFEQ@mail.gmail.com>
On Tue, Aug 21, 2012 at 7:31 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Aug 21, 2012 at 8:23 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
>> diff --git a/block_int.h b/block_int.h
>> index d72317f..a9d07a2 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -333,6 +333,10 @@ struct BlockDriverState {
>>
>> /* long-running background operation */
>> BlockJob *job;
>> +
>> + unsigned int max_sectors;
>
> With 32 bit ints and even with 4k sector size, the maximum disk size
> would be 16TB which may be soon exceeded by disks on the market.
> Please use 64 bit values, probably also for segment values below.
This doesn't specify device size, it specifies max sectors per I/O
request. When reviewing, I checked that the Linux kernel also uses
unsigned int for these fields.
Stefan
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-22 1:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120822000741.GI9027@redhat.com>
On Wed, Aug 22, 2012 at 03:07:41AM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 21, 2012 at 05:45:56PM -0300, Rafael Aquini wrote:
> > On Tue, Aug 21, 2012 at 10:30:31PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Aug 21, 2012 at 04:23:58PM -0300, Rafael Aquini wrote:
> > > > On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:
> > > > > >
> > > > > > I believe rcu_dereference_protected() is what I want/need here, since this code
> > > > > > is always called for pages which we hold locked (PG_locked bit).
> > > > >
> > > > > It would only help if we locked the page while updating the mapping,
> > > > > as far as I can see we don't.
> > > > >
> > > >
> > > > But we can do it. In fact, by doing it (locking the page) we can easily avoid
> > > > the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.
> > >
> > > Absolutely. Further, we should look hard at whether most RCU uses
> > > in this patchset can be replaced with page lock.
> > >
> >
> > Yeah, In fact, by testing/grabbing the page lock at leak_balloon() even the
> > module unload X migration / putback race seems to fade away, since migration
> > code holds the page locked all the way.
> > And that seems a quite easy task to be accomplished:
> >
> > ....
> > @@ -169,21 +197,61 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
> > num)
> > /* We can only do one array worth at a time. */
> > num = min(num, ARRAY_SIZE(vb->pfns));
> >
> > + mutex_lock(&vb->balloon_lock);
> > for (vb->num_pfns = 0; vb->num_pfns < num;
> > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > + spin_lock(&vb->pages_lock);
> > + /*
> > + * 'virtballoon_isolatepage()' can drain vb->pages list
> > + * making us to stumble across a _temporarily_ empty list.
>
> This still worries me. If this happens we do not
> lock the page so module can go away?
> if not need to document why.
>
The module won't unload unless it leaks all its pages. If we hit that test that
worries you, leak_balloon() will get back to its caller -- remove_common(), and
it will kept looping at:
/* There might be pages left in the balloon: free them. */
while (vb->num_pages)
leak_balloon(vb, vb->num_pages);
This is true because we do not mess with vb->num_pages while isolating/migrating
balloon pages, so the module will only unload when all isolated pages get back
to vb->pages_list and leak_balloon() reap them appropriatelly. As we will be
doing isolation/migration/putback steps under 'page lock' that race is gone.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-22 0:37 UTC (permalink / raw)
To: Rusty Russell
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, Minchan Kim,
linux-kernel, virtualization, linux-mm, Andi Kleen, Andrew Morton
In-Reply-To: <87mx1o3j5y.fsf@rustcorp.com.au>
On Tue, Aug 21, 2012 at 03:01:37PM +0930, Rusty Russell wrote:
> On Wed, 15 Aug 2012 14:28:51 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Aug 15, 2012 at 12:16:51PM +0100, Mel Gorman wrote:
> > > I was thinking of exactly that page->mapping == balloon_mapping check. As I
> > > do not know how many active balloon drivers there might be I cannot guess
> > > in advance how much of a scalability problem it will be.
> >
> > Not at all sure multiple drivers are worth supporting, but multiple
> > *devices* is I think worth supporting, if for no other reason than that
> > they can work today. For that, we need a device pointer which Rafael
> > wants to put into the mapping, this means multiple balloon mappings.
>
> Rafael, please make sure that the balloon driver fails on the second and
> subsequent balloon devices.
>
> Michael, we only allow multiple balloon devices because it fell out of
> the implementation. If it causes us even the slightest issue, we should
> not support it. It's not a sensible setup.
>
> Cheers,
> Rusty.
Looks like latest revision does it using a flag which seems cleaner,
so I think the point is moot.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-22 0:07 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120821204556.GF12294@t510.redhat.com>
On Tue, Aug 21, 2012 at 05:45:56PM -0300, Rafael Aquini wrote:
> On Tue, Aug 21, 2012 at 10:30:31PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 21, 2012 at 04:23:58PM -0300, Rafael Aquini wrote:
> > > On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:
> > > > >
> > > > > I believe rcu_dereference_protected() is what I want/need here, since this code
> > > > > is always called for pages which we hold locked (PG_locked bit).
> > > >
> > > > It would only help if we locked the page while updating the mapping,
> > > > as far as I can see we don't.
> > > >
> > >
> > > But we can do it. In fact, by doing it (locking the page) we can easily avoid
> > > the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.
> >
> > Absolutely. Further, we should look hard at whether most RCU uses
> > in this patchset can be replaced with page lock.
> >
>
> Yeah, In fact, by testing/grabbing the page lock at leak_balloon() even the
> module unload X migration / putback race seems to fade away, since migration
> code holds the page locked all the way.
> And that seems a quite easy task to be accomplished:
>
> ....
> @@ -169,21 +197,61 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
> num)
> /* We can only do one array worth at a time. */
> num = min(num, ARRAY_SIZE(vb->pfns));
>
> + mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> + spin_lock(&vb->pages_lock);
> + /*
> + * 'virtballoon_isolatepage()' can drain vb->pages list
> + * making us to stumble across a _temporarily_ empty list.
This still worries me. If this happens we do not
lock the page so module can go away?
if not need to document why.
> + *
> + * Release the spinlock and resume from here in order to
> + * give page migration a shot to refill vb->pages list.
> + */
> + if (unlikely(list_empty(&vb->pages))) {
> + spin_unlock(&vb->pages_lock);
> + break;
> + }
> +
> page = list_first_entry(&vb->pages, struct page, lru);
> +
> + /*
> + * Grab the page lock to avoid racing against threads isolating
> + * pages from vb->pages list (it's done under page lock).
> + *
> + * Failing to grab the page lock here means this page has been
> + * selected for isolation already.
> + */
> + if (!trylock_page(page)) {
> + spin_unlock(&vb->pages_lock);
> + break;
> + }
> +
> + clear_balloon_mapping(page);
> list_del(&page->lru);
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> + unlock_page(page);
> + spin_unlock(&vb->pages_lock);
> }
>
> .....
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-22 0:06 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821193438.GE12294@t510.redhat.com>
On Tue, Aug 21, 2012 at 04:34:39PM -0300, Rafael Aquini wrote:
> On Tue, Aug 21, 2012 at 10:16:12PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 21, 2012 at 02:55:03PM -0300, Rafael Aquini wrote:
> > > On Tue, Aug 21, 2012 at 04:52:23PM +0300, Michael S. Tsirkin wrote:
> > > > > + * address_space_operations utilized methods for ballooned pages:
> > > > > + * .migratepage - used to perform balloon's page migration (as is)
> > > > > + * .launder_page - used to isolate a page from balloon's page list
> > > > > + * .freepage - used to reinsert an isolated page to balloon's page list
> > > > > + */
> > > >
> > > > It would be a good idea to document the assumptions here.
> > > > Looks like .launder_page and .freepage are called in rcu critical
> > > > section.
> > > > But migratepage isn't - why is that safe?
> > > >
> > >
> > > The migratepage callback for virtio_balloon can sleep, and IIUC we cannot sleep
> > > within a RCU critical section.
> > >
> > > Also, The migratepage callback is called at inner migration's circle function
> > > move_to_new_page(), and I don't think embedding it in a RCU critical section
> > > would be a good idea, for the same understanding aforementioned.
> >
> > Yes but this means it is still exposed to the module unloading
> > races that RCU was supposed to fix.
> > So need to either rework that code so it won't sleep
> > or switch to some other synchronization.
> >
> Can you refactor tell_host() to not sleep? Or, can I get rid of calling it at
> virtballoon_migratepage()? If 'no' is the answer for both questions, that's the
> way that code has to remain, even if we find a way around to hack the
> migratepage callback and have it embedded into a RCU crit section.
>
> That's why I believe once the balloon driver is commanded to unload, we must
> flag virtballoon_migratepage to skip it's work. By doing this, the thread
> performing memory compaction will have to recur to the 'putback' path which is
> RCU protected. (IMHO).
>
> As the module will not uload utill it leaks all pages on its list, that unload
> race you pointed before will be covered.
It can not be: nothing callback does can prevent it from
running after module unload: you must have some synchronization
in the calling code.
--
MST
^ permalink raw reply
* [RFC-v3 5/5] virtio-scsi: Set max_target=0 during vhost-scsi operation
From: Nicholas A. Bellinger @ 2012-08-21 20:52 UTC (permalink / raw)
To: target-devel
Cc: Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin, Jan Kiszka,
qemu-devel, Zhi Yong Wu, Anthony Liguori, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1345582331-22593-1-git-send-email-nab@linux-iscsi.org>
From: Nicholas Bellinger <nab@linux-iscsi.org>
This QEMU patch sets VirtIOSCSIConfig->max_target=0 for vhost-scsi operation
to restrict virtio-scsi LLD guest scanning to max_id=0 (a single target ID
instance) when connected to individual tcm_vhost endpoints.
This ensures that virtio-scsi LLD only attempts to scan target IDs up to
VIRTIO_SCSI_MAX_TARGET when connected via virtio-scsi-raw.
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
hw/virtio-scsi.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
index edda097..ab5ca72 100644
--- a/hw/virtio-scsi.c
+++ b/hw/virtio-scsi.c
@@ -546,7 +546,11 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
stl_raw(&scsiconf->sense_size, s->sense_size);
stl_raw(&scsiconf->cdb_size, s->cdb_size);
stl_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
- stl_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+ if (s->vhost_scsi) {
+ stl_raw(&scsiconf->max_target, 0);
+ } else {
+ stl_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
+ }
stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
}
--
1.7.2.5
^ permalink raw reply related
* [RFC-v3 4/5] virtio-scsi: Add start/stop functionality for vhost-scsi
From: Nicholas A. Bellinger @ 2012-08-21 20:52 UTC (permalink / raw)
To: target-devel
Cc: Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin, Jan Kiszka,
qemu-devel, Zhi Yong Wu, Anthony Liguori, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1345582331-22593-1-git-send-email-nab@linux-iscsi.org>
From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
This patch starts and stops vhost as the virtio device transitions
through its status phases. Vhost can only be started once the guest
reports its driver has successfully initialized, which means the
virtqueues have been set up by the guest.
v3: - Add vhost-scsi.h include for DEFINE_PROP_VHOST_SCSI (mst + nab)
- Move vhost-scsi related struct members ahead of *cmd_vqs[0] within
VirtIOSCSI definition. (paolo + nab)
v2: - Squash virtio-scsi: use the vhost-scsi host device from stefan (nab)
- Fix up virtio_scsi_properties[] conflict w/ upstream qemu (nab)
- Drop usage of to_virtio_scsi() in virtio_scsi_set_status()
(reported by paolo)
- Use modern VirtIOSCSIConf define in virtio-scsi.h (reported by paolo)
- Use s->conf->vhost_scsi instead of proxyconf->vhost_scsi in
virtio_scsi_init() (reported by paolo)
- Only register QEMU SCSI bus is vhost-scsi is not active (reported
by paolo)
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
hw/virtio-pci.c | 2 ++
hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/virtio-scsi.h | 1 +
3 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 125eded..8ec7cf1 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -22,6 +22,7 @@
#include "virtio-net.h"
#include "virtio-serial.h"
#include "virtio-scsi.h"
+#include "vhost-scsi.h"
#include "pci.h"
#include "qemu-error.h"
#include "msi.h"
@@ -1036,6 +1037,7 @@ static void virtio_scsi_exit_pci(PCIDevice *pci_dev)
}
static Property virtio_scsi_properties[] = {
+ DEFINE_PROP_VHOST_SCSI("vhost-scsi", VirtIOPCIProxy, scsi.vhost_scsi),
DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
index 5f737ac..edda097 100644
--- a/hw/virtio-scsi.c
+++ b/hw/virtio-scsi.c
@@ -13,9 +13,13 @@
*
*/
+#include "qemu-common.h"
+#include "qemu-error.h"
+#include "vhost-scsi.h"
#include "virtio-scsi.h"
#include <hw/scsi.h>
#include <hw/scsi-defs.h>
+#include "vhost.h"
#define VIRTIO_SCSI_VQ_SIZE 128
#define VIRTIO_SCSI_CDB_SIZE 32
@@ -144,6 +148,10 @@ typedef struct {
uint32_t cdb_size;
int resetting;
bool events_dropped;
+
+ bool vhost_started;
+ VHostSCSI *vhost_scsi;
+
VirtQueue *ctrl_vq;
VirtQueue *event_vq;
VirtQueue *cmd_vqs[0];
@@ -699,6 +707,38 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
.load_request = virtio_scsi_load_request,
};
+static bool virtio_scsi_started(VirtIOSCSI *s, uint8_t val)
+{
+ return (val & VIRTIO_CONFIG_S_DRIVER_OK) && s->vdev.vm_running;
+}
+
+static void virtio_scsi_set_status(VirtIODevice *vdev, uint8_t val)
+{
+ VirtIOSCSI *s = (VirtIOSCSI *)vdev;
+ bool start = virtio_scsi_started(s, val);
+
+ if (s->vhost_started == start) {
+ return;
+ }
+
+ if (start) {
+ int ret;
+
+ ret = vhost_scsi_start(s->vhost_scsi, vdev);
+ if (ret < 0) {
+ error_report("virtio-scsi: unable to start vhost: %s\n",
+ strerror(-ret));
+
+ /* There is no userspace virtio-scsi fallback so exit */
+ exit(1);
+ }
+ } else {
+ vhost_scsi_stop(s->vhost_scsi, vdev);
+ }
+
+ s->vhost_started = start;
+}
+
VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf)
{
VirtIOSCSI *s;
@@ -712,12 +752,17 @@ VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf)
s->qdev = dev;
s->conf = proxyconf;
+ s->vhost_started = false;
+ s->vhost_scsi = s->conf->vhost_scsi;
/* TODO set up vdev function pointers */
s->vdev.get_config = virtio_scsi_get_config;
s->vdev.set_config = virtio_scsi_set_config;
s->vdev.get_features = virtio_scsi_get_features;
s->vdev.reset = virtio_scsi_reset;
+ if (s->vhost_scsi) {
+ s->vdev.set_status = virtio_scsi_set_status;
+ }
s->ctrl_vq = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE,
virtio_scsi_handle_ctrl);
@@ -743,5 +788,9 @@ void virtio_scsi_exit(VirtIODevice *vdev)
{
VirtIOSCSI *s = (VirtIOSCSI *)vdev;
unregister_savevm(s->qdev, "virtio-scsi", s);
+
+ /* This will stop vhost backend if appropriate. */
+ virtio_scsi_set_status(vdev, 0);
+
virtio_cleanup(vdev);
}
diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h
index 4bc889d..74e9422 100644
--- a/hw/virtio-scsi.h
+++ b/hw/virtio-scsi.h
@@ -22,6 +22,7 @@
#define VIRTIO_ID_SCSI 8
struct VirtIOSCSIConf {
+ VHostSCSI *vhost_scsi;
uint32_t num_queues;
uint32_t max_sectors;
uint32_t cmd_per_lun;
--
1.7.2.5
^ permalink raw reply related
* [RFC-v3 3/5] vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
From: Nicholas A. Bellinger @ 2012-08-21 20:52 UTC (permalink / raw)
To: target-devel
Cc: Anthony Liguori, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
Jan Kiszka, qemu-devel, Zhi Yong Wu, Anthony Liguori,
Paolo Bonzini, lf-virt, Christoph Hellwig
In-Reply-To: <1345582331-22593-1-git-send-email-nab@linux-iscsi.org>
From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
This patch adds a new type of host device that drives the vhost_scsi
device. The syntax to add vhost-scsi is:
qemu -vhost-scsi id=vhost-scsi0,wwpn=...,tpgt=123
The virtio-scsi emulated device will make use of vhost-scsi to process
virtio-scsi requests inside the kernel and hand them to the in-kernel
SCSI target stack using the tcm_vhost fabric driver.
The tcm_vhost driver was merged into the upstream linux kernel for 3.6-rc2,
and the commit can be found here:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=057cbf49a1f08297
Changelog v2 -> v3:
- Move qdev_prop_vhost_scsi + DEFINE_PROP_VHOST_SCSI defs into vhost-scsi.[c,h]
(reported by MST)
- Add enum vhost_scsi_vq_list for VHostSCSI->vqs[] enumeration (reported by MST)
- Add missing braces around single like if statement to following QEMU
style (reported by Blue Swirl)
- Change vhost_scsi_target->vhost_wwpn to char *, in order to drop casts to
pstrcpy in vhost_scsi_start() + vhost_scsi_stop() (reported by Blue Swirl)
- Change VHOST_SCSI_GET_ABI_VERSION to 'int' type (MST)
- Fix 4 byte alignment of vhost_scsi_target (MST)
- Convert fprintf(stderr, ...) usage to -> error_report() (reported by MST)
- Do explict memset of backend before calling VHOST_SCSI_CLEAR_ENDPOINT
in vhost_scsi_stop() (reported by MST)
- Add support for vhostfd passing in vhost_scsi_add() (reported by MST)
- Change vhost_scsi_add() to use monitor_handle_fd_param() (reported by MST)
Changelog v1 -> v2:
- Expose ABI version via VHOST_SCSI_GET_ABI_VERSION + use Rev 0 as
starting point for v3.6-rc code (Stefan + ALiguori + nab)
- Fix upstream qemu conflict in hw/qdev-properties.c
- Make GET_ABI_VERSION use int (nab + mst)
- Fix vhost-scsi case lables in configure (reported by paolo)
- Convert qdev_prop_vhost_scsi to use ->get() + ->set() following
qdev_prop_netdev (reported by paolo)
- Fix typo in qemu-options.hx definition of vhost-scsi (reported by paolo)
Changelog v0 -> v1:
- Add VHOST_SCSI_SET_ENDPOINT call (stefan)
- Enable vhost notifiers for multiple queues (Zhi)
- clear vhost-scsi endpoint on stopped (Zhi)
- Add CONFIG_VHOST_SCSI for QEMU build configure (nab)
- Rename vhost_vring_target -> vhost_scsi_target (mst + nab)
- Add support for VHOST_SCSI_GET_ABI_VERSION ioctl (aliguori + nab)
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
configure | 10 +++
hw/Makefile.objs | 1 +
hw/qdev-properties.c | 41 +++++++++++
hw/vhost-scsi.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
hw/vhost-scsi.h | 62 ++++++++++++++++
qemu-common.h | 1 +
qemu-config.c | 19 +++++
qemu-options.hx | 4 +
vl.c | 18 +++++
9 files changed, 346 insertions(+), 0 deletions(-)
create mode 100644 hw/vhost-scsi.c
create mode 100644 hw/vhost-scsi.h
diff --git a/configure b/configure
index f0dbc03..1f03202 100755
--- a/configure
+++ b/configure
@@ -168,6 +168,7 @@ libattr=""
xfs=""
vhost_net="no"
+vhost_scsi="no"
kvm="no"
gprof="no"
debug_tcg="no"
@@ -513,6 +514,7 @@ Haiku)
usb="linux"
kvm="yes"
vhost_net="yes"
+ vhost_scsi="yes"
if [ "$cpu" = "i386" -o "$cpu" = "x86_64" ] ; then
audio_possible_drivers="$audio_possible_drivers fmod"
fi
@@ -818,6 +820,10 @@ for opt do
;;
--enable-vhost-net) vhost_net="yes"
;;
+ --disable-vhost-scsi) vhost_scsi="no"
+ ;;
+ --enable-vhost-scsi) vhost_scsi="yes"
+ ;;
--disable-opengl) opengl="no"
;;
--enable-opengl) opengl="yes"
@@ -3116,6 +3122,7 @@ echo "posix_madvise $posix_madvise"
echo "uuid support $uuid"
echo "libcap-ng support $cap_ng"
echo "vhost-net support $vhost_net"
+echo "vhost-scsi support $vhost_scsi"
echo "Trace backend $trace_backend"
echo "Trace output file $trace_file-<pid>"
echo "spice support $spice"
@@ -3828,6 +3835,9 @@ case "$target_arch2" in
if test "$vhost_net" = "yes" ; then
echo "CONFIG_VHOST_NET=y" >> $config_target_mak
fi
+ if test "$vhost_scsi" = "yes" ; then
+ echo "CONFIG_VHOST_SCSI=y" >> $config_target_mak
+ fi
fi
esac
case "$target_arch2" in
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 3ba5dd0..6ab75ec 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -169,6 +169,7 @@ obj-$(CONFIG_VIRTIO) += virtio.o virtio-blk.o virtio-balloon.o virtio-net.o
obj-$(CONFIG_VIRTIO) += virtio-serial-bus.o virtio-scsi.o
obj-$(CONFIG_SOFTMMU) += vhost_net.o
obj-$(CONFIG_VHOST_NET) += vhost.o
+obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o
obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/
obj-$(CONFIG_NO_PCI) += pci-stub.o
obj-$(CONFIG_VGA) += vga.o
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 8aca0d4..8b505ca 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -4,6 +4,7 @@
#include "blockdev.h"
#include "hw/block-common.h"
#include "net/hub.h"
+#include "vhost-scsi.h"
void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
{
@@ -696,6 +697,46 @@ PropertyInfo qdev_prop_vlan = {
.set = set_vlan,
};
+/* --- vhost-scsi --- */
+
+static int parse_vhost_scsi_dev(DeviceState *dev, const char *str, void **ptr)
+{
+ VHostSCSI *p;
+
+ p = find_vhost_scsi(str);
+ if (p == NULL) {
+ return -ENOENT;
+ }
+
+ *ptr = p;
+ return 0;
+}
+
+static const char *print_vhost_scsi_dev(void *ptr)
+{
+ VHostSCSI *p = ptr;
+
+ return (p) ? vhost_scsi_get_id(p) : "<null>";
+}
+
+static void get_vhost_scsi_dev(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ get_pointer(obj, v, opaque, print_vhost_scsi_dev, name, errp);
+}
+
+static void set_vhost_scsi_dev(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ set_pointer(obj, v, opaque, parse_vhost_scsi_dev, name, errp);
+}
+
+PropertyInfo qdev_prop_vhost_scsi = {
+ .name = "vhost-scsi",
+ .get = get_vhost_scsi_dev,
+ .set = set_vhost_scsi_dev,
+};
+
/* --- pointer --- */
/* Not a proper property, just for dirty hacks. TODO Remove it! */
diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
new file mode 100644
index 0000000..96da2fc
--- /dev/null
+++ b/hw/vhost-scsi.c
@@ -0,0 +1,190 @@
+/*
+ * vhost_scsi host device
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
+ *
+ * Changes for QEMU mainline + tcm_vhost kernel upstream:
+ * Nicholas Bellinger <nab@risingtidesystems.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include <sys/ioctl.h>
+#include "config.h"
+#include "qemu-queue.h"
+#include "monitor.h"
+#include "vhost-scsi.h"
+#include "vhost.h"
+
+struct VHostSCSI {
+ const char *id;
+ const char *wwpn;
+ uint16_t tpgt;
+ int vhostfd;
+ struct vhost_dev dev;
+ struct vhost_virtqueue vqs[VHOST_SCSI_VQ_NUM];
+ QLIST_ENTRY(VHostSCSI) list;
+};
+
+static QLIST_HEAD(, VHostSCSI) vhost_scsi_list =
+ QLIST_HEAD_INITIALIZER(vhost_scsi_list);
+
+VHostSCSI *find_vhost_scsi(const char *id)
+{
+ VHostSCSI *vs;
+
+ QLIST_FOREACH(vs, &vhost_scsi_list, list) {
+ if (!strcmp(id, vs->id)) {
+ return vs;
+ }
+ }
+ return NULL;
+}
+
+const char *vhost_scsi_get_id(VHostSCSI *vs)
+{
+ return vs->id;
+}
+
+int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev)
+{
+ int ret, abi_version;
+ struct vhost_scsi_target backend;
+
+ if (!vhost_dev_query(&vs->dev, vdev)) {
+ return -ENOTSUP;
+ }
+
+ vs->dev.nvqs = VHOST_SCSI_VQ_NUM;
+ vs->dev.vqs = vs->vqs;
+
+ ret = vhost_dev_enable_notifiers(&vs->dev, vdev);
+ if (ret < 0) {
+ return ret;
+ }
+
+ ret = vhost_dev_start(&vs->dev, vdev);
+ if (ret < 0) {
+ return ret;
+ }
+
+ ret = ioctl(vs->dev.control, VHOST_SCSI_GET_ABI_VERSION, &abi_version);
+ if (ret < 0) {
+ ret = -errno;
+ vhost_dev_stop(&vs->dev, vdev);
+ return ret;
+ }
+ if (abi_version > VHOST_SCSI_ABI_VERSION) {
+ error_report("vhost-scsi: The running tcm_vhost kernel abi_version:"
+ " %d is greater than vhost_scsi userspace supports: %d, please"
+ " upgrade your version of QEMU\n", abi_version,
+ VHOST_SCSI_ABI_VERSION);
+ ret = -ENOSYS;
+ vhost_dev_stop(&vs->dev, vdev);
+ return ret;
+ }
+ fprintf(stdout, "TCM_vHost ABI version: %d\n", abi_version);
+
+ memset(&backend, 0, sizeof(backend));
+ pstrcpy(backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
+ backend.vhost_tpgt = vs->tpgt;
+ ret = ioctl(vs->dev.control, VHOST_SCSI_SET_ENDPOINT, &backend);
+ if (ret < 0) {
+ ret = -errno;
+ vhost_dev_stop(&vs->dev, vdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+void vhost_scsi_stop(VHostSCSI *vs, VirtIODevice *vdev)
+{
+ int ret;
+ struct vhost_scsi_target backend;
+
+ memset(&backend, 0, sizeof(backend));
+ pstrcpy(backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
+ backend.vhost_tpgt = vs->tpgt;
+ ret = ioctl(vs->dev.control, VHOST_SCSI_CLEAR_ENDPOINT, &backend);
+ if (ret < 0) {
+ error_report("vhost-scsi: Failed to clear endpoint\n");
+ }
+
+ vhost_dev_stop(&vs->dev, vdev);
+}
+
+static VHostSCSI *vhost_scsi_add(const char *id, const char *wwpn,
+ uint16_t tpgt, const char *vhostfd_str)
+{
+ VHostSCSI *vs;
+ int ret;
+
+ vs = g_malloc0(sizeof(*vs));
+ if (!vs) {
+ error_report("vhost-scsi: unable to allocate *vs\n");
+ return NULL;
+ }
+ vs->vhostfd = -1;
+
+ if (vhostfd_str) {
+ vs->vhostfd = monitor_handle_fd_param(cur_mon, vhostfd_str);
+ if (vs->vhostfd == -1) {
+ error_report("vhost-scsi: unable to parse vs->vhostfd\n");
+ return NULL;
+ }
+ }
+ /* TODO set up vhost-scsi device and bind to tcm_vhost/$wwpm/tpgt_$tpgt */
+ ret = vhost_dev_init(&vs->dev, vs->vhostfd, "/dev/vhost-scsi", false);
+ if (ret < 0) {
+ error_report("vhost-scsi: vhost initialization failed: %s\n",
+ strerror(-ret));
+ return NULL;
+ }
+ vs->dev.backend_features = 0;
+ vs->dev.acked_features = 0;
+
+ vs->id = g_strdup(id);
+ vs->wwpn = g_strdup(wwpn);
+ vs->tpgt = tpgt;
+ QLIST_INSERT_HEAD(&vhost_scsi_list, vs, list);
+
+ return vs;
+}
+
+VHostSCSI *vhost_scsi_add_opts(QemuOpts *opts)
+{
+ const char *id;
+ const char *wwpn, *vhostfd;
+ uint64_t tpgt;
+
+ id = qemu_opts_id(opts);
+ if (!id) {
+ error_report("vhost-scsi: no id specified\n");
+ return NULL;
+ }
+ if (find_vhost_scsi(id)) {
+ error_report("duplicate vhost-scsi: \"%s\"\n", id);
+ return NULL;
+ }
+
+ wwpn = qemu_opt_get(opts, "wwpn");
+ if (!wwpn) {
+ error_report("vhost-scsi: \"%s\" missing wwpn\n", id);
+ return NULL;
+ }
+
+ tpgt = qemu_opt_get_number(opts, "tpgt", UINT64_MAX);
+ if (tpgt > UINT16_MAX) {
+ error_report("vhost-scsi: \"%s\" needs a 16-bit tpgt\n", id);
+ return NULL;
+ }
+ vhostfd = qemu_opt_get(opts, "vhostfd");
+
+ return vhost_scsi_add(id, wwpn, tpgt, vhostfd);
+}
diff --git a/hw/vhost-scsi.h b/hw/vhost-scsi.h
new file mode 100644
index 0000000..68de51a
--- /dev/null
+++ b/hw/vhost-scsi.h
@@ -0,0 +1,62 @@
+/*
+ * vhost_scsi host device
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ * Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef VHOST_SCSI_H
+#define VHOST_SCSI_H
+
+#include "qemu-common.h"
+#include "qemu-option.h"
+#include "qdev.h"
+
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: July 2012 version starting point for v3.6-rc merge candidate +
+ * RFC-v2 vhost-scsi userspace. Add GET_ABI_VERSION ioctl usage
+ */
+
+#define VHOST_SCSI_ABI_VERSION 0
+
+/* TODO #include <linux/vhost.h> properly */
+/* For VHOST_SCSI_SET_ENDPOINT/VHOST_SCSI_CLEAR_ENDPOINT ioctl */
+struct vhost_scsi_target {
+ int abi_version;
+ char vhost_wwpn[224];
+ unsigned short vhost_tpgt;
+ unsigned short reserved;
+};
+
+enum vhost_scsi_vq_list {
+ VHOST_SCSI_VQ_CTL = 0,
+ VHOST_SCSI_VQ_EVT = 1,
+ VHOST_SCSI_VQ_IO = 2,
+ VHOST_SCSI_VQ_NUM = 3,
+};
+
+#define VHOST_VIRTIO 0xAF
+#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
+#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
+#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, int)
+
+extern PropertyInfo qdev_prop_vhost_scsi;
+
+#define DEFINE_PROP_VHOST_SCSI(_n, _s, _f) \
+ DEFINE_PROP(_n, _s, _f, qdev_prop_vhost_scsi, VHostSCSI*)
+
+VHostSCSI *find_vhost_scsi(const char *id);
+const char *vhost_scsi_get_id(VHostSCSI *vs);
+VHostSCSI *vhost_scsi_add_opts(QemuOpts *opts);
+int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev);
+void vhost_scsi_stop(VHostSCSI *vs, VirtIODevice *vdev);
+
+#endif
diff --git a/qemu-common.h b/qemu-common.h
index f9deca6..ec36002 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -280,6 +280,7 @@ typedef struct EventNotifier EventNotifier;
typedef struct VirtIODevice VirtIODevice;
typedef struct QEMUSGList QEMUSGList;
typedef struct SHPCDevice SHPCDevice;
+typedef struct VHostSCSI VHostSCSI;
typedef uint64_t pcibus_t;
diff --git a/qemu-config.c b/qemu-config.c
index 5c3296b..2d4884c 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -626,6 +626,24 @@ QemuOptsList qemu_boot_opts = {
},
};
+QemuOptsList qemu_vhost_scsi_opts = {
+ .name = "vhost-scsi",
+ .head = QTAILQ_HEAD_INITIALIZER(qemu_vhost_scsi_opts.head),
+ .desc = {
+ {
+ .name = "wwpn",
+ .type = QEMU_OPT_STRING,
+ }, {
+ .name = "tpgt",
+ .type = QEMU_OPT_NUMBER,
+ }, {
+ .name = "vhostfd",
+ .type = QEMU_OPT_STRING,
+ },
+ { /* end of list */ }
+ },
+};
+
static QemuOptsList *vm_config_groups[32] = {
&qemu_drive_opts,
&qemu_chardev_opts,
@@ -641,6 +659,7 @@ static QemuOptsList *vm_config_groups[32] = {
&qemu_machine_opts,
&qemu_boot_opts,
&qemu_iscsi_opts,
+ &qemu_vhost_scsi_opts,
NULL,
};
diff --git a/qemu-options.hx b/qemu-options.hx
index 47cb5bd..4e7a03c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -565,6 +565,10 @@ possible drivers and properties, use @code{-device ?} and
ETEXI
DEFHEADING()
+DEF("vhost-scsi", HAS_ARG, QEMU_OPTION_vhost_scsi,
+ "-vhost-scsi wwpn=string0,tpgt=number0\n"
+ " add vhost-scsi device\n",
+ QEMU_ARCH_ALL)
DEFHEADING(File system options:)
diff --git a/vl.c b/vl.c
index 91076f0..61c8284 100644
--- a/vl.c
+++ b/vl.c
@@ -144,6 +144,7 @@ int main(int argc, char **argv)
#include "qemu-options.h"
#include "qmp-commands.h"
#include "main-loop.h"
+#include "hw/vhost-scsi.h"
#ifdef CONFIG_VIRTFS
#include "fsdev/qemu-fsdev.h"
#endif
@@ -1858,6 +1859,14 @@ static int fsdev_init_func(QemuOpts *opts, void *opaque)
}
#endif
+static int vhost_scsi_init_func(QemuOpts *opts, void *opaque)
+{
+ if (!vhost_scsi_add_opts(opts)) {
+ return -1;
+ }
+ return 0;
+}
+
static int mon_init_func(QemuOpts *opts, void *opaque)
{
CharDriverState *chr;
@@ -2617,6 +2626,11 @@ int main(int argc, char **argv, char **envp)
}
break;
#endif
+ case QEMU_OPTION_vhost_scsi:
+ if (!qemu_opts_parse(qemu_find_opts("vhost-scsi"), optarg, 0)) {
+ exit(1);
+ }
+ break;
#ifdef CONFIG_SLIRP
case QEMU_OPTION_tftp:
legacy_tftp_prefix = optarg;
@@ -3337,6 +3351,10 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
#endif
+ if (qemu_opts_foreach(qemu_find_opts("vhost-scsi"),
+ vhost_scsi_init_func, NULL, 1)) {
+ exit(1);
+ }
os_daemonize();
--
1.7.2.5
^ permalink raw reply related
* [RFC-v3 2/5] vhost: Pass device path to vhost_dev_init()
From: Nicholas A. Bellinger @ 2012-08-21 20:52 UTC (permalink / raw)
To: target-devel
Cc: Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin, Jan Kiszka,
qemu-devel, Zhi Yong Wu, Anthony Liguori, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1345582331-22593-1-git-send-email-nab@linux-iscsi.org>
From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
The path to /dev/vhost-net is currently hardcoded in vhost_dev_init().
This needs to be changed so that /dev/vhost-scsi can be used. Pass in
the device path instead of hardcoding it.
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
hw/vhost.c | 5 +++--
hw/vhost.h | 3 ++-
hw/vhost_net.c | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/vhost.c b/hw/vhost.c
index 0fd8da8..d0ce5aa 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -747,14 +747,15 @@ static void vhost_eventfd_del(MemoryListener *listener,
{
}
-int vhost_dev_init(struct vhost_dev *hdev, int devfd, bool force)
+int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
+ bool force)
{
uint64_t features;
int r;
if (devfd >= 0) {
hdev->control = devfd;
} else {
- hdev->control = open("/dev/vhost-net", O_RDWR);
+ hdev->control = open(devpath, O_RDWR);
if (hdev->control < 0) {
return -errno;
}
diff --git a/hw/vhost.h b/hw/vhost.h
index 80e64df..0c47229 100644
--- a/hw/vhost.h
+++ b/hw/vhost.h
@@ -44,7 +44,8 @@ struct vhost_dev {
bool force;
};
-int vhost_dev_init(struct vhost_dev *hdev, int devfd, bool force);
+int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
+ bool force);
void vhost_dev_cleanup(struct vhost_dev *hdev);
bool vhost_dev_query(struct vhost_dev *hdev, VirtIODevice *vdev);
int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
diff --git a/hw/vhost_net.c b/hw/vhost_net.c
index ecaa22d..df2c4a3 100644
--- a/hw/vhost_net.c
+++ b/hw/vhost_net.c
@@ -109,7 +109,7 @@ struct vhost_net *vhost_net_init(NetClientState *backend, int devfd,
(1 << VHOST_NET_F_VIRTIO_NET_HDR);
net->backend = r;
- r = vhost_dev_init(&net->dev, devfd, force);
+ r = vhost_dev_init(&net->dev, devfd, "/dev/vhost-net", force);
if (r < 0) {
goto fail;
}
--
1.7.2.5
^ permalink raw reply related
* [RFC-v3 1/5] monitor: Rename+move net_handle_fd_param -> monitor_handle_fd_param
From: Nicholas A. Bellinger @ 2012-08-21 20:52 UTC (permalink / raw)
To: target-devel
Cc: Anthony Liguori, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
Jan Kiszka, qemu-devel, Zhi Yong Wu, Anthony Liguori,
Paolo Bonzini, lf-virt, Christoph Hellwig
In-Reply-To: <1345582331-22593-1-git-send-email-nab@linux-iscsi.org>
From: Nicholas Bellinger <nab@linux-iscsi.org>
This patch renames+moves the net_handle_fd_param() caller used to
obtain a file descriptor from either qemu_parse_fd() (the normal case)
or from monitor_get_fd() (migration case) into a generically prefixed
monitor_handle_fd_param() to be used by vhost-scsi code.
Also update net/[socket,tap].c consumers to use the new prefix.
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
monitor.c | 18 ++++++++++++++++++
monitor.h | 1 +
net.c | 18 ------------------
net.h | 2 --
net/socket.c | 2 +-
net/tap.c | 4 ++--
6 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/monitor.c b/monitor.c
index 49dccfe..0641efe 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2389,6 +2389,24 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
return -1;
}
+int monitor_handle_fd_param(Monitor *mon, const char *fdname)
+{
+ int fd;
+
+ if (!qemu_isdigit(fdname[0]) && mon) {
+
+ fd = monitor_get_fd(mon, fdname);
+ if (fd == -1) {
+ error_report("No file descriptor named %s found", fdname);
+ return -1;
+ }
+ } else {
+ fd = qemu_parse_fd(fdname);
+ }
+
+ return fd;
+}
+
/* mon_cmds and info_cmds would be sorted at runtime */
static mon_cmd_t mon_cmds[] = {
#include "hmp-commands.h"
diff --git a/monitor.h b/monitor.h
index 5f4de1b..d557e97 100644
--- a/monitor.h
+++ b/monitor.h
@@ -65,6 +65,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
void *opaque);
int monitor_get_fd(Monitor *mon, const char *fdname);
+int monitor_handle_fd_param(Monitor *mon, const char *fdname);
void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
GCC_FMT_ATTR(2, 0);
diff --git a/net.c b/net.c
index 60043dd..e5d25d4 100644
--- a/net.c
+++ b/net.c
@@ -522,24 +522,6 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
return -1;
}
-int net_handle_fd_param(Monitor *mon, const char *param)
-{
- int fd;
-
- if (!qemu_isdigit(param[0]) && mon) {
-
- fd = monitor_get_fd(mon, param);
- if (fd == -1) {
- error_report("No file descriptor named %s found", param);
- return -1;
- }
- } else {
- fd = qemu_parse_fd(param);
- }
-
- return fd;
-}
-
static int net_init_nic(const NetClientOptions *opts, const char *name,
NetClientState *peer)
{
diff --git a/net.h b/net.h
index 2975056..04fda1d 100644
--- a/net.h
+++ b/net.h
@@ -168,8 +168,6 @@ int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
-int net_handle_fd_param(Monitor *mon, const char *param);
-
#define POLYNOMIAL 0x04c11db6
unsigned compute_mcast_idx(const uint8_t *ep);
diff --git a/net/socket.c b/net/socket.c
index c172c24..7c602e4 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -629,7 +629,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
if (sock->has_fd) {
int fd;
- fd = net_handle_fd_param(cur_mon, sock->fd);
+ fd = monitor_handle_fd_param(cur_mon, sock->fd);
if (fd == -1 || !net_socket_fd_init(peer, "socket", name, fd, 1)) {
return -1;
}
diff --git a/net/tap.c b/net/tap.c
index 1971525..a88ae8f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -610,7 +610,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
return -1;
}
- fd = net_handle_fd_param(cur_mon, tap->fd);
+ fd = monitor_handle_fd_param(cur_mon, tap->fd);
if (fd == -1) {
return -1;
}
@@ -686,7 +686,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
int vhostfd;
if (tap->has_vhostfd) {
- vhostfd = net_handle_fd_param(cur_mon, tap->vhostfd);
+ vhostfd = monitor_handle_fd_param(cur_mon, tap->vhostfd);
if (vhostfd == -1) {
return -1;
}
--
1.7.2.5
^ permalink raw reply related
* [RFC-v3 0/5] vhost-scsi: Add support for host virtualized target
From: Nicholas A. Bellinger @ 2012-08-21 20:52 UTC (permalink / raw)
To: target-devel
Cc: Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin, Jan Kiszka,
qemu-devel, Zhi Yong Wu, Anthony Liguori, Paolo Bonzini, lf-virt,
Christoph Hellwig
From: Nicholas Bellinger <nab@linux-iscsi.org>
Hi folks,
This is the third RFC for vhost-scsi patches against mainline QEMU v1.1
to support the upstream tcm_vhost host virtualized target driver now
available in v3.6-rc kernel code. This series is based upon last week's
commit 346fe0c4c0b, and is aiming for a future QEMU v1.3 merge.
The patch series is available directly from:
git://git.kernel.org/pub/scm/virt/kvm/nab/qemu-kvm.git vhost-scsi-merge-v3
This -v3 series contains further review changes based upon feedback from
MST, Paolo, and Blue. It also contains the changes to function against
the changes in target-pending/master -> headed for v3.6-rc3 code.
Changes from v3 -> v2:
- Move qdev_prop_vhost_scsi + DEFINE_PROP_VHOST_SCSI defs into vhost-scsi.[c,h]
(reported by MST)
- Add enum vhost_scsi_vq_list for VHostSCSI->vqs[] enumeration (reported by MST)
- Add missing braces around single like if statement to following QEMU
style (reported by Blue Swirl)
- Change vhost_scsi_target->vhost_wwpn to char *, in order to drop casts to
pstrcpy in vhost_scsi_start() + vhost_scsi_stop() (reported by Blue Swirl)
- Change VHOST_SCSI_GET_ABI_VERSION to 'int' type (MST)
- Add vhost-scsi.h include for DEFINE_PROP_VHOST_SCSI (mst + nab)
- Move vhost-scsi related struct members ahead of *cmd_vqs[0] within
VirtIOSCSI definition. (paolo + nab)
- Fix 4 byte alignment of vhost_scsi_target (MST)
- Convert fprintf(stderr, ...) usage to -> error_report() (reported by MST)
- Do explict memset of backend before calling VHOST_SCSI_CLEAR_ENDPOINT
in vhost_scsi_stop() (reported by MST)
- Add support for vhostfd passing in vhost_scsi_add() (reported by MST)
- Move net_handle_fd_param() -> monitor_handle_fd_param() for generic
usage by net/ + vhost-scsi (reported by MST)
- Change vhost_scsi_add() to use monitor_handle_fd_param() (reported by MST)
Changes from v1 -> v2:
- Expose ABI version via VHOST_SCSI_GET_ABI_VERSION + use Rev 0 as
starting point for v3.6-rc code (Stefan + ALiguori + nab)
- Fix upstream qemu conflict in hw/qdev-properties.c
- Make GET_ABI_VERSION use int (nab + mst)
- Drop unnecessary event-notifier changes (nab)
- Fix vhost-scsi case lables in configure (reported by paolo)
- Convert qdev_prop_vhost_scsi to use ->get() + ->set() following
qdev_prop_netdev (reported by paolo)
- Fix typo in qemu-options.hx definition of vhost-scsi (reported by paolo)
- Squash virtio-scsi: use the vhost-scsi host device from stefan (nab)
- Fix up virtio_scsi_properties[] conflict w/ upstream qemu (nab)
- Drop usage of to_virtio_scsi() in virtio_scsi_set_status()
(reported by paolo)
- Use modern VirtIOSCSIConf define in virtio-scsi.h (reported by paolo)
- Use s->conf->vhost_scsi instead of proxyconf->vhost_scsi in
virtio_scsi_init() (reported by paolo)
- Only register QEMU SCSI bus is vhost-scsi is not active (reported by paolo)
- Fix incorrect VirtIOSCSI->cmd_vqs[0] definition (nab)
Thanks again to everyone who has been reviewing this series!
--nab
Nicholas Bellinger (2):
monitor: Rename+move net_handle_fd_param -> monitor_handle_fd_param
virtio-scsi: Set max_target=0 during vhost-scsi operation
Stefan Hajnoczi (3):
vhost: Pass device path to vhost_dev_init()
vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
virtio-scsi: Add start/stop functionality for vhost-scsi
configure | 10 +++
hw/Makefile.objs | 1 +
hw/qdev-properties.c | 41 +++++++++++
hw/vhost-scsi.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++
hw/vhost-scsi.h | 62 ++++++++++++++++
hw/vhost.c | 5 +-
hw/vhost.h | 3 +-
hw/vhost_net.c | 2 +-
hw/virtio-pci.c | 2 +
hw/virtio-scsi.c | 55 ++++++++++++++-
hw/virtio-scsi.h | 1 +
monitor.c | 18 +++++
monitor.h | 1 +
net.c | 18 -----
net.h | 2 -
net/socket.c | 2 +-
net/tap.c | 4 +-
qemu-common.h | 1 +
qemu-config.c | 19 +++++
qemu-options.hx | 4 +
vl.c | 18 +++++
21 files changed, 431 insertions(+), 28 deletions(-)
create mode 100644 hw/vhost-scsi.c
create mode 100644 hw/vhost-scsi.h
--
1.7.2.5
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 20:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120821193031.GC9027@redhat.com>
On Tue, Aug 21, 2012 at 10:30:31PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 21, 2012 at 04:23:58PM -0300, Rafael Aquini wrote:
> > On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:
> > > >
> > > > I believe rcu_dereference_protected() is what I want/need here, since this code
> > > > is always called for pages which we hold locked (PG_locked bit).
> > >
> > > It would only help if we locked the page while updating the mapping,
> > > as far as I can see we don't.
> > >
> >
> > But we can do it. In fact, by doing it (locking the page) we can easily avoid
> > the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.
>
> Absolutely. Further, we should look hard at whether most RCU uses
> in this patchset can be replaced with page lock.
>
Yeah, In fact, by testing/grabbing the page lock at leak_balloon() even the
module unload X migration / putback race seems to fade away, since migration
code holds the page locked all the way.
And that seems a quite easy task to be accomplished:
....
@@ -169,21 +197,61 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ mutex_lock(&vb->balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+ spin_lock(&vb->pages_lock);
+ /*
+ * 'virtballoon_isolatepage()' can drain vb->pages list
+ * making us to stumble across a _temporarily_ empty list.
+ *
+ * Release the spinlock and resume from here in order to
+ * give page migration a shot to refill vb->pages list.
+ */
+ if (unlikely(list_empty(&vb->pages))) {
+ spin_unlock(&vb->pages_lock);
+ break;
+ }
+
page = list_first_entry(&vb->pages, struct page, lru);
+
+ /*
+ * Grab the page lock to avoid racing against threads isolating
+ * pages from vb->pages list (it's done under page lock).
+ *
+ * Failing to grab the page lock here means this page has been
+ * selected for isolation already.
+ */
+ if (!trylock_page(page)) {
+ spin_unlock(&vb->pages_lock);
+ break;
+ }
+
+ clear_balloon_mapping(page);
list_del(&page->lru);
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
+ unlock_page(page);
+ spin_unlock(&vb->pages_lock);
}
.....
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 19:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821191612.GA9027@redhat.com>
On Tue, Aug 21, 2012 at 10:16:12PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 21, 2012 at 02:55:03PM -0300, Rafael Aquini wrote:
> > On Tue, Aug 21, 2012 at 04:52:23PM +0300, Michael S. Tsirkin wrote:
> > > > + * address_space_operations utilized methods for ballooned pages:
> > > > + * .migratepage - used to perform balloon's page migration (as is)
> > > > + * .launder_page - used to isolate a page from balloon's page list
> > > > + * .freepage - used to reinsert an isolated page to balloon's page list
> > > > + */
> > >
> > > It would be a good idea to document the assumptions here.
> > > Looks like .launder_page and .freepage are called in rcu critical
> > > section.
> > > But migratepage isn't - why is that safe?
> > >
> >
> > The migratepage callback for virtio_balloon can sleep, and IIUC we cannot sleep
> > within a RCU critical section.
> >
> > Also, The migratepage callback is called at inner migration's circle function
> > move_to_new_page(), and I don't think embedding it in a RCU critical section
> > would be a good idea, for the same understanding aforementioned.
>
> Yes but this means it is still exposed to the module unloading
> races that RCU was supposed to fix.
> So need to either rework that code so it won't sleep
> or switch to some other synchronization.
>
Can you refactor tell_host() to not sleep? Or, can I get rid of calling it at
virtballoon_migratepage()? If 'no' is the answer for both questions, that's the
way that code has to remain, even if we find a way around to hack the
migratepage callback and have it embedded into a RCU crit section.
That's why I believe once the balloon driver is commanded to unload, we must
flag virtballoon_migratepage to skip it's work. By doing this, the thread
performing memory compaction will have to recur to the 'putback' path which is
RCU protected. (IMHO).
As the module will not uload utill it leaks all pages on its list, that unload
race you pointed before will be covered.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 19:30 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120821192357.GD12294@t510.redhat.com>
On Tue, Aug 21, 2012 at 04:23:58PM -0300, Rafael Aquini wrote:
> On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:
> > >
> > > I believe rcu_dereference_protected() is what I want/need here, since this code
> > > is always called for pages which we hold locked (PG_locked bit).
> >
> > It would only help if we locked the page while updating the mapping,
> > as far as I can see we don't.
> >
>
> But we can do it. In fact, by doing it (locking the page) we can easily avoid
> the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.
Absolutely. Further, we should look hard at whether most RCU uses
in this patchset can be replaced with page lock.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 19:28 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821174251.GB12294@t510.redhat.com>
On Tue, Aug 21, 2012 at 02:42:52PM -0300, Rafael Aquini wrote:
> On Tue, Aug 21, 2012 at 06:41:42PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 21, 2012 at 05:16:06PM +0200, Peter Zijlstra wrote:
> > > On Tue, 2012-08-21 at 16:52 +0300, Michael S. Tsirkin wrote:
> > > > > + rcu_read_lock();
> > > > > + mapping = rcu_dereference(page->mapping);
> > > > > + if (mapping_balloon(mapping))
> > > > > + ret = true;
> > > > > + rcu_read_unlock();
> > > >
> > > > This looks suspicious: you drop rcu_read_unlock
> > > > so can't page switch from balloon to non balloon?
> > >
> > > RCU read lock is a non-exclusive lock, it cannot avoid anything like
> > > that.
> >
> > You are right, of course. So even keeping rcu_read_lock across both test
> > and operation won't be enough - you need to make this function return
> > the mapping and pass it to isolate_page/putback_page so that it is only
> > dereferenced once.
> >
> No, I need to dereference page->mapping to check ->mapping flags here, before
> returning. Remember this function is used at MM's compaction/migration inner
> circles to identify ballooned pages and decide what's the next step. This
> function is doing the right thing, IMHO.
Yes but the calling code is not doing the right thing.
What Peter pointed out here is that two calls to rcu dereference pointer
can return different values: rcu critical section is not a lock.
So the test for balloon page is not effective: it can change
after the fact.
To fix, get the pointer once and then pass the mapping
around.
> Also, looking at how compaction/migration work, we verify the only critical path
> for this function is the page isolation step. The other steps (migration and
> putback) perform their work on private lists previouly isolated from a given
> source.
I vaguely understand but it would be nice to document this properly.
The interaction between page->lru handling in balloon and in mm
is especially confusing.
> So, we just need to make sure that the isolation part does not screw things up
> by isolating pages that balloon driver is about to release. That's why there are
> so many checkpoints down the page isolation path assuring we really are
> isolating a balloon page.
Well, testing same thing multiple times is just confusing. It is very
hard to make sure there are no races with so much complexity,
and the requirements from the balloon driver are unclear to me -
it very much looks like it is poking in mm internals.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 19:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120821191330.GA8324@redhat.com>
On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:
> >
> > I believe rcu_dereference_protected() is what I want/need here, since this code
> > is always called for pages which we hold locked (PG_locked bit).
>
> It would only help if we locked the page while updating the mapping,
> as far as I can see we don't.
>
But we can do it. In fact, by doing it (locking the page) we can easily avoid
the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 19:16 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821175502.GC12294@t510.redhat.com>
On Tue, Aug 21, 2012 at 02:55:03PM -0300, Rafael Aquini wrote:
> On Tue, Aug 21, 2012 at 04:52:23PM +0300, Michael S. Tsirkin wrote:
> > > + * address_space_operations utilized methods for ballooned pages:
> > > + * .migratepage - used to perform balloon's page migration (as is)
> > > + * .launder_page - used to isolate a page from balloon's page list
> > > + * .freepage - used to reinsert an isolated page to balloon's page list
> > > + */
> >
> > It would be a good idea to document the assumptions here.
> > Looks like .launder_page and .freepage are called in rcu critical
> > section.
> > But migratepage isn't - why is that safe?
> >
>
> The migratepage callback for virtio_balloon can sleep, and IIUC we cannot sleep
> within a RCU critical section.
>
> Also, The migratepage callback is called at inner migration's circle function
> move_to_new_page(), and I don't think embedding it in a RCU critical section
> would be a good idea, for the same understanding aforementioned.
Yes but this means it is still exposed to the module unloading
races that RCU was supposed to fix.
So need to either rework that code so it won't sleep
or switch to some other synchronization.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 19:13 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120821172819.GA12294@t510.redhat.com>
On Tue, Aug 21, 2012 at 02:28:20PM -0300, Rafael Aquini wrote:
> On Tue, Aug 21, 2012 at 09:24:32AM -0700, Paul E. McKenney wrote:
> > On Tue, Aug 21, 2012 at 05:20:11PM +0200, Peter Zijlstra wrote:
> > > On Tue, 2012-08-21 at 09:47 -0300, Rafael Aquini wrote:
> > > > + mapping = rcu_access_pointer(page->mapping);
> > > > + if (mapping)
> > > > + mapping = mapping->assoc_mapping;
> > >
> > > The comment near rcu_access_pointer() explicitly says:
> > >
> > > * Return the value of the specified RCU-protected pointer, but omit the
> > > * smp_read_barrier_depends() and keep the ACCESS_ONCE(). This is useful
> > > * when the value of this pointer is accessed, but the pointer is not
> > > * dereferenced,
> > >
> > > Yet you dereference the pointer... smells like fail to me.
> >
> > Indeed!
> >
> > This will break DEC Alpha. In addition, if ->mapping can transition
> > from non-NULL to NULL, and if you used rcu_access_pointer() rather
> > than rcu_dereference() to avoid lockdep-RCU from yelling at you about
> > not either being in an RCU read-side critical section or holding an
> > update-side lock, you can see failures as follows:
> >
> > 1. CPU 0 runs the above code, picks up mapping, and finds it non-NULL.
> >
> > 2. CPU 0 is preempted or otherwise delayed. (Keep in mind that
> > even disabling interrupts in a guest OS does not prevent the
> > host hypervisor from preempting!)
> >
> > 3. Some other CPU NULLs page->mapping. Because CPU 0 isn't doing
> > anything to prevent it, this other CPU frees the memory.
> >
> > 4. CPU 0 resumes, and then accesses what is now the freelist.
> > Arbitrarily bad things start happening.
> >
> > If you are in a read-side critical section, use rcu_dereference() instead
> > of rcu_access_pointer(). If you are holding an update-side lock, use
> > rcu_dereference_protected() and say what lock you are holding. If you
> > are doing something else, please say what it is.
> >
> > Thanx, Paul
> >
> Paul & Peter,
>
> Thanks for looking into this stuff and providing me such valuable feedback, and
> RCU usage crashcourse.
>
> I believe rcu_dereference_protected() is what I want/need here, since this code
> is always called for pages which we hold locked (PG_locked bit).
It would only help if we locked the page while updating the mapping,
as far as I can see we don't.
> So, it brings me
> to ask you if the following usage looks sane enough to fix the well pointed issue,
> or if it's another misuse of RCU API:
>
> + mapping = rcu_dereference_protecetd(page->mapping, PageLocked(page));
> + if (mapping)
> + mapping = mapping->assoc_mapping;
>
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Blue Swirl @ 2012-08-21 18:31 UTC (permalink / raw)
To: Cong Meng
Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization,
Paolo Bonzini
In-Reply-To: <1345537427-21601-1-git-send-email-mc@linux.vnet.ibm.com>
On Tue, Aug 21, 2012 at 8:23 AM, Cong Meng <mc@linux.vnet.ibm.com> wrote:
> This patch adds some queue limit parameters into block drive. And inits them
> for sg block drive. Some interfaces are also added for accessing them.
>
> Signed-off-by: Cong Meng <mc@linux.vnet.ibm.com>
> ---
> block/raw-posix.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> block_int.h | 4 +++
> blockdev.c | 15 +++++++++++++
> hw/block-common.h | 3 ++
> 4 files changed, 80 insertions(+), 0 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 0dce089..a086f89 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -53,6 +53,7 @@
> #include <linux/cdrom.h>
> #include <linux/fd.h>
> #include <linux/fs.h>
> +#include <dirent.h>
> #endif
> #ifdef CONFIG_FIEMAP
> #include <linux/fiemap.h>
> @@ -829,6 +830,62 @@ static int hdev_probe_device(const char *filename)
> return 0;
> }
>
> +static void read_queue_limit(char *path, const char *filename, unsigned int *val)
> +{
> + FILE *f;
> + char *tail = path + strlen(path);
> +
> + pstrcat(path, MAXPATHLEN, filename);
> + f = fopen(path, "r");
> + if (!f) {
> + goto out;
> + }
> +
> + fscanf(f, "%u", val);
Please handle errors, otherwise *val may be left to uninitialized state.
> + fclose(f);
> +
> +out:
> + *tail = 0;
> +}
> +
> +static void sg_get_queue_limits(BlockDriverState *bs, const char *filename)
> +{
> + DIR *ffs;
> + struct dirent *d;
> + char path[MAXPATHLEN];
> +
> + snprintf(path, MAXPATHLEN,
> + "/sys/class/scsi_generic/sg%s/device/block/",
> + filename + strlen("/dev/sg"));
> +
I'd init bs->max_sectors etc. here so they are not left uninitialized.
> + ffs = opendir(path);
> + if (!ffs) {
> + return;
> + }
> +
> + for (;;) {
> + d = readdir(ffs);
> + if (!d) {
> + return;
> + }
> +
> + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) {
> + continue;
> + }
> +
> + break;
> + }
> +
> + closedir(ffs);
> +
> + pstrcat(path, MAXPATHLEN, d->d_name);
> + pstrcat(path, MAXPATHLEN, "/queue/");
> +
> + read_queue_limit(path, "max_sectors_kb", &bs->max_sectors);
> + read_queue_limit(path, "max_segments", &bs->max_segments);
> + read_queue_limit(path, "max_segment_size", &bs->max_segment_size);
> +}
> +
> static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
> {
> BDRVRawState *s = bs->opaque;
> @@ -868,6 +925,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
> temp = realpath(filename, resolved_path);
> if (temp && strstart(temp, "/dev/sg", NULL)) {
> bs->sg = 1;
> + sg_get_queue_limits(bs, temp);
> }
> }
> #endif
> diff --git a/block_int.h b/block_int.h
> index d72317f..a9d07a2 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -333,6 +333,10 @@ struct BlockDriverState {
>
> /* long-running background operation */
> BlockJob *job;
> +
> + unsigned int max_sectors;
With 32 bit ints and even with 4k sector size, the maximum disk size
would be 16TB which may be soon exceeded by disks on the market.
Please use 64 bit values, probably also for segment values below.
> + unsigned int max_segments;
> + unsigned int max_segment_size;
> };
>
> int get_tmp_filename(char *filename, int size);
> diff --git a/blockdev.c b/blockdev.c
> index 3d75015..e17edbf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1191,3 +1191,18 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
> bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
> return dummy.next;
> }
> +
> +unsigned int get_queue_max_sectors(BlockDriverState *bs)
> +{
> + return (bs->file && bs->file->sg) ? bs->file->max_sectors : 0;
> +}
> +
> +unsigned int get_queue_max_segments(BlockDriverState *bs)
> +{
> + return (bs->file && bs->file->sg) ? bs->file->max_segments : 0;
> +}
> +
> +unsigned int get_queue_max_segment_size(BlockDriverState *bs)
> +{
> + return (bs->file && bs->file->sg) ? bs->file->max_segment_size : 0;
> +}
> diff --git a/hw/block-common.h b/hw/block-common.h
> index bb808f7..d47fcd7 100644
> --- a/hw/block-common.h
> +++ b/hw/block-common.h
> @@ -76,4 +76,7 @@ void hd_geometry_guess(BlockDriverState *bs,
> int *ptrans);
> int hd_bios_chs_auto_trans(uint32_t cyls, uint32_t heads, uint32_t secs);
>
> +unsigned int get_queue_max_sectors(BlockDriverState *bs);
> +unsigned int get_queue_max_segments(BlockDriverState *bs);
> +unsigned int get_queue_max_segment_size(BlockDriverState *bs);
> #endif
> --
> 1.7.7.6
>
>
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 17:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, linux-kernel, virtualization,
linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821135223.GA7117@redhat.com>
On Tue, Aug 21, 2012 at 04:52:23PM +0300, Michael S. Tsirkin wrote:
> > + * address_space_operations utilized methods for ballooned pages:
> > + * .migratepage - used to perform balloon's page migration (as is)
> > + * .launder_page - used to isolate a page from balloon's page list
> > + * .freepage - used to reinsert an isolated page to balloon's page list
> > + */
>
> It would be a good idea to document the assumptions here.
> Looks like .launder_page and .freepage are called in rcu critical
> section.
> But migratepage isn't - why is that safe?
>
The migratepage callback for virtio_balloon can sleep, and IIUC we cannot sleep
within a RCU critical section.
Also, The migratepage callback is called at inner migration's circle function
move_to_new_page(), and I don't think embedding it in a RCU critical section
would be a good idea, for the same understanding aforementioned.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 17:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821154142.GA8268@redhat.com>
On Tue, Aug 21, 2012 at 06:41:42PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 21, 2012 at 05:16:06PM +0200, Peter Zijlstra wrote:
> > On Tue, 2012-08-21 at 16:52 +0300, Michael S. Tsirkin wrote:
> > > > + rcu_read_lock();
> > > > + mapping = rcu_dereference(page->mapping);
> > > > + if (mapping_balloon(mapping))
> > > > + ret = true;
> > > > + rcu_read_unlock();
> > >
> > > This looks suspicious: you drop rcu_read_unlock
> > > so can't page switch from balloon to non balloon?
> >
> > RCU read lock is a non-exclusive lock, it cannot avoid anything like
> > that.
>
> You are right, of course. So even keeping rcu_read_lock across both test
> and operation won't be enough - you need to make this function return
> the mapping and pass it to isolate_page/putback_page so that it is only
> dereferenced once.
>
No, I need to dereference page->mapping to check ->mapping flags here, before
returning. Remember this function is used at MM's compaction/migration inner
circles to identify ballooned pages and decide what's the next step. This
function is doing the right thing, IMHO.
Also, looking at how compaction/migration work, we verify the only critical path
for this function is the page isolation step. The other steps (migration and
putback) perform their work on private lists previouly isolated from a given
source.
So, we just need to make sure that the isolation part does not screw things up
by isolating pages that balloon driver is about to release. That's why there are
so many checkpoints down the page isolation path assuring we really are
isolating a balloon page.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-21 17:28 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
Peter Zijlstra, linux-kernel, virtualization, linux-mm,
Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120821162432.GG2456@linux.vnet.ibm.com>
On Tue, Aug 21, 2012 at 09:24:32AM -0700, Paul E. McKenney wrote:
> On Tue, Aug 21, 2012 at 05:20:11PM +0200, Peter Zijlstra wrote:
> > On Tue, 2012-08-21 at 09:47 -0300, Rafael Aquini wrote:
> > > + mapping = rcu_access_pointer(page->mapping);
> > > + if (mapping)
> > > + mapping = mapping->assoc_mapping;
> >
> > The comment near rcu_access_pointer() explicitly says:
> >
> > * Return the value of the specified RCU-protected pointer, but omit the
> > * smp_read_barrier_depends() and keep the ACCESS_ONCE(). This is useful
> > * when the value of this pointer is accessed, but the pointer is not
> > * dereferenced,
> >
> > Yet you dereference the pointer... smells like fail to me.
>
> Indeed!
>
> This will break DEC Alpha. In addition, if ->mapping can transition
> from non-NULL to NULL, and if you used rcu_access_pointer() rather
> than rcu_dereference() to avoid lockdep-RCU from yelling at you about
> not either being in an RCU read-side critical section or holding an
> update-side lock, you can see failures as follows:
>
> 1. CPU 0 runs the above code, picks up mapping, and finds it non-NULL.
>
> 2. CPU 0 is preempted or otherwise delayed. (Keep in mind that
> even disabling interrupts in a guest OS does not prevent the
> host hypervisor from preempting!)
>
> 3. Some other CPU NULLs page->mapping. Because CPU 0 isn't doing
> anything to prevent it, this other CPU frees the memory.
>
> 4. CPU 0 resumes, and then accesses what is now the freelist.
> Arbitrarily bad things start happening.
>
> If you are in a read-side critical section, use rcu_dereference() instead
> of rcu_access_pointer(). If you are holding an update-side lock, use
> rcu_dereference_protected() and say what lock you are holding. If you
> are doing something else, please say what it is.
>
> Thanx, Paul
>
Paul & Peter,
Thanks for looking into this stuff and providing me such valuable feedback, and
RCU usage crashcourse.
I believe rcu_dereference_protected() is what I want/need here, since this code
is always called for pages which we hold locked (PG_locked bit). So, it brings me
to ask you if the following usage looks sane enough to fix the well pointed issue,
or if it's another misuse of RCU API:
+ mapping = rcu_dereference_protecetd(page->mapping, PageLocked(page));
+ if (mapping)
+ mapping = mapping->assoc_mapping;
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Paul E. McKenney @ 2012-08-21 16:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, linux-mm,
Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <1345562411.23018.111.camel@twins>
On Tue, Aug 21, 2012 at 05:20:11PM +0200, Peter Zijlstra wrote:
> On Tue, 2012-08-21 at 09:47 -0300, Rafael Aquini wrote:
> > + mapping = rcu_access_pointer(page->mapping);
> > + if (mapping)
> > + mapping = mapping->assoc_mapping;
>
> The comment near rcu_access_pointer() explicitly says:
>
> * Return the value of the specified RCU-protected pointer, but omit the
> * smp_read_barrier_depends() and keep the ACCESS_ONCE(). This is useful
> * when the value of this pointer is accessed, but the pointer is not
> * dereferenced,
>
> Yet you dereference the pointer... smells like fail to me.
Indeed!
This will break DEC Alpha. In addition, if ->mapping can transition
from non-NULL to NULL, and if you used rcu_access_pointer() rather
than rcu_dereference() to avoid lockdep-RCU from yelling at you about
not either being in an RCU read-side critical section or holding an
update-side lock, you can see failures as follows:
1. CPU 0 runs the above code, picks up mapping, and finds it non-NULL.
2. CPU 0 is preempted or otherwise delayed. (Keep in mind that
even disabling interrupts in a guest OS does not prevent the
host hypervisor from preempting!)
3. Some other CPU NULLs page->mapping. Because CPU 0 isn't doing
anything to prevent it, this other CPU frees the memory.
4. CPU 0 resumes, and then accesses what is now the freelist.
Arbitrarily bad things start happening.
If you are in a read-side critical section, use rcu_dereference() instead
of rcu_access_pointer(). If you are holding an update-side lock, use
rcu_dereference_protected() and say what lock you are holding. If you
are doing something else, please say what it is.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-21 15:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <1345562166.23018.109.camel@twins>
On Tue, Aug 21, 2012 at 05:16:06PM +0200, Peter Zijlstra wrote:
> On Tue, 2012-08-21 at 16:52 +0300, Michael S. Tsirkin wrote:
> > > + rcu_read_lock();
> > > + mapping = rcu_dereference(page->mapping);
> > > + if (mapping_balloon(mapping))
> > > + ret = true;
> > > + rcu_read_unlock();
> >
> > This looks suspicious: you drop rcu_read_unlock
> > so can't page switch from balloon to non balloon?
>
> RCU read lock is a non-exclusive lock, it cannot avoid anything like
> that.
You are right, of course. So even keeping rcu_read_lock across both test
and operation won't be enough - you need to make this function return
the mapping and pass it to isolate_page/putback_page so that it is only
dereferenced once.
--
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