* [ 03/82] virtio-blk: Use block layer provided spinlock
From: Greg Kroah-Hartman @ 2012-08-13 20:18 UTC (permalink / raw)
To: linux-kernel, stable
Cc: kvm, Michael S. Tsirkin, Greg KH, virtualization, akpm, torvalds,
alan
In-Reply-To: <20120813201746.448504360@linuxfoundation.org>
From: Greg KH <gregkh@linuxfoundation.org>
3.5-stable review patch. If anyone has any objections, please let me know.
------------------
From: Asias He <asias@redhat.com>
commit 2c95a3290919541b846bee3e0fbaa75860929f53 upstream.
Block layer will allocate a spinlock for the queue if the driver does
not provide one in blk_init_queue().
The reason to use the internal spinlock is that blk_cleanup_queue() will
switch to use the internal spinlock in the cleanup code path.
if (q->queue_lock != &q->__queue_lock)
q->queue_lock = &q->__queue_lock;
However, processes which are in D state might have taken the driver
provided spinlock, when the processes wake up, they would release the
block provided spinlock.
=====================================
[ BUG: bad unlock balance detected! ]
3.4.0-rc7+ #238 Not tainted
-------------------------------------
fio/3587 is trying to release lock (&(&q->__queue_lock)->rlock) at:
[<ffffffff813274d2>] blk_queue_bio+0x2a2/0x380
but there are no more locks to release!
other info that might help us debug this:
1 lock held by fio/3587:
#0: (&(&vblk->lock)->rlock){......}, at:
[<ffffffff8132661a>] get_request_wait+0x19a/0x250
Other drivers use block layer provided spinlock as well, e.g. SCSI.
Switching to the block layer provided spinlock saves a bit of memory and
does not increase lock contention. Performance test shows no real
difference is observed before and after this patch.
Changes in v2: Improve commit log as Michael suggested.
Signed-off-by: Asias He <asias@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/block/virtio_blk.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -21,8 +21,6 @@ struct workqueue_struct *virtblk_wq;
struct virtio_blk
{
- spinlock_t lock;
-
struct virtio_device *vdev;
struct virtqueue *vq;
@@ -65,7 +63,7 @@ static void blk_done(struct virtqueue *v
unsigned int len;
unsigned long flags;
- spin_lock_irqsave(&vblk->lock, flags);
+ spin_lock_irqsave(vblk->disk->queue->queue_lock, flags);
while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
int error;
@@ -99,7 +97,7 @@ static void blk_done(struct virtqueue *v
}
/* In case queue is stopped waiting for more buffers. */
blk_start_queue(vblk->disk->queue);
- spin_unlock_irqrestore(&vblk->lock, flags);
+ spin_unlock_irqrestore(vblk->disk->queue->queue_lock, flags);
}
static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
@@ -431,7 +429,6 @@ static int __devinit virtblk_probe(struc
goto out_free_index;
}
- spin_lock_init(&vblk->lock);
vblk->vdev = vdev;
vblk->sg_elems = sg_elems;
sg_init_table(vblk->sg, vblk->sg_elems);
@@ -456,7 +453,7 @@ static int __devinit virtblk_probe(struc
goto out_mempool;
}
- q = vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
+ q = vblk->disk->queue = blk_init_queue(do_virtblk_request, NULL);
if (!q) {
err = -ENOMEM;
goto out_put_disk;
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rusty Russell @ 2012-08-13 23:59 UTC (permalink / raw)
To: Michael S. Tsirkin, Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Minchan Kim, linux-kernel,
virtualization, linux-mm, Andi Kleen, Andrew Morton
In-Reply-To: <20120813084123.GF14081@redhat.com>
On Mon, 13 Aug 2012 11:41:23 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Aug 10, 2012 at 02:55:15PM -0300, Rafael Aquini wrote:
> > +/*
> > + * Populate balloon_mapping->a_ops->freepage method to help compaction on
> > + * re-inserting an isolated page into the balloon page list.
> > + */
> > +void virtballoon_putbackpage(struct page *page)
> > +{
> > + spin_lock(&pages_lock);
> > + list_add(&page->lru, &vb_ptr->pages);
> > + spin_unlock(&pages_lock);
>
> Could the following race trigger:
> migration happens while module unloading is in progress,
> module goes away between here and when the function
> returns, then code for this function gets overwritten?
> If yes we need locking external to module to prevent this.
> Maybe add a spinlock to struct address_space?
The balloon module cannot be unloaded until it has leaked all its pages,
so I think this is safe:
static void remove_common(struct virtio_balloon *vb)
{
/* There might be pages left in the balloon: free them. */
while (vb->num_pages)
leak_balloon(vb, vb->num_pages);
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
From: Greg KH @ 2012-08-14 1:38 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: olaf, linux-kernel, virtualization, apw, devel, ben
In-Reply-To: <1344877627-21779-2-git-send-email-kys@microsoft.com>
On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> Add the necessary definitions for supporting the IP injection functionality.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Olaf Hering <olaf@aepfle.de>
> Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> drivers/hv/hv_util.c | 4 +-
> include/linux/hyperv.h | 76 ++++++++++++++++++++++++++++++++++++++++++++-
> tools/hv/hv_kvp_daemon.c | 2 +-
> 3 files changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index d3ac6a4..a0667de 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
> (struct hv_util_service *)dev_id->driver_data;
> int ret;
>
> - srv->recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + srv->recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> if (!srv->recv_buffer)
> return -ENOMEM;
> if (srv->util_init) {
> @@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
> }
> }
>
> - ret = vmbus_open(dev->channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL, 0,
> + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL, 0,
> srv->util_cb, dev->channel);
> if (ret)
> goto error;
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 68ed7f7..11afc4e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -122,12 +122,53 @@
> #define REG_U32 4
> #define REG_U64 8
>
> +/*
> + * As we look at expanding the KVP functionality to include
> + * IP injection functionality, we need to maintain binary
> + * compatibility with older daemons.
> + *
> + * The KVP opcodes are defined by the host and it was unfortunate
> + * that I chose to treat the registration operation as part of the
> + * KVP operations defined by the host.
> + * Here is the level of compatibility
> + * (between the user level daemon and the kernel KVP driver) that we
> + * will implement:
> + *
> + * An older daemon will always be supported on a newer driver.
> + * A given user level daemon will require a minimal version of the
> + * kernel driver.
> + * If we cannot handle the version differences, we will fail gracefully
> + * (this can happen when we have a user level daemon that is more
> + * advanced than the KVP driver.
> + *
> + * We will use values used in this handshake for determining if we have
> + * workable user level daemon and the kernel driver. We begin by taking the
> + * registration opcode out of the KVP opcode namespace. We will however,
> + * maintain compatibility with the existing user-level daemon code.
> + */
> +
> +/*
> + * Daemon code not supporting IP injection (legacy daemon).
> + */
> +
> +#define KVP_OP_REGISTER 4
Huh?
> +/*
> + * Daemon code supporting IP injection.
> + * The KVP opcode field is used to communicate the
> + * registration information; so define a namespace that
> + * will be distinct from the host defined KVP opcode.
> + */
> +
> +#define KVP_OP_REGISTER1 100
> +
> enum hv_kvp_exchg_op {
> KVP_OP_GET = 0,
> KVP_OP_SET,
> KVP_OP_DELETE,
> KVP_OP_ENUMERATE,
> - KVP_OP_REGISTER,
> + KVP_OP_GET_IP_INFO,
> + KVP_OP_SET_IP_INFO,
So you overloaded the command and somehow think that is ok? How is that
supposed to work? Why not just always keep it there, but fail if it is
called as you know you have a mismatch?
Otherwise, again, you just broke older tools on a newer kernel.
Or am I missing something here?
confused,
greg k-h
^ permalink raw reply
* Re: [PATCH V2 06/18] Tools: hv: Further refactor kvp_get_ip_address()
From: Ben Hutchings @ 2012-08-14 1:46 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, devel, virtualization, olaf, apw
In-Reply-To: <1344877627-21779-6-git-send-email-kys@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 4125 bytes --]
On Mon, 2012-08-13 at 10:06 -0700, K. Y. Srinivasan wrote:
> In preparation for making kvp_get_ip_address() more generic, factor out
> the code for handling IP addresses.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Olaf Hering <olaf@aepfle.de>
> Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
I looked at your last patch set, so in a sense these have been reviewed
by me. But the 'Reviewed-by' line in a commit message means the
reviewer thinks it's OK; the reviewer must say that, and I didn't.
Ben.
> ---
> tools/hv/hv_kvp_daemon.c | 94 ++++++++++++++++++++-------------------------
> 1 files changed, 42 insertions(+), 52 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 3af37f0..3dc989f 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -491,17 +491,50 @@ done:
> return;
> }
>
> +static int kvp_process_ip_address(void *addrp,
> + int family, char *buffer,
> + int length, int *offset)
> +{
> + struct sockaddr_in *addr;
> + struct sockaddr_in6 *addr6;
> + int addr_length;
> + char tmp[50];
> + const char *str;
> +
> + if (family == AF_INET) {
> + addr = (struct sockaddr_in *)addrp;
> + str = inet_ntop(family, &addr->sin_addr, tmp, 50);
> + addr_length = INET_ADDRSTRLEN;
> + } else {
> + addr6 = (struct sockaddr_in6 *)addrp;
> + str = inet_ntop(family, &addr6->sin6_addr.s6_addr, tmp, 50);
> + addr_length = INET6_ADDRSTRLEN;
> + }
> +
> + if ((length - *offset) < addr_length + 1)
> + return 1;
> + if (str == NULL) {
> + strcpy(buffer, "inet_ntop failed\n");
> + return 1;
> + }
> + if (*offset == 0)
> + strcpy(buffer, tmp);
> + else
> + strcat(buffer, tmp);
> + strcat(buffer, ";");
> +
> + *offset += strlen(str) + 1;
> + return 0;
> +}
> +
> static int
> kvp_get_ip_address(int family, char *if_name, int op,
> void *out_buffer, int length)
> {
> struct ifaddrs *ifap;
> struct ifaddrs *curp;
> - int ipv4_len = strlen("255.255.255.255") + 1;
> - int ipv6_len = strlen("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff")+1;
> int offset = 0;
> const char *str;
> - char tmp[50];
> int error = 0;
> char *buffer;
> struct hv_kvp_ipaddr_value *ip_buffer;
> @@ -556,55 +589,12 @@ kvp_get_ip_address(int family, char *if_name, int op,
> continue;
> }
>
> - if ((curp->ifa_addr->sa_family == AF_INET) &&
> - ((family == AF_INET) || (family == 0))) {
> - struct sockaddr_in *addr =
> - (struct sockaddr_in *) curp->ifa_addr;
> -
> - str = inet_ntop(AF_INET, &addr->sin_addr, tmp, 50);
> - if (str == NULL) {
> - strcpy(buffer, "inet_ntop failed\n");
> - error = 1;
> - goto getaddr_done;
> - }
> - if (offset == 0)
> - strcpy(buffer, tmp);
> - else
> - strcat(buffer, tmp);
> - strcat(buffer, ";");
> -
> - offset += strlen(str) + 1;
> - if ((length - offset) < (ipv4_len + 1))
> - goto getaddr_done;
> -
> - } else if ((family == AF_INET6) || (family == 0)) {
> -
> - /*
> - * We only support AF_INET and AF_INET6
> - * and the list of addresses is separated by a ";".
> - */
> - struct sockaddr_in6 *addr =
> - (struct sockaddr_in6 *) curp->ifa_addr;
> -
> - str = inet_ntop(AF_INET6,
> - &addr->sin6_addr.s6_addr,
> - tmp, 50);
> - if (str == NULL) {
> - strcpy(buffer, "inet_ntop failed\n");
> - error = 1;
> - goto getaddr_done;
> - }
> - if (offset == 0)
> - strcpy(buffer, tmp);
> - else
> - strcat(buffer, tmp);
> - strcat(buffer, ";");
> - offset += strlen(str) + 1;
> - if ((length - offset) < (ipv6_len + 1))
> - goto getaddr_done;
> -
> - }
> -
> + error = kvp_process_ip_address(curp->ifa_addr,
> + curp->ifa_addr->sa_family,
> + buffer,
> + length, &offset);
> + if (error)
> + goto getaddr_done;
>
> curp = curp->ifa_next;
> }
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
From: Ben Hutchings @ 2012-08-14 1:56 UTC (permalink / raw)
To: Greg KH; +Cc: K. Y. Srinivasan, linux-kernel, devel, virtualization, olaf, apw
In-Reply-To: <20120814013815.GA29829@kroah.com>
[-- Attachment #1: Type: text/plain, Size: 3254 bytes --]
On Mon, 2012-08-13 at 18:38 -0700, Greg KH wrote:
> On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> > Add the necessary definitions for supporting the IP injection functionality.
[...]
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -122,12 +122,53 @@
> > #define REG_U32 4
> > #define REG_U64 8
> >
> > +/*
> > + * As we look at expanding the KVP functionality to include
> > + * IP injection functionality, we need to maintain binary
> > + * compatibility with older daemons.
> > + *
> > + * The KVP opcodes are defined by the host and it was unfortunate
> > + * that I chose to treat the registration operation as part of the
> > + * KVP operations defined by the host.
> > + * Here is the level of compatibility
> > + * (between the user level daemon and the kernel KVP driver) that we
> > + * will implement:
> > + *
> > + * An older daemon will always be supported on a newer driver.
> > + * A given user level daemon will require a minimal version of the
> > + * kernel driver.
> > + * If we cannot handle the version differences, we will fail gracefully
> > + * (this can happen when we have a user level daemon that is more
> > + * advanced than the KVP driver.
> > + *
> > + * We will use values used in this handshake for determining if we have
> > + * workable user level daemon and the kernel driver. We begin by taking the
> > + * registration opcode out of the KVP opcode namespace. We will however,
> > + * maintain compatibility with the existing user-level daemon code.
> > + */
> > +
> > +/*
> > + * Daemon code not supporting IP injection (legacy daemon).
> > + */
> > +
> > +#define KVP_OP_REGISTER 4
>
> Huh?
>
> > +/*
> > + * Daemon code supporting IP injection.
> > + * The KVP opcode field is used to communicate the
> > + * registration information; so define a namespace that
> > + * will be distinct from the host defined KVP opcode.
> > + */
> > +
> > +#define KVP_OP_REGISTER1 100
> > +
> > enum hv_kvp_exchg_op {
> > KVP_OP_GET = 0,
> > KVP_OP_SET,
> > KVP_OP_DELETE,
> > KVP_OP_ENUMERATE,
> > - KVP_OP_REGISTER,
> > + KVP_OP_GET_IP_INFO,
> > + KVP_OP_SET_IP_INFO,
>
> So you overloaded the command and somehow think that is ok? How is that
> supposed to work? Why not just always keep it there, but fail if it is
> called as you know you have a mismatch?
>
> Otherwise, again, you just broke older tools on a newer kernel.
>
> Or am I missing something here?
You are. The above enumeration is for the hypervisor-to-guest protocol,
whereas KVP_OP_REGISTER and KVP_OP_REGISTER1 are only used between
daemon and driver. The registration operation code should have been
defined as a sufficiently high value to avoid collision.
However, since the daemon will always send one of the registration
operations at start of day (and then never again), it seems that the
driver can avoid confusing registration with a reply to
KVP_OP_GET_IP_INFO. Instead, the two registration operation codes
distinguish the capabilities of the daemon and actually aid backward
compatibility.
Ben.
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: [PATCH V2 18/18] Tools: hv: Properly manage open streams
From: Ben Hutchings @ 2012-08-14 1:57 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, devel, virtualization, olaf, apw
In-Reply-To: <1344877627-21779-18-git-send-email-kys@microsoft.com>
[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]
On Mon, 2012-08-13 at 10:07 -0700, K. Y. Srinivasan wrote:
> Close the open streams properly.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Reviewed-by: Olaf Hering <olaf@aepfle.de>
> Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
This isn't reviewed by me, I'm the author!
Ben.
> ---
> tools/hv/hv_kvp_daemon.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index c8e1013..ca6424f 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -160,7 +160,7 @@ static void kvp_update_file(int pool)
> sizeof(struct kvp_record),
> kvp_file_info[pool].num_records, filep);
>
> - fflush(filep);
> + fclose(filep);
> kvp_release_lock(pool);
> }
>
> @@ -206,7 +206,7 @@ static void kvp_update_mem_state(int pool)
> kvp_file_info[pool].num_blocks = num_blocks;
> kvp_file_info[pool].records = record;
> kvp_file_info[pool].num_records = records_read;
> -
> + fclose(filep);
> kvp_release_lock(pool);
> }
> static int kvp_file_init(void)
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
From: KY Srinivasan @ 2012-08-14 2:56 UTC (permalink / raw)
To: Greg KH
Cc: olaf@aepfle.de, linux-kernel@vger.kernel.org,
virtualization@lists.osdl.org, apw@canonical.com,
devel@linuxdriverproject.org, ben@decadent.org.uk
In-Reply-To: <20120814013815.GA29829@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Monday, August 13, 2012 9:38 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; olaf@aepfle.de; apw@canonical.com;
> ben@decadent.org.uk
> Subject: Re: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address
> injection
>
> On Mon, Aug 13, 2012 at 10:06:51AM -0700, K. Y. Srinivasan wrote:
> > Add the necessary definitions for supporting the IP injection functionality.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Reviewed-by: Olaf Hering <olaf@aepfle.de>
> > Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> > drivers/hv/hv_util.c | 4 +-
> > include/linux/hyperv.h | 76
> ++++++++++++++++++++++++++++++++++++++++++++-
> > tools/hv/hv_kvp_daemon.c | 2 +-
> > 3 files changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > index d3ac6a4..a0667de 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -263,7 +263,7 @@ static int util_probe(struct hv_device *dev,
> > (struct hv_util_service *)dev_id->driver_data;
> > int ret;
> >
> > - srv->recv_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > + srv->recv_buffer = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
> > if (!srv->recv_buffer)
> > return -ENOMEM;
> > if (srv->util_init) {
> > @@ -274,7 +274,7 @@ static int util_probe(struct hv_device *dev,
> > }
> > }
> >
> > - ret = vmbus_open(dev->channel, 2 * PAGE_SIZE, 2 * PAGE_SIZE, NULL,
> 0,
> > + ret = vmbus_open(dev->channel, 4 * PAGE_SIZE, 4 * PAGE_SIZE, NULL,
> 0,
> > srv->util_cb, dev->channel);
> > if (ret)
> > goto error;
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 68ed7f7..11afc4e 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -122,12 +122,53 @@
> > #define REG_U32 4
> > #define REG_U64 8
> >
> > +/*
> > + * As we look at expanding the KVP functionality to include
> > + * IP injection functionality, we need to maintain binary
> > + * compatibility with older daemons.
> > + *
> > + * The KVP opcodes are defined by the host and it was unfortunate
> > + * that I chose to treat the registration operation as part of the
> > + * KVP operations defined by the host.
> > + * Here is the level of compatibility
> > + * (between the user level daemon and the kernel KVP driver) that we
> > + * will implement:
> > + *
> > + * An older daemon will always be supported on a newer driver.
> > + * A given user level daemon will require a minimal version of the
> > + * kernel driver.
> > + * If we cannot handle the version differences, we will fail gracefully
> > + * (this can happen when we have a user level daemon that is more
> > + * advanced than the KVP driver.
> > + *
> > + * We will use values used in this handshake for determining if we have
> > + * workable user level daemon and the kernel driver. We begin by taking the
> > + * registration opcode out of the KVP opcode namespace. We will however,
> > + * maintain compatibility with the existing user-level daemon code.
> > + */
> > +
> > +/*
> > + * Daemon code not supporting IP injection (legacy daemon).
> > + */
> > +
> > +#define KVP_OP_REGISTER 4
>
> Huh?
>
> > +/*
> > + * Daemon code supporting IP injection.
> > + * The KVP opcode field is used to communicate the
> > + * registration information; so define a namespace that
> > + * will be distinct from the host defined KVP opcode.
> > + */
> > +
> > +#define KVP_OP_REGISTER1 100
> > +
> > enum hv_kvp_exchg_op {
> > KVP_OP_GET = 0,
> > KVP_OP_SET,
> > KVP_OP_DELETE,
> > KVP_OP_ENUMERATE,
> > - KVP_OP_REGISTER,
> > + KVP_OP_GET_IP_INFO,
> > + KVP_OP_SET_IP_INFO,
>
> So you overloaded the command and somehow think that is ok? How is that
> supposed to work? Why not just always keep it there, but fail if it is
> called as you know you have a mismatch?
>
> Otherwise, again, you just broke older tools on a newer kernel.
>
> Or am I missing something here?
Greg,
The registration operation occurs when the daemon first starts up. I should have established
a distinct namespace for the daemon versions that would not overlap with the host
defined KVP operations initially. Unfortunately when I first implemented KVP, I did not know
about the new KVP verbs and so selected a value that ended up colliding with the new KVP
operations. To maintain compatibility with older daemons, I have to support this old registration
value, which is what you are seeing here. Since the initial driver/daemon handshake phase does
not overlap with the normal functioning of the KVP stack, we can use the old daemon
registration value to distinguish that the daemon does not support IP injection. The current
implementation does support a compatible environment for older daemons.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH V2 06/18] Tools: hv: Further refactor kvp_get_ip_address()
From: KY Srinivasan @ 2012-08-14 3:01 UTC (permalink / raw)
To: Ben Hutchings
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
olaf@aepfle.de, apw@canonical.com
In-Reply-To: <1344908771.824.156.camel@deadeye.wl.decadent.org.uk>
> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Monday, August 13, 2012 9:46 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com
> Subject: Re: [PATCH V2 06/18] Tools: hv: Further refactor kvp_get_ip_address()
>
> On Mon, 2012-08-13 at 10:06 -0700, K. Y. Srinivasan wrote:
> > In preparation for making kvp_get_ip_address() more generic, factor out
> > the code for handling IP addresses.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Reviewed-by: Olaf Hering <olaf@aepfle.de>
> > Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
>
> I looked at your last patch set, so in a sense these have been reviewed
> by me. But the 'Reviewed-by' line in a commit message means the
> reviewer thinks it's OK; the reviewer must say that, and I didn't.
My mistake, sorry about that.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH V2 18/18] Tools: hv: Properly manage open streams
From: KY Srinivasan @ 2012-08-14 3:17 UTC (permalink / raw)
To: Ben Hutchings
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
olaf@aepfle.de, apw@canonical.com
In-Reply-To: <1344909469.824.167.camel@deadeye.wl.decadent.org.uk>
> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Monday, August 13, 2012 9:58 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com
> Subject: Re: [PATCH V2 18/18] Tools: hv: Properly manage open streams
>
> On Mon, 2012-08-13 at 10:07 -0700, K. Y. Srinivasan wrote:
> > Close the open streams properly.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Reviewed-by: Olaf Hering <olaf@aepfle.de>
> > Reviewed-by: Ben Hutchings <ben@decadent.org.uk>
>
> This isn't reviewed by me, I'm the author!
I am truly sorry for this confusion. I was cleaning up the code based on the
last set of comments I got from Olaf and I did not realize that you had authored
this very patch! Perhaps we can drop this patch from this set.
Regards,
K. Y
>
> Ben.
>
> > ---
> > tools/hv/hv_kvp_daemon.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > index c8e1013..ca6424f 100644
> > --- a/tools/hv/hv_kvp_daemon.c
> > +++ b/tools/hv/hv_kvp_daemon.c
> > @@ -160,7 +160,7 @@ static void kvp_update_file(int pool)
> > sizeof(struct kvp_record),
> > kvp_file_info[pool].num_records, filep);
> >
> > - fflush(filep);
> > + fclose(filep);
> > kvp_release_lock(pool);
> > }
> >
> > @@ -206,7 +206,7 @@ static void kvp_update_mem_state(int pool)
> > kvp_file_info[pool].num_blocks = num_blocks;
> > kvp_file_info[pool].records = record;
> > kvp_file_info[pool].num_records = records_read;
> > -
> > + fclose(filep);
> > kvp_release_lock(pool);
> > }
> > static int kvp_file_init(void)
>
> --
> Ben Hutchings
> I say we take off; nuke the site from orbit. It's the only way to be sure.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 8:33 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: <87lihis5qi.fsf@rustcorp.com.au>
On Tue, Aug 14, 2012 at 09:29:49AM +0930, Rusty Russell wrote:
> On Mon, 13 Aug 2012 11:41:23 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Fri, Aug 10, 2012 at 02:55:15PM -0300, Rafael Aquini wrote:
> > > +/*
> > > + * Populate balloon_mapping->a_ops->freepage method to help compaction on
> > > + * re-inserting an isolated page into the balloon page list.
> > > + */
> > > +void virtballoon_putbackpage(struct page *page)
> > > +{
> > > + spin_lock(&pages_lock);
> > > + list_add(&page->lru, &vb_ptr->pages);
> > > + spin_unlock(&pages_lock);
> >
> > Could the following race trigger:
> > migration happens while module unloading is in progress,
> > module goes away between here and when the function
> > returns, then code for this function gets overwritten?
> > If yes we need locking external to module to prevent this.
> > Maybe add a spinlock to struct address_space?
>
> The balloon module cannot be unloaded until it has leaked all its pages,
> so I think this is safe:
>
> static void remove_common(struct virtio_balloon *vb)
> {
> /* There might be pages left in the balloon: free them. */
> while (vb->num_pages)
> leak_balloon(vb, vb->num_pages);
>
> Cheers,
> Rusty.
I know I meant something else.
Let me lay this out:
CPU1 executes:
void virtballoon_putbackpage(struct page *page)
{
spin_lock(&pages_lock);
list_add(&page->lru, &vb_ptr->pages);
spin_unlock(&pages_lock);
at this point CPU2 unloads module:
leak_balloon
......
next CPU2 loads another module so code memory gets overwritten
now CPU1 executes the next instruction:
}
which would normally return to function's caller,
but it has been overwritten by CPU2 so we get corruption.
No?
--
MST
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-08-14 17:44 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: <20120813082619.GE14081@redhat.com>
On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote:
> > +static inline bool movable_balloon_page(struct page *page)
> > +{
> > + return (page->mapping && page->mapping == balloon_mapping);
>
> I am guessing this needs smp_read_barrier_depends, and maybe
> ACCESS_ONCE ...
>
I'm curious about your guessing here. Could you ellaborate it further, please?
> > +#else
> > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > +static inline void putback_balloon_page(struct page *page) { return false; }
> > +static inline bool movable_balloon_page(struct page *page) { return false; }
> > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
> > +
>
> This does mean that only one type of balloon is useable at a time.
> I wonder whether using a flag in address_space structure instead
> is possible ...
This means we are only introducing this feature for virtio_balloon by now.
Despite the flagging address_space stuff is something we surely can look in the
future, I quite didn't get how we could be using two different types of balloon
devices at the same time for the same system. Could you ellaborate it a little
more, please?
> > +/* __isolate_lru_page() counterpart for a ballooned page */
> > +bool isolate_balloon_page(struct page *page)
> > +{
> > + if (WARN_ON(!movable_balloon_page(page)))
>
> Looks like this actually can happen if the page is leaked
> between previous movable_balloon_page and here.
>
> > + return false;
Yes, it surely can happen, and it does not harm to catch it here, print a warn and
return. While testing it, I wasn't lucky to see this small window opening, though.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-14 18:22 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: <20120813084123.GF14081@redhat.com>
On Mon, Aug 13, 2012 at 11:41:23AM +0300, Michael S. Tsirkin wrote:
> > @@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > set_page_pfns(vb->pfns + vb->num_pfns, page);
> > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > totalram_pages--;
> > + spin_lock(&pages_lock);
> > list_add(&page->lru, &vb->pages);
>
> If list_add above is reordered with mapping assignment below,
> then nothing bad happens because balloon_mapping takes
> pages_lock.
>
> > + page->mapping = balloon_mapping;
> > + spin_unlock(&pages_lock);
> > }
> >
> > /* Didn't get any? Oh well. */
> > @@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > return;
> >
> > tell_host(vb, vb->inflate_vq);
> > + mutex_unlock(&balloon_lock);
> > }
> >
> > static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> > @@ -169,10 +183,22 @@ 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(&balloon_lock);
> > for (vb->num_pfns = 0; vb->num_pfns < num;
> > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > + /*
> > + * We can race against virtballoon_isolatepage() and end up
> > + * stumbling across a _temporarily_ empty 'pages' list.
> > + */
> > + spin_lock(&pages_lock);
> > + if (unlikely(list_empty(&vb->pages))) {
> > + spin_unlock(&pages_lock);
> > + break;
> > + }
> > page = list_first_entry(&vb->pages, struct page, lru);
> > + page->mapping = NULL;
>
> Unlike the case above, here
> if = NULL write above is reordered with list_del below,
> then isolate_page can run on a page that is not
> on lru.
>
> So I think this needs a wmb().
> And maybe a comment above explaining why it is safe?
Good point. Presumably, this nit has potential to explain your guessing on the
read barrier stuff at movable_balloon_page() on the other patch.
> > list_del(&page->lru);
>
> I wonder why changing page->lru here is safe against
> races with unmap_and_move in the previous patch.
leak_balloon() doesn't race against unmap_and_move() because the later works on
an isolated page set. So, theoretically, pages being dequeued from balloon page
list here are either migrated (already) or they were not isolated yet.
> > +/*
> > + * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
> > + * access pertinent elements from struct virtio_balloon
> > + */
>
> What if there is more than one balloon device?
Is it possible to load this driver twice, or are you foreseeing a future case
where this driver will be able to manage several distinct memory balloons for
the same guest?
> > + /* Init the ballooned page->mapping special balloon_mapping */
> > + balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
> > + if (!balloon_mapping) {
> > + err = -ENOMEM;
> > + goto out_free_vb;
> > + }
>
> Can balloon_mapping be dereferenced at this point?
> Then what happens?
Since there's no balloon page enqueued for this balloon driver yet, there's no
chance of balloon_mapping being dereferenced at this point.
> > +
> > + INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
> > + INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
> > + spin_lock_init(&balloon_mapping->tree_lock);
> > + balloon_mapping->a_ops = &virtio_balloon_aops;
> >
> > err = init_vqs(vb);
> > if (err)
> > @@ -373,6 +493,7 @@ out_del_vqs:
> > vdev->config->del_vqs(vdev);
> > out_free_vb:
> > kfree(vb);
> > + kfree(balloon_mapping);
>
> No need to set it to NULL? It seems if someone else allocates a mapping
> and gets this chunk of memory by chance, the logic in mm will get
> confused.
Good point. It surely doesn't hurt be asured of this sort of safety.
> > out:
> > return err;
> > }
> > @@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
> > kthread_stop(vb->thread);
> > remove_common(vb);
> > kfree(vb);
> > + kfree(balloon_mapping);
>
> Neither here?
ditto.
> > }
> >
> > #ifdef CONFIG_PM
> > diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
> > index 652dc8b..930f1b7 100644
> > --- a/include/linux/virtio_balloon.h
> > +++ b/include/linux/virtio_balloon.h
> > @@ -56,4 +56,8 @@ struct virtio_balloon_stat {
> > u64 val;
> > } __attribute__((packed));
> >
> > +#if !defined(CONFIG_COMPACTION)
> > +struct address_space *balloon_mapping;
> > +#endif
> > +
>
> Anyone including this header will get a different copy of
> balloon_mapping. Besides, need to be ifdef KERNEL.
Good point. I'll move this hunk to the balloon driver itself.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-14 18:44 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: <20120814083320.GA3597@redhat.com>
On Tue, Aug 14, 2012 at 11:33:20AM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 14, 2012 at 09:29:49AM +0930, Rusty Russell wrote:
> > On Mon, 13 Aug 2012 11:41:23 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Fri, Aug 10, 2012 at 02:55:15PM -0300, Rafael Aquini wrote:
> > > > +/*
> > > > + * Populate balloon_mapping->a_ops->freepage method to help compaction on
> > > > + * re-inserting an isolated page into the balloon page list.
> > > > + */
> > > > +void virtballoon_putbackpage(struct page *page)
> > > > +{
> > > > + spin_lock(&pages_lock);
> > > > + list_add(&page->lru, &vb_ptr->pages);
> > > > + spin_unlock(&pages_lock);
> > >
> > > Could the following race trigger:
> > > migration happens while module unloading is in progress,
> > > module goes away between here and when the function
> > > returns, then code for this function gets overwritten?
> > > If yes we need locking external to module to prevent this.
> > > Maybe add a spinlock to struct address_space?
> >
> > The balloon module cannot be unloaded until it has leaked all its pages,
> > so I think this is safe:
> >
> > static void remove_common(struct virtio_balloon *vb)
> > {
> > /* There might be pages left in the balloon: free them. */
> > while (vb->num_pages)
> > leak_balloon(vb, vb->num_pages);
> >
> > Cheers,
> > Rusty.
>
> I know I meant something else.
> Let me lay this out:
>
> CPU1 executes:
> void virtballoon_putbackpage(struct page *page)
> {
> spin_lock(&pages_lock);
> list_add(&page->lru, &vb_ptr->pages);
> spin_unlock(&pages_lock);
>
>
> at this point CPU2 unloads module:
> leak_balloon
> ......
>
> next CPU2 loads another module so code memory gets overwritten
>
> now CPU1 executes the next instruction:
>
> }
>
> which would normally return to function's caller,
> but it has been overwritten by CPU2 so we get corruption.
>
> No?
At the point CPU2 is unloading the module, it will be kept looping at the
snippet Rusty pointed out because the isolation / migration steps do not mess
with 'vb->num_pages'. The driver will only unload after leaking the total amount
of balloon's inflated pages, which means (for this hypothetical case) CPU2 will
wait until CPU1 finishes the putaback procedure.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 19:31 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: <20120814184409.GC13338@t510.redhat.com>
On Tue, Aug 14, 2012 at 03:44:09PM -0300, Rafael Aquini wrote:
> On Tue, Aug 14, 2012 at 11:33:20AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 14, 2012 at 09:29:49AM +0930, Rusty Russell wrote:
> > > On Mon, 13 Aug 2012 11:41:23 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Fri, Aug 10, 2012 at 02:55:15PM -0300, Rafael Aquini wrote:
> > > > > +/*
> > > > > + * Populate balloon_mapping->a_ops->freepage method to help compaction on
> > > > > + * re-inserting an isolated page into the balloon page list.
> > > > > + */
> > > > > +void virtballoon_putbackpage(struct page *page)
> > > > > +{
> > > > > + spin_lock(&pages_lock);
> > > > > + list_add(&page->lru, &vb_ptr->pages);
> > > > > + spin_unlock(&pages_lock);
> > > >
> > > > Could the following race trigger:
> > > > migration happens while module unloading is in progress,
> > > > module goes away between here and when the function
> > > > returns, then code for this function gets overwritten?
> > > > If yes we need locking external to module to prevent this.
> > > > Maybe add a spinlock to struct address_space?
> > >
> > > The balloon module cannot be unloaded until it has leaked all its pages,
> > > so I think this is safe:
> > >
> > > static void remove_common(struct virtio_balloon *vb)
> > > {
> > > /* There might be pages left in the balloon: free them. */
> > > while (vb->num_pages)
> > > leak_balloon(vb, vb->num_pages);
> > >
> > > Cheers,
> > > Rusty.
> >
> > I know I meant something else.
> > Let me lay this out:
> >
> > CPU1 executes:
> > void virtballoon_putbackpage(struct page *page)
> > {
> > spin_lock(&pages_lock);
> > list_add(&page->lru, &vb_ptr->pages);
> > spin_unlock(&pages_lock);
> >
> >
> > at this point CPU2 unloads module:
> > leak_balloon
> > ......
> >
> > next CPU2 loads another module so code memory gets overwritten
> >
> > now CPU1 executes the next instruction:
> >
> > }
> >
> > which would normally return to function's caller,
> > but it has been overwritten by CPU2 so we get corruption.
> >
> > No?
>
> At the point CPU2 is unloading the module, it will be kept looping at the
> snippet Rusty pointed out because the isolation / migration steps do not mess
> with 'vb->num_pages'. The driver will only unload after leaking the total amount
> of balloon's inflated pages, which means (for this hypothetical case) CPU2 will
> wait until CPU1 finishes the putaback procedure.
>
Yes but only until unlock finishes. The last return from function
is not guarded and can be overwritten.
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Michael S. Tsirkin @ 2012-08-14 19:35 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: <20120814174404.GA13338@t510.redhat.com>
On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote:
> On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote:
> > > +static inline bool movable_balloon_page(struct page *page)
> > > +{
> > > + return (page->mapping && page->mapping == balloon_mapping);
> >
> > I am guessing this needs smp_read_barrier_depends, and maybe
> > ACCESS_ONCE ...
> >
>
> I'm curious about your guessing here. Could you ellaborate it further, please?
>
>
> > > +#else
> > > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > > +static inline void putback_balloon_page(struct page *page) { return false; }
> > > +static inline bool movable_balloon_page(struct page *page) { return false; }
> > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
> > > +
> >
> > This does mean that only one type of balloon is useable at a time.
> > I wonder whether using a flag in address_space structure instead
> > is possible ...
>
> This means we are only introducing this feature for virtio_balloon by now.
> Despite the flagging address_space stuff is something we surely can look in the
> future, I quite didn't get how we could be using two different types of balloon
> devices at the same time for the same system. Could you ellaborate it a little
> more, please?
>
E.g. kvm can emulate hyperv so it could in theory have hyperv balloon.
This is mm stuff it is best not to tie it to specific drivers.
> > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > +bool isolate_balloon_page(struct page *page)
> > > +{
> > > + if (WARN_ON(!movable_balloon_page(page)))
> >
> > Looks like this actually can happen if the page is leaked
> > between previous movable_balloon_page and here.
> >
> > > + return false;
>
> Yes, it surely can happen, and it does not harm to catch it here, print a warn and
> return.
If it is legal, why warn? For that matter why test here at all?
> While testing it, I wasn't lucky to see this small window opening, though.
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Michael S. Tsirkin @ 2012-08-14 19:48 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: <20120814193525.GB28840@redhat.com>
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote:
> > On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote:
> > > > +static inline bool movable_balloon_page(struct page *page)
> > > > +{
> > > > + return (page->mapping && page->mapping == balloon_mapping);
> > >
> > > I am guessing this needs smp_read_barrier_depends, and maybe
> > > ACCESS_ONCE ...
> > >
> >
> > I'm curious about your guessing here. Could you ellaborate it further, please?
> >
> >
> > > > +#else
> > > > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > > > +static inline void putback_balloon_page(struct page *page) { return false; }
> > > > +static inline bool movable_balloon_page(struct page *page) { return false; }
> > > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
> > > > +
> > >
> > > This does mean that only one type of balloon is useable at a time.
> > > I wonder whether using a flag in address_space structure instead
> > > is possible ...
> >
> > This means we are only introducing this feature for virtio_balloon by now.
> > Despite the flagging address_space stuff is something we surely can look in the
> > future, I quite didn't get how we could be using two different types of balloon
> > devices at the same time for the same system. Could you ellaborate it a little
> > more, please?
> >
>
> E.g. kvm can emulate hyperv so it could in theory have hyperv balloon.
> This is mm stuff it is best not to tie it to specific drivers.
But of course I agree this is not top priority, no need
to block submission on this, just nice to have.
> > > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > > +bool isolate_balloon_page(struct page *page)
> > > > +{
> > > > + if (WARN_ON(!movable_balloon_page(page)))
> > >
> > > Looks like this actually can happen if the page is leaked
> > > between previous movable_balloon_page and here.
> > >
> > > > + return false;
> >
> > Yes, it surely can happen, and it does not harm to catch it here, print a warn and
> > return.
>
> If it is legal, why warn? For that matter why test here at all?
>
> > While testing it, I wasn't lucky to see this small window opening, though.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 19:51 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: <20120814182244.GB13338@t510.redhat.com>
On Tue, Aug 14, 2012 at 03:22:45PM -0300, Rafael Aquini wrote:
> On Mon, Aug 13, 2012 at 11:41:23AM +0300, Michael S. Tsirkin wrote:
> > > @@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > > set_page_pfns(vb->pfns + vb->num_pfns, page);
> > > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > totalram_pages--;
> > > + spin_lock(&pages_lock);
> > > list_add(&page->lru, &vb->pages);
> >
> > If list_add above is reordered with mapping assignment below,
> > then nothing bad happens because balloon_mapping takes
> > pages_lock.
> >
> > > + page->mapping = balloon_mapping;
> > > + spin_unlock(&pages_lock);
> > > }
> > >
> > > /* Didn't get any? Oh well. */
> > > @@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > > return;
> > >
> > > tell_host(vb, vb->inflate_vq);
> > > + mutex_unlock(&balloon_lock);
> > > }
> > >
> > > static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> > > @@ -169,10 +183,22 @@ 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(&balloon_lock);
> > > for (vb->num_pfns = 0; vb->num_pfns < num;
> > > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > > + /*
> > > + * We can race against virtballoon_isolatepage() and end up
> > > + * stumbling across a _temporarily_ empty 'pages' list.
> > > + */
> > > + spin_lock(&pages_lock);
> > > + if (unlikely(list_empty(&vb->pages))) {
> > > + spin_unlock(&pages_lock);
> > > + break;
> > > + }
> > > page = list_first_entry(&vb->pages, struct page, lru);
> > > + page->mapping = NULL;
> >
> > Unlike the case above, here
> > if = NULL write above is reordered with list_del below,
> > then isolate_page can run on a page that is not
> > on lru.
> >
> > So I think this needs a wmb().
> > And maybe a comment above explaining why it is safe?
>
> Good point. Presumably, this nit has potential to explain your guessing on the
> read barrier stuff at movable_balloon_page() on the other patch.
>
What I think you should do is use rcu for access.
And here sync rcu before freeing.
Maybe an overkill but at least a documented synchronization
primitive, and it is very light weight.
> > > list_del(&page->lru);
> >
> > I wonder why changing page->lru here is safe against
> > races with unmap_and_move in the previous patch.
>
> leak_balloon() doesn't race against unmap_and_move() because the later works on
> an isolated page set. So, theoretically, pages being dequeued from balloon page
> list here are either migrated (already) or they were not isolated yet.
>
So add a comment explaining why it is safe pls.
> > > +/*
> > > + * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
> > > + * access pertinent elements from struct virtio_balloon
> > > + */
> >
> > What if there is more than one balloon device?
>
> Is it possible to load this driver twice, or are you foreseeing a future case
> where this driver will be able to manage several distinct memory balloons for
> the same guest?
>
Second.
It is easy to create several balloons they are just
pci devices.
> > > + /* Init the ballooned page->mapping special balloon_mapping */
> > > + balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
> > > + if (!balloon_mapping) {
> > > + err = -ENOMEM;
> > > + goto out_free_vb;
> > > + }
> >
> > Can balloon_mapping be dereferenced at this point?
> > Then what happens?
>
> Since there's no balloon page enqueued for this balloon driver yet, there's no
> chance of balloon_mapping being dereferenced at this point.
>
>
> > > +
> > > + INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
> > > + INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
> > > + spin_lock_init(&balloon_mapping->tree_lock);
> > > + balloon_mapping->a_ops = &virtio_balloon_aops;
> > >
> > > err = init_vqs(vb);
> > > if (err)
> > > @@ -373,6 +493,7 @@ out_del_vqs:
> > > vdev->config->del_vqs(vdev);
> > > out_free_vb:
> > > kfree(vb);
> > > + kfree(balloon_mapping);
> >
> > No need to set it to NULL? It seems if someone else allocates a mapping
> > and gets this chunk of memory by chance, the logic in mm will get
> > confused.
>
> Good point. It surely doesn't hurt be asured of this sort of safety.
>
> > > out:
> > > return err;
> > > }
> > > @@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
> > > kthread_stop(vb->thread);
> > > remove_common(vb);
> > > kfree(vb);
> > > + kfree(balloon_mapping);
> >
> > Neither here?
>
> ditto.
>
>
> > > }
> > >
> > > #ifdef CONFIG_PM
> > > diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
> > > index 652dc8b..930f1b7 100644
> > > --- a/include/linux/virtio_balloon.h
> > > +++ b/include/linux/virtio_balloon.h
> > > @@ -56,4 +56,8 @@ struct virtio_balloon_stat {
> > > u64 val;
> > > } __attribute__((packed));
> > >
> > > +#if !defined(CONFIG_COMPACTION)
> > > +struct address_space *balloon_mapping;
> > > +#endif
> > > +
> >
> > Anyone including this header will get a different copy of
> > balloon_mapping. Besides, need to be ifdef KERNEL.
>
> Good point. I'll move this hunk to the balloon driver itself.
>
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Michael S. Tsirkin @ 2012-08-14 19:52 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: <20120814174404.GA13338@t510.redhat.com>
On Tue, Aug 14, 2012 at 02:44:05PM -0300, Rafael Aquini wrote:
> On Mon, Aug 13, 2012 at 11:26:19AM +0300, Michael S. Tsirkin wrote:
> > > +static inline bool movable_balloon_page(struct page *page)
> > > +{
> > > + return (page->mapping && page->mapping == balloon_mapping);
> >
> > I am guessing this needs smp_read_barrier_depends, and maybe
> > ACCESS_ONCE ...
> >
>
> I'm curious about your guessing here. Could you ellaborate it further, please?
well balloon_mapping can change dynamically.
I think actually rcu is a good fit here.
>
> > > +#else
> > > +static inline bool isolate_balloon_page(struct page *page) { return false; }
> > > +static inline void putback_balloon_page(struct page *page) { return false; }
> > > +static inline bool movable_balloon_page(struct page *page) { return false; }
> > > +#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && CONFIG_COMPACTION */
> > > +
> >
> > This does mean that only one type of balloon is useable at a time.
> > I wonder whether using a flag in address_space structure instead
> > is possible ...
>
> This means we are only introducing this feature for virtio_balloon by now.
> Despite the flagging address_space stuff is something we surely can look in the
> future, I quite didn't get how we could be using two different types of balloon
> devices at the same time for the same system. Could you ellaborate it a little
> more, please?
>
>
> > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > +bool isolate_balloon_page(struct page *page)
> > > +{
> > > + if (WARN_ON(!movable_balloon_page(page)))
> >
> > Looks like this actually can happen if the page is leaked
> > between previous movable_balloon_page and here.
> >
> > > + return false;
>
> Yes, it surely can happen, and it does not harm to catch it here, print a warn and
> return. While testing it, I wasn't lucky to see this small window opening, though.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 19:59 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: <20120814195139.GA28870@redhat.com>
On Tue, Aug 14, 2012 at 10:51:39PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 14, 2012 at 03:22:45PM -0300, Rafael Aquini wrote:
> > On Mon, Aug 13, 2012 at 11:41:23AM +0300, Michael S. Tsirkin wrote:
> > > > @@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > > > set_page_pfns(vb->pfns + vb->num_pfns, page);
> > > > vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > > totalram_pages--;
> > > > + spin_lock(&pages_lock);
> > > > list_add(&page->lru, &vb->pages);
> > >
> > > If list_add above is reordered with mapping assignment below,
> > > then nothing bad happens because balloon_mapping takes
> > > pages_lock.
> > >
> > > > + page->mapping = balloon_mapping;
> > > > + spin_unlock(&pages_lock);
> > > > }
> > > >
> > > > /* Didn't get any? Oh well. */
> > > > @@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> > > > return;
> > > >
> > > > tell_host(vb, vb->inflate_vq);
> > > > + mutex_unlock(&balloon_lock);
> > > > }
> > > >
> > > > static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> > > > @@ -169,10 +183,22 @@ 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(&balloon_lock);
> > > > for (vb->num_pfns = 0; vb->num_pfns < num;
> > > > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > > > + /*
> > > > + * We can race against virtballoon_isolatepage() and end up
> > > > + * stumbling across a _temporarily_ empty 'pages' list.
> > > > + */
> > > > + spin_lock(&pages_lock);
> > > > + if (unlikely(list_empty(&vb->pages))) {
> > > > + spin_unlock(&pages_lock);
> > > > + break;
> > > > + }
> > > > page = list_first_entry(&vb->pages, struct page, lru);
> > > > + page->mapping = NULL;
> > >
> > > Unlike the case above, here
> > > if = NULL write above is reordered with list_del below,
> > > then isolate_page can run on a page that is not
> > > on lru.
> > >
> > > So I think this needs a wmb().
> > > And maybe a comment above explaining why it is safe?
> >
> > Good point. Presumably, this nit has potential to explain your guessing on the
> > read barrier stuff at movable_balloon_page() on the other patch.
> >
>
>
> What I think you should do is use rcu for access.
> And here sync rcu before freeing.
> Maybe an overkill but at least a documented synchronization
> primitive, and it is very light weight.
>
> > > > list_del(&page->lru);
> > >
> > > I wonder why changing page->lru here is safe against
> > > races with unmap_and_move in the previous patch.
> >
> > leak_balloon() doesn't race against unmap_and_move() because the later works on
> > an isolated page set. So, theoretically, pages being dequeued from balloon page
> > list here are either migrated (already) or they were not isolated yet.
> >
>
> So add a comment explaining why it is safe pls.
>
> > > > +/*
> > > > + * '*vb_ptr' allows virtballoon_migratepage() & virtballoon_putbackpage() to
> > > > + * access pertinent elements from struct virtio_balloon
> > > > + */
> > >
> > > What if there is more than one balloon device?
> >
> > Is it possible to load this driver twice, or are you foreseeing a future case
> > where this driver will be able to manage several distinct memory balloons for
> > the same guest?
> >
>
> Second.
> It is easy to create several balloons they are just
> pci devices.
and it might not be too important to make it work but
at least would be nice not to have a crash in this
setup.
> > > > + /* Init the ballooned page->mapping special balloon_mapping */
> > > > + balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
> > > > + if (!balloon_mapping) {
> > > > + err = -ENOMEM;
> > > > + goto out_free_vb;
> > > > + }
> > >
> > > Can balloon_mapping be dereferenced at this point?
> > > Then what happens?
> >
> > Since there's no balloon page enqueued for this balloon driver yet, there's no
> > chance of balloon_mapping being dereferenced at this point.
> >
> >
> > > > +
> > > > + INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
> > > > + INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
> > > > + spin_lock_init(&balloon_mapping->tree_lock);
> > > > + balloon_mapping->a_ops = &virtio_balloon_aops;
> > > >
> > > > err = init_vqs(vb);
> > > > if (err)
> > > > @@ -373,6 +493,7 @@ out_del_vqs:
> > > > vdev->config->del_vqs(vdev);
> > > > out_free_vb:
> > > > kfree(vb);
> > > > + kfree(balloon_mapping);
> > >
> > > No need to set it to NULL? It seems if someone else allocates a mapping
> > > and gets this chunk of memory by chance, the logic in mm will get
> > > confused.
> >
> > Good point. It surely doesn't hurt be asured of this sort of safety.
> >
> > > > out:
> > > > return err;
> > > > }
> > > > @@ -397,6 +518,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
> > > > kthread_stop(vb->thread);
> > > > remove_common(vb);
> > > > kfree(vb);
> > > > + kfree(balloon_mapping);
> > >
> > > Neither here?
> >
> > ditto.
> >
> >
> > > > }
> > > >
> > > > #ifdef CONFIG_PM
> > > > diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
> > > > index 652dc8b..930f1b7 100644
> > > > --- a/include/linux/virtio_balloon.h
> > > > +++ b/include/linux/virtio_balloon.h
> > > > @@ -56,4 +56,8 @@ struct virtio_balloon_stat {
> > > > u64 val;
> > > > } __attribute__((packed));
> > > >
> > > > +#if !defined(CONFIG_COMPACTION)
> > > > +struct address_space *balloon_mapping;
> > > > +#endif
> > > > +
> > >
> > > Anyone including this header will get a different copy of
> > > balloon_mapping. Besides, need to be ifdef KERNEL.
> >
> > Good point. I'll move this hunk to the balloon driver itself.
> >
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-08-14 20:00 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: <20120814193525.GB28840@redhat.com>
On Tue, Aug 14, 2012 at 10:35:25PM +0300, Michael S. Tsirkin wrote:
> > > > +/* __isolate_lru_page() counterpart for a ballooned page */
> > > > +bool isolate_balloon_page(struct page *page)
> > > > +{
> > > > + if (WARN_ON(!movable_balloon_page(page)))
> > >
> > > Looks like this actually can happen if the page is leaked
> > > between previous movable_balloon_page and here.
> > >
> > > > + return false;
> >
> > Yes, it surely can happen, and it does not harm to catch it here, print a warn and
> > return.
>
> If it is legal, why warn? For that matter why test here at all?
>
As this is a public symbol, and despite the usage we introduce is sane, the warn
was placed as an insurance policy to let us know about any insane attempt to use
the procedure in the future. That was due to a nice review nitpick, actually.
Even though the code already had a test to properly avoid this race you
mention, I thought that sustaining the warn was a good thing. As I told you,
despite real, I've never got (un)lucky enough to stumble across that race window
while testing the patch.
If your concern is about being too much verbose on logging, under certain
conditions, perhaps we can change that test to a WARN_ON_ONCE() ?
Mel, what are your thoughts here?
> > While testing it, I wasn't lucky to see this small window opening, though.
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-08-14 20:03 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: <20120814194837.GA28863@redhat.com>
On Tue, Aug 14, 2012 at 10:48:37PM +0300, Michael S. Tsirkin wrote:
> >
> > E.g. kvm can emulate hyperv so it could in theory have hyperv balloon.
> > This is mm stuff it is best not to tie it to specific drivers.
>
> But of course I agree this is not top priority, no need
> to block submission on this, just nice to have.
>
This surely is interesting to look at, in the near future.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-14 20:08 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: <20120814195916.GC28870@redhat.com>
On Tue, Aug 14, 2012 at 10:59:16PM +0300, Michael S. Tsirkin wrote:
> > > > What if there is more than one balloon device?
> > >
> > > Is it possible to load this driver twice, or are you foreseeing a future case
> > > where this driver will be able to manage several distinct memory balloons for
> > > the same guest?
> > >
> >
> > Second.
> > It is easy to create several balloons they are just
> > pci devices.
>
>
>
> and it might not be too important to make it work but
> at least would be nice not to have a crash in this
> setup.
>
Fair enough. For now, as I believe it's safe to assume we are only inflating one
balloon per guest, I'd like to propose this as a future enhancement. Sounds
good?
^ permalink raw reply
* Re: [RFC-v2 1/6] msix: Work-around for vhost-scsi with KVM in-kernel MSI injection
From: Nicholas A. Bellinger @ 2012-08-14 20:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, Stefan Hajnoczi, kvm-devel, Jan Kiszka,
qemu-devel, Zhi Yong Wu, Anthony Liguori, target-devel,
Paolo Bonzini, lf-virt, Christoph Hellwig
In-Reply-To: <20120813181705.GC19460@redhat.com>
On Mon, 2012-08-13 at 21:17 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2012 at 08:06:17PM +0200, Jan Kiszka wrote:
> > On 2012-08-13 20:03, Michael S. Tsirkin wrote:
> > > On Mon, Aug 13, 2012 at 02:06:10PM +0200, Jan Kiszka wrote:
> > >> On 2012-08-13 10:35, Nicholas A. Bellinger wrote:
> > >>> From: Nicholas Bellinger <nab@linux-iscsi.org>
> > >>>
> > >>> This is required to get past the following assert with:
> > >>>
> > >>> commit 1523ed9e1d46b0b54540049d491475ccac7e6421
> > >>> Author: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Date: Thu May 17 10:32:39 2012 -0300
> > >>>
> > >>> virtio/vhost: Add support for KVM in-kernel MSI injection
> > >>>
> > >>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > >>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> > >>> Cc: Anthony Liguori <aliguori@us.ibm.com>
> > >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > >>> ---
> > >>> hw/msix.c | 3 +++
> > >>> 1 files changed, 3 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/hw/msix.c b/hw/msix.c
> > >>> index 800fc32..c1e6dc3 100644
> > >>> --- a/hw/msix.c
> > >>> +++ b/hw/msix.c
> > >>> @@ -544,6 +544,9 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
> > >>> {
> > >>> int vector;
> > >>>
> > >>> + if (!dev->msix_vector_use_notifier && !dev->msix_vector_release_notifier)
> > >>> + return;
> > >>> +
> > >>> assert(dev->msix_vector_use_notifier &&
> > >>> dev->msix_vector_release_notifier);
> > >>>
> > >>>
> > >>
> > >> I think to remember pointing out that there is a bug somewhere in the
> > >> reset code which deactivates a non-active vhost instance, no?
> > >>
> > >> Jan
> > >
> > > Could not find it. Could you dig it up pls?
> >
> > http://thread.gmane.org/gmane.linux.scsi.target.devel/2277/focus=2309
> >
> > Jan
>
> Ah yes. So let's not work around, need to get to the bottom of that.
>
Ok, so the assert being triggered in msix_unset_vector_notifiers()
appears to have been a side effect of the memory corruption bug in
virtio-scsi fixed in Patch #6, and is no longer required to start
vhost-scsi with the bugfix in place.
That said, dropping this patch for RFC-v3..
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-14 20:11 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: <20120814195139.GA28870@redhat.com>
On Tue, Aug 14, 2012 at 10:51:39PM +0300, Michael S. Tsirkin wrote:
> What I think you should do is use rcu for access.
> And here sync rcu before freeing.
> Maybe an overkill but at least a documented synchronization
> primitive, and it is very light weight.
>
I liked your suggestion on barriers, as well.
Rik, Mel ?
^ permalink raw reply
* Re: [RFC-v2 6/6] virtio-scsi: Fix incorrect VirtIOSCSI->cmd_vqs[0] definition
From: Nicholas A. Bellinger @ 2012-08-14 20:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, kvm-devel, Jan Kiszka, qemu-devel, Zhi Yong Wu,
Anthony Liguori, target-devel, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <20120813090259.GJ14081@redhat.com>
On Mon, 2012-08-13 at 12:02 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2012 at 08:35:17AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch fixes bug in the definition of VirtIOSCSI->cmd_vqs[0],
> > where the return of virtio_add_queue() in virtio_scsi_init() ends up
> > overwriting past the end of ->cmd_vqs[0].
> >
> > Since virtio_scsi currently assumes a single vqs for data, this patch
> > simply changes ->cmd_vqs[1] to handle the single VirtQueue.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This is a bugfix we need even without vhost, right?
>
I believe so, as it appears to be stomping past the end of memory for
every virtio-scsi initialization regardless of vhost usage..
Paolo, can you pickup this fix now for stable so it can be dropped from
RFC-v3..?
--nab
> > ---
> > hw/virtio-scsi.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
> > index 5e2ff6b..2c70f89 100644
> > --- a/hw/virtio-scsi.c
> > +++ b/hw/virtio-scsi.c
> > @@ -150,7 +150,7 @@ typedef struct {
> > bool events_dropped;
> > VirtQueue *ctrl_vq;
> > VirtQueue *event_vq;
> > - VirtQueue *cmd_vqs[0];
> > + VirtQueue *cmd_vqs[1];
> >
> > bool vhost_started;
> > VHostSCSI *vhost_scsi;
> > --
> > 1.7.2.5
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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