* 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
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Michael S. Tsirkin @ 2012-08-14 20:23 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: <20120814200043.GB22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote:
> 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.
think about it: you see it in log. so you know race triggered.
now what? why is it useful to know this?
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 20:24 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: <20120814200830.GD22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 05:08:31PM -0300, Rafael Aquini wrote:
> 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?
>
Since guest crashes when it's not the case, no it doesn't, sorry :(.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-14 20:29 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: <20120814202401.GB28990@redhat.com>
On Tue, Aug 14, 2012 at 11:24:01PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 14, 2012 at 05:08:31PM -0300, Rafael Aquini wrote:
> > 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?
> >
>
> Since guest crashes when it's not the case, no it doesn't, sorry :(.
>
Ok, but right now this driver only takes care of 1 balloon per guest, so how
could this approach crash it?
Your point is a good thing to be on a to-do list for future enhancements, but
it's not a dealbreaker for the present balloon driver implementation, IMHO.
^ permalink raw reply
* Re: [RFC-v2 3/6] vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
From: Nicholas A. Bellinger @ 2012-08-14 20:31 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: <20120813085325.GH14081@redhat.com>
On Mon, 2012-08-13 at 11:53 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2012 at 08:35:14AM +0000, Nicholas A. Bellinger wrote:
> > 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.
<SNIP>
> > +static VHostSCSI *vhost_scsi_add(const char *id, const char *wwpn,
> > + uint16_t tpgt)
> > +{
> > + VHostSCSI *vs = g_malloc0(sizeof(*vs));
> > + int ret;
> > +
> > + /* TODO set up vhost-scsi device and bind to tcm_vhost/$wwpm/tpgt_$tpgt */
> > + fprintf(stderr, "wwpn = \"%s\" tpgt = \"%u\"\n", id, tpgt);
> > +
> > + ret = vhost_dev_init(&vs->dev, -1, "/dev/vhost-scsi", false);
>
> This -1 is a hack. You need to support passing in fd from
> the monitor, and pass it here.
>
Mmm, looking at how vhost_net_init + tap.c does this, but am not quite
what fd needs to be propagated up for virtio-scsi -> vhost-scsi..
Can you please elaborate on this one a bit more..?
--nab
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 20: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: <20120814201113.GE22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> 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 ?
Further instead of simple assignment I would add an api
in mm to set/clear the balloon mapping, with proper locking.
This could fail if already set, and thus fix crash with
many ballons.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 20:49 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: <20120814202949.GF22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 05:29:50PM -0300, Rafael Aquini wrote:
> On Tue, Aug 14, 2012 at 11:24:01PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 14, 2012 at 05:08:31PM -0300, Rafael Aquini wrote:
> > > 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?
> > >
> >
> > Since guest crashes when it's not the case, no it doesn't, sorry :(.
> >
> Ok, but right now this driver only takes care of 1 balloon per guest,
It does? Are you sure? There is no global state as far as I can see. So
I can create 2 devices and driver will happily create two instances,
each one can be inflated/deflated independently.
> so how
> could this approach crash it?
Add device. inflate. Add another device. inflate. deflate. unplug.
Now you have pointer to freed memory and when mm touches
page from first device, you ge use after free.
> Your point is a good thing to be on a to-do list for future enhancements, but
> it's not a dealbreaker for the present balloon driver implementation, IMHO.
>
Yes it looks like a dealbreaker to me.
--
MST
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 20:54 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: <20120814204906.GD28990@redhat.com>
On Tue, Aug 14, 2012 at 11:49:06PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 14, 2012 at 05:29:50PM -0300, Rafael Aquini wrote:
> > On Tue, Aug 14, 2012 at 11:24:01PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Aug 14, 2012 at 05:08:31PM -0300, Rafael Aquini wrote:
> > > > 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?
> > > >
> > >
> > > Since guest crashes when it's not the case, no it doesn't, sorry :(.
> > >
> > Ok, but right now this driver only takes care of 1 balloon per guest,
>
> It does? Are you sure? There is no global state as far as I can see. So
> I can create 2 devices and driver will happily create two instances,
> each one can be inflated/deflated independently.
>
> > so how
> > could this approach crash it?
>
> Add device. inflate. Add another device. inflate. deflate. unplug.
> Now you have pointer to freed memory and when mm touches
> page from first device, you ge use after free.
>
> > Your point is a good thing to be on a to-do list for future enhancements, but
> > it's not a dealbreaker for the present balloon driver implementation, IMHO.
> >
>
> Yes it looks like a dealbreaker to me.
To clarify, the global state that this patch adds, is ugly
even if we didn't support multiple balloons yet.
So I don't think I can accept such a patch.
Rusty has a final word here, maybe he thinks differently.
> --
> MST
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rik van Riel @ 2012-08-14 20:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120814205426.GA29162@redhat.com>
On 08/14/2012 04:54 PM, Michael S. Tsirkin wrote:
> To clarify, the global state that this patch adds, is ugly
> even if we didn't support multiple balloons yet.
> So I don't think I can accept such a patch.
> Rusty has a final word here, maybe he thinks differently.
Before deciding that "does not support multiple balloon drivers
at once" is an issue, is there any use case at all for having
multiple balloon drivers active at a time?
I do not see any.
--
All rights reversed
^ permalink raw reply
* Re: [RFC-v2 3/6] vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
From: Nicholas A. Bellinger @ 2012-08-14 21:12 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: <20120813085929.GI14081@redhat.com>
On Mon, 2012-08-13 at 11:59 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2012 at 08:35:14AM +0000, Nicholas A. Bellinger wrote:
> > 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 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>
>
>
> Sent mail too fast, sorry. More comments below.
>
> > ---
> > configure | 10 +++
> > hw/Makefile.objs | 1 +
> > hw/qdev-properties.c | 40 ++++++++++++
> > hw/qdev.h | 3 +
> > hw/vhost-scsi.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/vhost-scsi.h | 50 +++++++++++++++
> > qemu-common.h | 1 +
> > qemu-config.c | 16 +++++
> > qemu-options.hx | 4 +
> > vl.c | 18 +++++
> > 10 files changed, 313 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..0266266 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,45 @@ 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! */
>
> Why does this make sense in the generic qdev-properties?
> There's exactly one device that can use this, no?
>
Mmmm, not sure on this one either.. Stefan..?
> > diff --git a/hw/qdev.h b/hw/qdev.h
> > index d699194..d5873bb 100644
> > --- a/hw/qdev.h
> > +++ b/hw/qdev.h
> > @@ -238,6 +238,7 @@ extern PropertyInfo qdev_prop_vlan;
> > extern PropertyInfo qdev_prop_pci_devfn;
> > extern PropertyInfo qdev_prop_blocksize;
> > extern PropertyInfo qdev_prop_pci_host_devaddr;
> > +extern PropertyInfo qdev_prop_vhost_scsi;
> >
> > #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
> > .name = (_name), \
> > @@ -305,6 +306,8 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
> > DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
> > #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> > DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> > +#define DEFINE_PROP_VHOST_SCSI(_n, _s, _f) \
> > + DEFINE_PROP(_n, _s, _f, qdev_prop_vhost_scsi, VHostSCSI*)
> >
>
> Can this move to vhost-scsi.c?
>
Done
> > #define DEFINE_PROP_END_OF_LIST() \
> > {}
> > diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
> > new file mode 100644
> > index 0000000..7145b2d
> > --- /dev/null
> > +++ b/hw/vhost-scsi.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * 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.
> > + *
> > + */
> > +
> > +#include <sys/ioctl.h>
> > +#include "config.h"
> > +#include "qemu-queue.h"
> > +#include "vhost-scsi.h"
> > +#include "vhost.h"
> > +
> > +struct VHostSCSI {
> > + const char *id;
> > + const char *wwpn;
> > + uint16_t tpgt;
> > + struct vhost_dev dev;
> > + struct vhost_virtqueue vqs[3];
>
> Could you add enum for vq numbers pls?
>
Done
> > + 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) == 0) {
>
> !strcmp
>
Done
> > + 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 = 3;
> > + 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;
> > + }
> > +
> > + memset(&backend, 0, sizeof(backend));
> > + 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) {
> > + fprintf(stderr, "The running tcm_vhost kernel abi_version: %d is greater"
> > + " than vhost_scsi userspace supports: %d\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);
> > +
> > + pstrcpy((char *)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;
> > +
> > + pstrcpy((char *)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) {
> > + fprintf(stderr, "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)
> > +{
> > + VHostSCSI *vs = g_malloc0(sizeof(*vs));
> > + int ret;
> > +
> > + /* TODO set up vhost-scsi device and bind to tcm_vhost/$wwpm/tpgt_$tpgt */
> > + fprintf(stderr, "wwpn = \"%s\" tpgt = \"%u\"\n", id, tpgt);
> > +
>
> Please do not keep debugging fprintfs around.
>
Dropped
> > + ret = vhost_dev_init(&vs->dev, -1, "/dev/vhost-scsi", false);
>
> commented on this separately
>
...
> > + if (ret < 0) {
> > + fprintf(stderr, "vhost-scsi: vhost initialization failed: %s\n",
> > + strerror(-ret));
>
> errors should go to monitor, here and elsewhere.
>
I think this means using monitor_printf() right..?
Looking at that now..
^ permalink raw reply
* Re: [Qemu-devel] [RFC-v2 3/6] vhost-scsi: add -vhost-scsi host device for use with tcm-vhost
From: Nicholas A. Bellinger @ 2012-08-14 21:17 UTC (permalink / raw)
To: Blue Swirl
Cc: Anthony Liguori, Stefan Hajnoczi, kvm-devel, Michael S. Tsirkin,
Jan Kiszka, qemu-devel, Zhi Yong Wu, Anthony Liguori,
target-devel, Paolo Bonzini, lf-virt, Christoph Hellwig
In-Reply-To: <CAAu8pHttCSW9bUtrdiDG8-PBCpVvW2O=PEdYQoceDDUYydZ+9Q@mail.gmail.com>
On Mon, 2012-08-13 at 19:47 +0000, Blue Swirl wrote:
> On Mon, Aug 13, 2012 at 8:35 AM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > 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 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 | 40 ++++++++++++
> > hw/qdev.h | 3 +
> > hw/vhost-scsi.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/vhost-scsi.h | 50 +++++++++++++++
> > qemu-common.h | 1 +
> > qemu-config.c | 16 +++++
> > qemu-options.hx | 4 +
> > vl.c | 18 +++++
> > 10 files changed, 313 insertions(+), 0 deletions(-)
> > create mode 100644 hw/vhost-scsi.c
> > create mode 100644 hw/vhost-scsi.h
> >
<SNIP>
> >
> > +/* --- 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;
>
> Braces, please.
>
Fixed
> > +
> > + *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/qdev.h b/hw/qdev.h
> > index d699194..d5873bb 100644
> > --- a/hw/qdev.h
> > +++ b/hw/qdev.h
> > @@ -238,6 +238,7 @@ extern PropertyInfo qdev_prop_vlan;
> > extern PropertyInfo qdev_prop_pci_devfn;
> > extern PropertyInfo qdev_prop_blocksize;
> > extern PropertyInfo qdev_prop_pci_host_devaddr;
> > +extern PropertyInfo qdev_prop_vhost_scsi;
> >
> > #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
> > .name = (_name), \
> > @@ -305,6 +306,8 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
> > DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
> > #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> > DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
> > +#define DEFINE_PROP_VHOST_SCSI(_n, _s, _f) \
> > + DEFINE_PROP(_n, _s, _f, qdev_prop_vhost_scsi, VHostSCSI*)
> >
> > #define DEFINE_PROP_END_OF_LIST() \
> > {}
> > diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
> > new file mode 100644
> > index 0000000..7145b2d
> > --- /dev/null
> > +++ b/hw/vhost-scsi.c
> > @@ -0,0 +1,170 @@
> > +/*
> > + * 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.
> > + *
> > + */
> > +
> > +#include <sys/ioctl.h>
> > +#include "config.h"
> > +#include "qemu-queue.h"
> > +#include "vhost-scsi.h"
> > +#include "vhost.h"
> > +
> > +struct VHostSCSI {
> > + const char *id;
> > + const char *wwpn;
> > + uint16_t tpgt;
> > + struct vhost_dev dev;
> > + struct vhost_virtqueue vqs[3];
> > + 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) == 0) {
> > + 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 = 3;
> > + 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;
> > + }
> > +
> > + memset(&backend, 0, sizeof(backend));
> > + 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) {
> > + fprintf(stderr, "The running tcm_vhost kernel abi_version: %d is greater"
> > + " than vhost_scsi userspace supports: %d\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);
> > +
> > + pstrcpy((char *)backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
>
> Please change vhost_wwpn to plain char *, then the cast can be removed.
>
<nod>, changed to char *, and updating tcm_vhost on the kernel side to
do the same.
> > + 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;
> > +
> > + pstrcpy((char *)backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->wwpn);
>
> Also here.
>
Done
Thanks for your review Blue!
--nab
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-14 21: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: <20120814204906.GD28990@redhat.com>
On Tue, Aug 14, 2012 at 11:49:06PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 14, 2012 at 05:29:50PM -0300, Rafael Aquini wrote:
> > On Tue, Aug 14, 2012 at 11:24:01PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Aug 14, 2012 at 05:08:31PM -0300, Rafael Aquini wrote:
> > > > 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?
> > > >
> > >
> > > Since guest crashes when it's not the case, no it doesn't, sorry :(.
> > >
> > Ok, but right now this driver only takes care of 1 balloon per guest,
>
> It does? Are you sure? There is no global state as far as I can see. So
> I can create 2 devices and driver will happily create two instances,
> each one can be inflated/deflated independently.
>
> > so how
> > could this approach crash it?
>
> Add device. inflate. Add another device. inflate. deflate. unplug.
> Now you have pointer to freed memory and when mm touches
> page from first device, you ge use after free.
>
> > Your point is a good thing to be on a to-do list for future enhancements, but
> > it's not a dealbreaker for the present balloon driver implementation, IMHO.
> >
>
> Yes it looks like a dealbreaker to me.
Sorry. You're right, I'm wrong.
I'll get back to the scracthpad to overcome this constraint. I believe the way
this patch was at its v4 revision (wrt this particular case) could possibly
address this concern of yours.
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 21:38 UTC (permalink / raw)
To: Rik van Riel
Cc: Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <502ABB9B.90108@redhat.com>
On Tue, Aug 14, 2012 at 04:56:59PM -0400, Rik van Riel wrote:
> On 08/14/2012 04:54 PM, Michael S. Tsirkin wrote:
>
> >To clarify, the global state that this patch adds, is ugly
> >even if we didn't support multiple balloons yet.
> >So I don't think I can accept such a patch.
> >Rusty has a final word here, maybe he thinks differently.
>
> Before deciding that "does not support multiple balloon drivers
> at once" is an issue, is there any use case at all for having
> multiple balloon drivers active at a time?
>
> I do not see any.
For example, we had a proposal for a page-cache backed
device. So it could be useful to have two, a regular balloon
and a pagecache backed one.
There could be other uses - it certainly looks like it
works so how can you be sure it's useless?
And even ignoring that, global pointer to a device
is an ugly hack and ugly hacks tend to explode.
And even ignoring estetics, and if we decide we are fine
with a single balloon, it needs to fail gracefully not
crash like it does now.
> --
> All rights reversed
^ permalink raw reply
* oVirt Workshop Europe 2012: Call For Participation
From: workshop-pc @ 2012-08-14 21:44 UTC (permalink / raw)
To: announce, users, engine-devel, vdsm-devel, virtualization; +Cc: workshop-pc
=================================================================
oVirt Workshop Europe 2012: Call For Participation
November 7-9, 2012 - Hotel Fira Palace - Barcelona, Spain
(All submissions must be received before midnight Sep 14th, 2012)
=================================================================
The oVirt Project is an open virtualization project for anyone who cares
about Linux-based KVM virtualization. Providing a feature-rich server
virtualization management system with advanced capabilities for hosts
and guests, including high availability, live migration, storage
management, system scheduler, and more. By open we mean open source &
open governance, done right.
During this workshop you’ll learn about the technical background and
direction of the oVirt project. You’ll meet the developers, and have an
opportunity to see and dive into the code right away. The workshop is
open to all who want to use, get involved with, or learn about the
comprehensive open virtualization management platform, oVirt. The
sessions cover the technical projects details, governance, getting
involved, usage, and much more. If you have any interest in an Open
Virtualization Management platform, this workshop is for you!
We are excited to announce that this oVirt Workshop will be held in
conjunction with the KVM Forum.
http://events.linuxfoundation.org/events/kvm-forum/
The KVM Forum and oVirt Workshop are co-located with the Linux
Foundation's 2012 LinuxCon Europe in Barcelona, Spain.
oVirt Workshop attendees will be able to attend KVM Forum sessions and
are eligible to attend LinuxCon Europe for a discounted rate.
http://events.linuxfoundation.org/events/kvm-forum/register
We invite you to lead part of the discussion by submitting a speaking
proposal for oVirt Workshop 2012.
http://events.linuxfoundation.org/cfp
Suggested topics:
- community use case/stories
- roadmaps
- deep dives into features/areas
- deep dives into code/debugging/tuning
- integration and extensions
- components: engine, vdsm, node, sdk/cli, reports, mom, guest agent, etc.
- subjects: network, storage, vm life cycle, scheduling & sla, gluster, etc.
- packaging, installation and distributions
- community infrastructure and services
SUBMISSION REQUIREMENTS
Abstracts due: Sep 14th, 2012
Notification: Sep 28th, 2012
Please submit a short abstract (~150 words) describing your presentation
proposal. In your submission please note how long your talk will take.
Slots vary in length up to 45 minutes. Also include in your proposal
the proposal type -- one of:
- technical talk
- end-user talk
- birds of a feather (BOF) session
Submit your proposal here:
http://events.linuxfoundation.org/cfp
You will receive a notification whether or not your presentation proposal
was accepted by Sep 14th.
END-USER COLLABORATION
One of the big challenges as developers is to know what, where and how
people actually use our software. We will reserve a few slots for end
users talking about their deployment challenges and achievements.
If you are using oVirt in production you are encouraged submit a speaking
proposal. Simply mark it as an end-user collaboration proposal. As an
end user, this is a unique opportunity to get your input to developers.
BOF SESSION
We will reserve some slots in the evening after the main conference
tracks, for birds of a feather (BOF) sessions. These sessions will be
less formal than presentation tracks and targetted for people who would
like to discuss specific issues with other developers and/or users.
If you are interested in getting developers and/or uses together to
discuss a specific problem, please submit a BOF proposal.
LIGHTNING TALKS
In addition to submitted talks we will also have some room for lightning
talks. These are short (5 minute) discussions to highlight new work or
ideas that aren't complete enough to warrant a full presentation slot.
Lightning talk submissions and scheduling will be handled on-site at
oVirt Workshop.
HOTEL / TRAVEL
The oVirt Workshop Europe 2012 will be held in Barcelona, Spain at the
Hotel Fira Palace.
http://events.linuxfoundation.org/events/kvm-forum/hotel
Thank you for your interest in oVirt. We're looking forward to your
submissions and seeing you at the oVirt Workshop Europe 2012 in November!
Thanks,
your oVirt Workshop Europe 2012 Program Commitee
Please contact us with any questions or comments.
workshop-pc@ovirt.org
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-14 22: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: <20120814213412.GG22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 06:34:13PM -0300, Rafael Aquini wrote:
> On Tue, Aug 14, 2012 at 11:49:06PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 14, 2012 at 05:29:50PM -0300, Rafael Aquini wrote:
> > > On Tue, Aug 14, 2012 at 11:24:01PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Aug 14, 2012 at 05:08:31PM -0300, Rafael Aquini wrote:
> > > > > 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?
> > > > >
> > > >
> > > > Since guest crashes when it's not the case, no it doesn't, sorry :(.
> > > >
> > > Ok, but right now this driver only takes care of 1 balloon per guest,
> >
> > It does? Are you sure? There is no global state as far as I can see. So
> > I can create 2 devices and driver will happily create two instances,
> > each one can be inflated/deflated independently.
> >
> > > so how
> > > could this approach crash it?
> >
> > Add device. inflate. Add another device. inflate. deflate. unplug.
> > Now you have pointer to freed memory and when mm touches
> > page from first device, you ge use after free.
> >
> > > Your point is a good thing to be on a to-do list for future enhancements, but
> > > it's not a dealbreaker for the present balloon driver implementation, IMHO.
> > >
> >
> > Yes it looks like a dealbreaker to me.
>
> Sorry. You're right, I'm wrong.
>
> I'll get back to the scracthpad to overcome this constraint. I believe the way
> this patch was at its v4 revision (wrt this particular case) could possibly
> address this concern of yours.
Almost. We still have a global balloon_mapping. The only reason for
it to exist seems solely to detect balloon mappings, so it
can just be replaced by a flag in the mapping, or in mapping
ops, or elsewhere. Also, please add APIs to mm so we can
avoid doing internal mm stuff like
INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
in the driver. It should be
alloc_address_mapping(&virtio_balloon_aops);
free_address_mapping
Make page->mapping use rcu, and sync rcu in
free_address_mapping.
--
MST
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rusty Russell @ 2012-08-15 3:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, Minchan Kim,
linux-kernel, virtualization, linux-mm, Andi Kleen, Andrew Morton
In-Reply-To: <20120814083320.GA3597@redhat.com>
On Tue, 14 Aug 2012 11:33:20 +0300, "Michael S. Tsirkin" <mst@redhat.com> 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.
Actually, I have no idea.
Where does virtballoon_putbackpage get called from? It's some weird mm
thing, and I stay out of that mess.
The vb thread is stopped before we spin checking vb->num_pages, so it's
not touching pages; who would be calling this?
Confused,
Rusty.
^ permalink raw reply
* Re: [PATCH v7 1/4] mm: introduce compaction and migration for virtio ballooned pages
From: Mel Gorman @ 2012-08-15 8:52 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton
In-Reply-To: <20120814200043.GB22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 05:00:49PM -0300, Rafael Aquini wrote:
> 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?
>
I viewed it as being defensive programming. VM_BUG_ON would be less
useful as it can be compiled out. If the race can be routinely hit then
multiple warnings is instructive in itself. I have no strong feelings
about this though. I see little harm in making the check but in light of
this conversation add a short comment explaining that the check should
be redundant.
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Mel Gorman @ 2012-08-15 9:05 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Michael S. Tsirkin,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton
In-Reply-To: <20120814201113.GE22133@t510.redhat.com>
On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> 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.
>
I have not thought about this as deeply as I shouold but is simply rechecking
the mapping under the pages_lock to make sure the page is still a balloon
page an option? i.e. use pages_lock to stabilise page->mapping.
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-15 9:25 UTC (permalink / raw)
To: Mel Gorman
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120815090528.GH4052@csn.ul.ie>
On Wed, Aug 15, 2012 at 10:05:28AM +0100, Mel Gorman wrote:
> On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> > 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.
> >
>
> I have not thought about this as deeply as I shouold but is simply rechecking
> the mapping under the pages_lock to make sure the page is still a balloon
> page an option? i.e. use pages_lock to stabilise page->mapping.
To clarify, are you concerned about cost of rcu_read_lock
for non balloon pages?
> --
> Mel Gorman
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Mel Gorman @ 2012-08-15 9:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120815092528.GA29214@redhat.com>
On Wed, Aug 15, 2012 at 12:25:28PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 15, 2012 at 10:05:28AM +0100, Mel Gorman wrote:
> > On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> > > 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.
> > >
> >
> > I have not thought about this as deeply as I shouold but is simply rechecking
> > the mapping under the pages_lock to make sure the page is still a balloon
> > page an option? i.e. use pages_lock to stabilise page->mapping.
>
> To clarify, are you concerned about cost of rcu_read_lock
> for non balloon pages?
>
Not as such, but given the choice between introducing RCU locking and
rechecking page->mapping under a spinlock I would choose the latter as it
is more straight-forward.
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-15 10:01 UTC (permalink / raw)
To: Mel Gorman
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120815094839.GJ4052@csn.ul.ie>
On Wed, Aug 15, 2012 at 10:48:39AM +0100, Mel Gorman wrote:
> On Wed, Aug 15, 2012 at 12:25:28PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Aug 15, 2012 at 10:05:28AM +0100, Mel Gorman wrote:
> > > On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> > > > 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.
> > > >
> > >
> > > I have not thought about this as deeply as I shouold but is simply rechecking
> > > the mapping under the pages_lock to make sure the page is still a balloon
> > > page an option? i.e. use pages_lock to stabilise page->mapping.
> >
> > To clarify, are you concerned about cost of rcu_read_lock
> > for non balloon pages?
> >
>
> Not as such, but given the choice between introducing RCU locking and
> rechecking page->mapping under a spinlock I would choose the latter as it
> is more straight-forward.
OK but checking it how? page->mapping == balloon_mapping does not scale to
multiple balloons, so I hoped we can switch to
page->mapping->flags & BALLOON_MAPPING or some such,
but this means we dereference it outside the lock ...
We will also need to add some API to set/clear mapping so that driver
does not need to poke in mm internals, but that's easy.
> --
> Mel Gorman
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Mel Gorman @ 2012-08-15 11:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120815100108.GA1999@redhat.com>
On Wed, Aug 15, 2012 at 01:01:08PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 15, 2012 at 10:48:39AM +0100, Mel Gorman wrote:
> > On Wed, Aug 15, 2012 at 12:25:28PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Aug 15, 2012 at 10:05:28AM +0100, Mel Gorman wrote:
> > > > On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> > > > > 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.
> > > > >
> > > >
> > > > I have not thought about this as deeply as I shouold but is simply rechecking
> > > > the mapping under the pages_lock to make sure the page is still a balloon
> > > > page an option? i.e. use pages_lock to stabilise page->mapping.
> > >
> > > To clarify, are you concerned about cost of rcu_read_lock
> > > for non balloon pages?
> > >
> >
> > Not as such, but given the choice between introducing RCU locking and
> > rechecking page->mapping under a spinlock I would choose the latter as it
> > is more straight-forward.
>
> OK but checking it how? page->mapping == balloon_mapping does not scale to
> multiple balloons,
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.
> so I hoped we can switch to
> page->mapping->flags & BALLOON_MAPPING or some such,
> but this means we dereference it outside the lock ...
>
That also sounded like future stuff to me that would be justified with
profiling if necessary. Personally I would have started with the spinlock
and a simple check and moved to RCU later when either scalability was a
problem or it was found there was a need to stabilise whether a page was
a balloon page or not outside a spinlock.
This is not a NAK to the idea and I'm not objecting to RCU being used now
if that is what is really desired. I just suspect it's making the series
more complex than it needs to be right now.
--
Mel Gorman
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Michael S. Tsirkin @ 2012-08-15 11:28 UTC (permalink / raw)
To: Mel Gorman
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton
In-Reply-To: <20120815111651.GL4052@csn.ul.ie>
On Wed, Aug 15, 2012 at 12:16:51PM +0100, Mel Gorman wrote:
> On Wed, Aug 15, 2012 at 01:01:08PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Aug 15, 2012 at 10:48:39AM +0100, Mel Gorman wrote:
> > > On Wed, Aug 15, 2012 at 12:25:28PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Aug 15, 2012 at 10:05:28AM +0100, Mel Gorman wrote:
> > > > > On Tue, Aug 14, 2012 at 05:11:13PM -0300, Rafael Aquini wrote:
> > > > > > 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.
> > > > > >
> > > > >
> > > > > I have not thought about this as deeply as I shouold but is simply rechecking
> > > > > the mapping under the pages_lock to make sure the page is still a balloon
> > > > > page an option? i.e. use pages_lock to stabilise page->mapping.
> > > >
> > > > To clarify, are you concerned about cost of rcu_read_lock
> > > > for non balloon pages?
> > > >
> > >
> > > Not as such, but given the choice between introducing RCU locking and
> > > rechecking page->mapping under a spinlock I would choose the latter as it
> > > is more straight-forward.
> >
> > OK but checking it how? page->mapping == balloon_mapping does not scale to
> > multiple balloons,
>
> 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.
> > so I hoped we can switch to
> > page->mapping->flags & BALLOON_MAPPING or some such,
> > but this means we dereference it outside the lock ...
> >
>
> That also sounded like future stuff to me that would be justified with
> profiling if necessary. Personally I would have started with the spinlock
> and a simple check and moved to RCU later when either scalability was a
> problem or it was found there was a need to stabilise whether a page was
> a balloon page or not outside a spinlock.
>
> This is not a NAK to the idea and I'm not objecting to RCU being used now
> if that is what is really desired. I just suspect it's making the series
> more complex than it needs to be right now.
>
> --
> Mel Gorman
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 2/4] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-08-15 12: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: <20120814193109.GA28840@redhat.com>
On Tue, Aug 14, 2012 at 10:31:09PM +0300, Michael S. Tsirkin wrote:
> > > 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.
CPU1 will be returning to putback_balloon_page() which code is located at core
mm/compaction.c, outside the driver.
^ permalink raw reply
* RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address injection
From: KY Srinivasan @ 2012-08-15 12:39 UTC (permalink / raw)
To: KY Srinivasan, 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: <426367E2313C2449837CD2DE46E7EAF9236BF8D0@SN2PRD0310MB382.namprd03.prod.outlook.com>
> -----Original Message-----
> From: devel-bounces@linuxdriverproject.org [mailto:devel-
> bounces@linuxdriverproject.org] On Behalf Of KY Srinivasan
> Sent: Monday, August 13, 2012 10:57 PM
> 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
> Subject: RE: [PATCH V2 02/18] Drivers: hv: Add KVP definitions for IP address
> injection
>
>
>
> > -----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.
>
Greg,
I hope this explanation is satisfactory. If there are other issues that you would
want me to address before this patch set can be accepted, let me know and I will
address them right away.
Regards,
K. Y
^ 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