* Re: virtio scsi host draft specification, v3
From: Hai Dong,Li @ 2011-07-04 13:38 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Christoph Hellwig, Stefan Hajnoczi, kvm, Michael S. Tsirkin,
qemu-devel, Linux Kernel Mailing List, Linux Virtualization
In-Reply-To: <4E0D86E0.1020406@redhat.com>
On 07/01/2011 09:14 AM, Hannes Reinecke wrote:
>> Actually, the kernel does _not_ do a LUN remapping.
>
> Not the kernel, the in-kernel target. The in-kernel target can and will
> map hardware LUNs (target_lun in drivers/target/*) to arbitrary LUNs
> (mapped_lun).
>
>>> Put in another way: the virtio-scsi device is itself a SCSI
>>> target,
>>
>> Argl. No way. The virtio-scsi device has to map to a single LUN.
>
> I think we are talking about different things. By "virtio-scsi device"
> I meant the "virtio-scsi HBA". When I referred to a LUN as seen by the
> guest, I was calling it a "virtual SCSI device". So yes, we were
> calling things with different names. Perhaps from now on
> we can call them virtio-scsi {initiator,target,LUN} and have no
> ambiguity? I'll also modify the spec in this sense.
>
>> The SCSI spec itself only deals with LUNs, so anything you'll read in
>> there obviously will only handle the interaction between the
>> initiator (read: host) and the LUN itself. However, the actual
>> command is send via an intermediat target, hence you'll always see
>> the reference to the ITL (initiator-target-lun) nexus.
>
> Yes, this I understand.
>
>> The SCSI spec details discovery of the individual LUNs presented by a
>> given target, it does _NOT_ detail the discovery of the targets
>> themselves. That is being delegated to the underlying transport
>
> And in fact I have this in virtio-scsi too, since virtio-scsi _is_ a
> transport:
Oh, here I catch up. I was wondering why there're ordering issues when
talking
about virtio-scsi, since in SAM3, the third and the last paragraph of
section
4.6.3 Request/Response ordering clearly describe it:
The manner in which ordering constraints are established is vendor
specific. An
implementation may delegate this responsibility to the application client
(e.g., the device driver). In-order delivery may be an intrinsic property of
the service delivery subsystem or a requirement established by the SCSI
transport protocol standard.
To simplify the description of behavior, the SCSI architecture model assumes
in-order delivery of requests or responses to be a property of the service
delivery subsystem. This assumption does not constitute a requirement. The
SCSI architecture model makes no assumption about and places no
requirement on
the ordering of requests or responses for different I_T nexuses.
So if I understand correctly, virtio-scsi looks like an SCSI tranport
protocol,
such as iSCSI, FCP and SRP which use tcp/ip, FC and Infiniband RDMA
respectively as the transfer media while virtio-scsi uses virtio, an
virtual IO
channel, as the transfer media?
>
> When VIRTIO_SCSI_EVT_RESET_REMOVED or VIRTIO_SCSI_EVT_RESET_RESCAN
> is sent for LUN 0, the driver should ask the initiator to rescan
> the target, in order to detect the case when an entire target has
> appeared or disappeared.
>
> [If the device fails] to report an event due to missing buffers,
> [...] the driver should poll the logical units for unit attention
> conditions, and/or do whatever form of bus scan is appropriate for
> the guest operating system.
>
>> In the case of NPIV it would make sense to map the virtual SCSI host
>> to the backend, so that all devices presented to the virtual SCSI
>> host will be presented to the backend, too. However, when doing so
>> these devices will normally be referenced by their original LUN, as
>> these will be presented to the guest via eg 'REPORT LUNS'.
>
> Right.
>
>> The above thread now tries to figure out if we should remap those LUN
>> numbers or just expose them as they are. If we decide on remapping,
>> we have to emulate _all_ commands referring explicitely to those LUN
>> numbers (persistent reservations, anyone?).
>
> But it seems to me that commands referring explicitly to LUN numbers
> most likely have to be reimplemented anyway for virtualization. I'm
> thinking exactly of persistent reservations. If two guests on the same
> host try a persistent reservation, they should conflict with each other.
> If reservation commands were just passed through, they would be seen
> as coming from the same initiator (the HBA driver or iSCSI initiator in
> the host OS).
>
> etc.
>
>> If we don't, we would expose some hardware detail to the guest, but
>> would save us _a lot_ of processing.
>
> But can we afford it? And would the architecture allow that at all?
>
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" 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] Modpost section mismatch fix
From: Ian Campbell @ 2011-07-04 8:49 UTC (permalink / raw)
To: Raghavendra D Prabhu
Cc: xen-devel, jeremy.fitzhardinge, virtualization, linux-kernel,
Konrad Rzeszutek Wilk
In-Reply-To: <20110703232508.GB4440@Xye>
On Mon, 2011-07-04 at 04:55 +0530, Raghavendra D Prabhu wrote:
> [Sorry if duplicate, one earlier was corrupt]
>
> Hi,
> I got section mismatches reported by modpost in latest build. It got
> reported for xen_register_pirq and xen_unplug_emulated_devices
> functions.
> xen_register_pirq makes reference to
> acpi_sci_override_gsi in init.data section; marking
> xen_register_pirq with __init is not feasible since calls are made
> to it from acpi_register_gsi in non-init contexts. So marking it
> __refdata based on assumption that when acpi_sci_override_gsi is
> referenced, it is in early stages where it is alive.
I don't think this assumption holds, since xen_register_pirq can be
called at any time and basically unconditionally references
acpi_sci_override_gsi.
If we don't want to remove the __init from acpi_sci_override_gsi then
perhaps xen_setup_acpi_sci needs to stash it somewhere?
Or maybe xen_register_pirq could take an "int force_irq" which, if not
-1, would force a particular IRQ. The callsite in xen_setup_acpi_sci
(actually via xen_register_gsi so the param would need to be propagated
there) would be the only actual user?
The xen_unplug_emulated_devices change looks correct to me since
xen_unplug_emulated_devices is called from xen_arch_hvm_post_suspend.
Ian.
>
>
> --------------------------
> Raghavendra Prabhu
> GPG Id : 0xD72BE977
> Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
> www: wnohang.net
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/virtualization
--
Ian Campbell
Current Noise: Crowbar - Remember Tomorrow (A Tribute To Iron Maiden)
SANTA CLAUS comes down a FIRE ESCAPE wearing bright blue LEG WARMERS
... He scrubs the POPE with a mild soap or detergent for 15 minutes,
starring JANE FONDA!!
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Rusty Russell @ 2011-07-04 7:37 UTC (permalink / raw)
To: Michael S. Tsirkin, Sasha Levin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <20110703121137.GA15826@redhat.com>
On Sun, 3 Jul 2011 15:11:37 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Jul 03, 2011 at 01:52:29PM +0300, Sasha Levin wrote:
> > This just leads me to believe that we should either not notify the host,
> > or not wait_for_completion() when telling the host.
>
> Interesting. The spec says
>
> (a) The driver constructs an array of addresses of memory pages it has
> previously given to the balloon, as described above. This descriptor
> is added to the deflateq.
> (b) If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set,
> the guest may not use these requested pages until that descriptor in
> the deflateq has been used by the device.
> (c) Otherwise, the guest may begin to re-use pages previously given to
> the balloon before the device has acknowledged their withdrawl.
> 21 In this case, deflation advice is merely a courtesy
>
> This does not discuss the following issue: what happens
> if the device never uses the descriptor in the deflateq
> and VIRTIO_BALLOON_F_MUST_TELL_HOST is not set?
>
> Rusty, any comments?
The device still *has* a queue. It should process it!
The feature should be called VIRTIO_BALLOON_F_ASK_HOST_BEFORE_REUSE.
If we didn't want the information at *all*, we would have made the queue
not exist in that case...
Thanks,
Rusty.
^ permalink raw reply
* [PATCH] Modpost section mismatch fix
From: Raghavendra D Prabhu @ 2011-07-03 23:25 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: linux-kernel, jeremy.fitzhardinge, xen-devel, virtualization
[-- Attachment #1: Type: text/plain, Size: 722 bytes --]
[Sorry if duplicate, one earlier was corrupt]
Hi,
I got section mismatches reported by modpost in latest build. It got
reported for xen_register_pirq and xen_unplug_emulated_devices
functions. xen_register_pirq makes reference to
acpi_sci_override_gsi in init.data section; marking
xen_register_pirq with __init is not feasible since calls are made
to it from acpi_register_gsi in non-init contexts. So marking it
__refdata based on assumption that when acpi_sci_override_gsi is
referenced, it is in early stages where it is alive.
--------------------------
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net
[-- Attachment #2: 0001-xen-pci-Fix-modpost-warnings-regarding-section-misma.patch --]
[-- Type: text/plain, Size: 1730 bytes --]
From 7dfd47575fd695fe8ddba951515cfac01a2ab8bc Mon Sep 17 00:00:00 2001
Message-Id: <7dfd47575fd695fe8ddba951515cfac01a2ab8bc.1309641255.git.rprabhu@wnohang.net>
From: Raghavendra D Prabhu <rprabhu@wnohang.net>
Date: Sun, 3 Jul 2011 01:48:50 +0530
Subject: [PATCH] xen/pci: Fix modpost warnings regarding section mismatch
Marking xen_register_pirq with __refdata since it is referencing
acpi_sci_override_gsi, an __initdata variable. Removing __init from
check_platform_magic since it is called by xen_unplug_emulated_devices in
non-init contexts (It probably gets inlined because of
-finline-functions-called-once, removing __init is more to avoid mismatch being
reported).
Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
arch/x86/pci/xen.c | 2 +-
arch/x86/xen/platform-pci-unplug.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index fe00830..fb5eeb0 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -327,7 +327,7 @@ int __init pci_xen_hvm_init(void)
}
#ifdef CONFIG_XEN_DOM0
-static int xen_register_pirq(u32 gsi, int triggering)
+static __refdata int xen_register_pirq(u32 gsi, int triggering)
{
int rc, pirq, irq = -1;
struct physdev_map_pirq map_irq;
diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c
index 25c52f9..ffcf261 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -35,7 +35,7 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
#ifdef CONFIG_XEN_PVHVM
static int xen_emul_unplug;
-static int __init check_platform_magic(void)
+static int check_platform_magic(void)
{
short magic;
char protocol;
--
1.7.6
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Michael S. Tsirkin @ 2011-07-03 16:36 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <1309705961.2250.36.camel@sasha>
On Sun, Jul 03, 2011 at 06:12:41PM +0300, Sasha Levin wrote:
> If we choose to
> leave it this way we can just go ahead and remove MUST_TELL_HOST.
It probably needs to stay so that the host can detect that
the guest does guarantee telling it first.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Sasha Levin @ 2011-07-03 15:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <20110703134439.GA22493@redhat.com>
On Sun, 2011-07-03 at 16:44 +0300, Michael S. Tsirkin wrote:
> On Sun, Jul 03, 2011 at 03:32:36PM +0300, Sasha Levin wrote:
> > On Sun, 2011-07-03 at 15:11 +0300, Michael S. Tsirkin wrote:
> > > On Sun, Jul 03, 2011 at 01:52:29PM +0300, Sasha Levin wrote:
> > > > On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:
> > > > > On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> > > > > > On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > > > > > > Doesn't this basically just revert
> > > > > > > bf50e69f63d21091e525185c3ae761412be0ba72?
> > > > > > >
> > > > > >
> > > > > > Yes. I haven't noticed that commit.
> > > > > >
> > > > > > Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> > > > > > unnecessary delay due to having to kick and wait for the host to process
> > > > > > the deflate vq before actually using the memory pages.
> > > > >
> > > > > OTOH, as the commit points out:
> > > > > Without this feature, we might free a page (and have another
> > > > > user touch it) while the hypervisor is unprepared for it
> > > > >
> > > > > Are you doing so many deflates that the speed is important?
> > > > >
> > > >
> > > > In the case of kvm tools and qemu for example, the hypervisor doesn't
> > > > need to be 'prepared' for it.
> > > >
> > > > When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
> > > > which means that we can access those pages immediately when deflating
> > > > without having to do anything special to prepare for it.
> > > >
> > > > I'm not saying that speed is critical here, I'm just saying that the
> > > > feature was added to the spec for a reason - to prevent unnecessary
> > > > overhead like in the case of kvm tools and qemu, and we just choose to
> > > > ignore it completely.
> > > >
> > > > > > I've stumbled on it when writing the virtio_balloon driver for kvm
> > > > > > tools. Initially my plan was to ignore the deflate vq completely, since
> > > > > > the virtio spec says that unless we set the MUST_TELL_HOST flag,
> > > > > > "deflation advice is merely a courtesy".
> > > > > >
> > > > > > I've noticed that if I don't process the deflate vq the guest doesn't
> > > > > > use the deflated pages at all - something which contradicts the feature
> > > > > > that lets him use the deflated pages without waiting for the deflate vq.
> > > > >
> > > > > OK, so you have a host that does not process the vq.
> > > > > As a result tell_host blocks forever.
> > > > > With your patch, guest will block after releasing the
> > > > > page, so the next deflate request will get stuck.
> > > > > This doesn't seem to be a huge improvement so
> > > > > we can conclude such a host is broken?
> > > > >
> > > >
> > > > No, the host is not doing anything wrong by not processing deflate vq.
> > > >
> > > > This just leads me to believe that we should either not notify the host,
> > > > or not wait_for_completion() when telling the host.
> > >
> > > Interesting. The spec says
> > >
> > > (a) The driver constructs an array of addresses of memory pages it has
> > > previously given to the balloon, as described above. This descriptor
> > > is added to the deflateq.
> > > (b) If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set,
> > > the guest may not use these requested pages until that descriptor in
> > > the deflateq has been used by the device.
> > > (c) Otherwise, the guest may begin to re-use pages previously given to
> > > the balloon before the device has acknowledged their withdrawl.
> > > 21 In this case, deflation advice is merely a courtesy
> > >
> > > This does not discuss the following issue: what happens
> > > if the device never uses the descriptor in the deflateq
> > > and VIRTIO_BALLOON_F_MUST_TELL_HOST is not set?
> > >
> > > Rusty, any comments?
> > >
> > > As far as I can tell, linux drivers assume that
> > > devices acknowledge all descriptors, and did this
> > > from the very beginning, so if you want the
> > > device not to process that queue at all,
> > > you will probably need a new flag for this rather than reuse
> > > MUST_TELL_HOST.
> > >
> >
> > There are 2 questions here:
> >
> > One is whether devices should acknowledge all descriptors. I agree with
> > what you said that if the assumption was that devices should acknowledge
> > all descriptors I should definitely work on processing deflate vq (maybe
> > then it's also worth a note in the spec to make it more than an
> > assumption).
>
> It's not by design but it seems to be required for existing drivers to
> work, so a historical note would be in place, yes. Spec patch?
>
> > The other is whether we should wait for the balloon device to
> > acknowledge deflated pages before using them with MUST_TELL_HOST not
> > set. Even if I do acknowledge deflate vq descriptors, I may want to do
> > it at set intervals (say once a minute for example) - theres no reason
> > for deflation to stall until this processing occurs.
>
> Well, it seems that if you process descriptors very slowly guest
> driver will get stuck for a while, with or without this patch.
> Can you present a convincing argument why batching deflate
> descriptors in this way makes sense?
>
It'll be slow with this patch, but as I said above - This patch is
probably wrong. The right patch should either avoid telling the host or
shouldn't wait for an answer from the host.
The real reason behind it is just following the specs. If we choose to
leave it this way we can just go ahead and remove MUST_TELL_HOST.
> It seems that this all started because the kvm tool didn't
> process deflate vq at all. Assuming it gains this processing,
> is some other reason left?
>
There wasn't any real kvm tools issue there, I've added the deflate vq
handling immediately - I just wanted to see why the balloon driver
doesn't behave like it was described in the specs.
> > >
> > > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > > > > > > Unless the host requires that requested pages won't be used until
> > > > > > > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > > > > > > deflating the balloon.
> > > > > > > >
> > > > > > > > This will avoid having to take an exit before actually using the pages.
> > > > > > > >
> > > > > > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > > > > Cc: kvm@vger.kernel.org
> > > > > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > > > > > > ---
> > > > > > > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > > > > > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > > > > > index e058ace..055f95d 100644
> > > > > > > > --- a/drivers/virtio/virtio_balloon.c
> > > > > > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > > > > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > > > > > > vb->num_pages--;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -
> > > > > > > > /*
> > > > > > > > - * Note that if
> > > > > > > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > > > > > > - * is true, we *have* to do it in this order
> > > > > > > > + * If the host doesn't require us to notify him before using
> > > > > > > > + * pages which belong to the balloon, update him only after
> > > > > > > > + * freeing those pages for guest use.
> > > > > > > > */
> > > > > > > > - tell_host(vb, vb->deflate_vq);
> > > > > > > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > > > > > > + tell_host(vb, vb->deflate_vq);
> > > > > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > > > + } else {
> > > > > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > > > + tell_host(vb, vb->deflate_vq);
> > > > > > > > + }
> > > > > > > > }
> > > > > > > >
> > > > > > > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > > > > > > --
> > > > > > > > 1.7.6
> > > > > >
> > > > > >
> > > > > > --
> > > > > >
> > > > > > Sasha.
> > > >
> > > >
> > > > --
> > > >
> > > > Sasha.
> >
> >
> > --
> >
> > Sasha.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Michael S. Tsirkin @ 2011-07-03 13:44 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <1309696356.2250.31.camel@sasha>
On Sun, Jul 03, 2011 at 03:32:36PM +0300, Sasha Levin wrote:
> On Sun, 2011-07-03 at 15:11 +0300, Michael S. Tsirkin wrote:
> > On Sun, Jul 03, 2011 at 01:52:29PM +0300, Sasha Levin wrote:
> > > On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:
> > > > On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> > > > > On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > > > > > Doesn't this basically just revert
> > > > > > bf50e69f63d21091e525185c3ae761412be0ba72?
> > > > > >
> > > > >
> > > > > Yes. I haven't noticed that commit.
> > > > >
> > > > > Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> > > > > unnecessary delay due to having to kick and wait for the host to process
> > > > > the deflate vq before actually using the memory pages.
> > > >
> > > > OTOH, as the commit points out:
> > > > Without this feature, we might free a page (and have another
> > > > user touch it) while the hypervisor is unprepared for it
> > > >
> > > > Are you doing so many deflates that the speed is important?
> > > >
> > >
> > > In the case of kvm tools and qemu for example, the hypervisor doesn't
> > > need to be 'prepared' for it.
> > >
> > > When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
> > > which means that we can access those pages immediately when deflating
> > > without having to do anything special to prepare for it.
> > >
> > > I'm not saying that speed is critical here, I'm just saying that the
> > > feature was added to the spec for a reason - to prevent unnecessary
> > > overhead like in the case of kvm tools and qemu, and we just choose to
> > > ignore it completely.
> > >
> > > > > I've stumbled on it when writing the virtio_balloon driver for kvm
> > > > > tools. Initially my plan was to ignore the deflate vq completely, since
> > > > > the virtio spec says that unless we set the MUST_TELL_HOST flag,
> > > > > "deflation advice is merely a courtesy".
> > > > >
> > > > > I've noticed that if I don't process the deflate vq the guest doesn't
> > > > > use the deflated pages at all - something which contradicts the feature
> > > > > that lets him use the deflated pages without waiting for the deflate vq.
> > > >
> > > > OK, so you have a host that does not process the vq.
> > > > As a result tell_host blocks forever.
> > > > With your patch, guest will block after releasing the
> > > > page, so the next deflate request will get stuck.
> > > > This doesn't seem to be a huge improvement so
> > > > we can conclude such a host is broken?
> > > >
> > >
> > > No, the host is not doing anything wrong by not processing deflate vq.
> > >
> > > This just leads me to believe that we should either not notify the host,
> > > or not wait_for_completion() when telling the host.
> >
> > Interesting. The spec says
> >
> > (a) The driver constructs an array of addresses of memory pages it has
> > previously given to the balloon, as described above. This descriptor
> > is added to the deflateq.
> > (b) If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set,
> > the guest may not use these requested pages until that descriptor in
> > the deflateq has been used by the device.
> > (c) Otherwise, the guest may begin to re-use pages previously given to
> > the balloon before the device has acknowledged their withdrawl.
> > 21 In this case, deflation advice is merely a courtesy
> >
> > This does not discuss the following issue: what happens
> > if the device never uses the descriptor in the deflateq
> > and VIRTIO_BALLOON_F_MUST_TELL_HOST is not set?
> >
> > Rusty, any comments?
> >
> > As far as I can tell, linux drivers assume that
> > devices acknowledge all descriptors, and did this
> > from the very beginning, so if you want the
> > device not to process that queue at all,
> > you will probably need a new flag for this rather than reuse
> > MUST_TELL_HOST.
> >
>
> There are 2 questions here:
>
> One is whether devices should acknowledge all descriptors. I agree with
> what you said that if the assumption was that devices should acknowledge
> all descriptors I should definitely work on processing deflate vq (maybe
> then it's also worth a note in the spec to make it more than an
> assumption).
It's not by design but it seems to be required for existing drivers to
work, so a historical note would be in place, yes. Spec patch?
> The other is whether we should wait for the balloon device to
> acknowledge deflated pages before using them with MUST_TELL_HOST not
> set. Even if I do acknowledge deflate vq descriptors, I may want to do
> it at set intervals (say once a minute for example) - theres no reason
> for deflation to stall until this processing occurs.
Well, it seems that if you process descriptors very slowly guest
driver will get stuck for a while, with or without this patch.
Can you present a convincing argument why batching deflate
descriptors in this way makes sense?
It seems that this all started because the kvm tool didn't
process deflate vq at all. Assuming it gains this processing,
is some other reason left?
> >
> > > >
> > > >
> > > >
> > > > > >
> > > > > > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > > > > > Unless the host requires that requested pages won't be used until
> > > > > > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > > > > > deflating the balloon.
> > > > > > >
> > > > > > > This will avoid having to take an exit before actually using the pages.
> > > > > > >
> > > > > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > > > Cc: kvm@vger.kernel.org
> > > > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > > > > > ---
> > > > > > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > > > > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > > > > index e058ace..055f95d 100644
> > > > > > > --- a/drivers/virtio/virtio_balloon.c
> > > > > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > > > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > > > > > vb->num_pages--;
> > > > > > > }
> > > > > > >
> > > > > > > -
> > > > > > > /*
> > > > > > > - * Note that if
> > > > > > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > > > > > - * is true, we *have* to do it in this order
> > > > > > > + * If the host doesn't require us to notify him before using
> > > > > > > + * pages which belong to the balloon, update him only after
> > > > > > > + * freeing those pages for guest use.
> > > > > > > */
> > > > > > > - tell_host(vb, vb->deflate_vq);
> > > > > > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > > > > > + tell_host(vb, vb->deflate_vq);
> > > > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > > + } else {
> > > > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > > + tell_host(vb, vb->deflate_vq);
> > > > > > > + }
> > > > > > > }
> > > > > > >
> > > > > > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > > > > > --
> > > > > > > 1.7.6
> > > > >
> > > > >
> > > > > --
> > > > >
> > > > > Sasha.
> > >
> > >
> > > --
> > >
> > > Sasha.
>
>
> --
>
> Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Sasha Levin @ 2011-07-03 12:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <20110703121137.GA15826@redhat.com>
On Sun, 2011-07-03 at 15:11 +0300, Michael S. Tsirkin wrote:
> On Sun, Jul 03, 2011 at 01:52:29PM +0300, Sasha Levin wrote:
> > On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:
> > > On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> > > > On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > > > > Doesn't this basically just revert
> > > > > bf50e69f63d21091e525185c3ae761412be0ba72?
> > > > >
> > > >
> > > > Yes. I haven't noticed that commit.
> > > >
> > > > Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> > > > unnecessary delay due to having to kick and wait for the host to process
> > > > the deflate vq before actually using the memory pages.
> > >
> > > OTOH, as the commit points out:
> > > Without this feature, we might free a page (and have another
> > > user touch it) while the hypervisor is unprepared for it
> > >
> > > Are you doing so many deflates that the speed is important?
> > >
> >
> > In the case of kvm tools and qemu for example, the hypervisor doesn't
> > need to be 'prepared' for it.
> >
> > When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
> > which means that we can access those pages immediately when deflating
> > without having to do anything special to prepare for it.
> >
> > I'm not saying that speed is critical here, I'm just saying that the
> > feature was added to the spec for a reason - to prevent unnecessary
> > overhead like in the case of kvm tools and qemu, and we just choose to
> > ignore it completely.
> >
> > > > I've stumbled on it when writing the virtio_balloon driver for kvm
> > > > tools. Initially my plan was to ignore the deflate vq completely, since
> > > > the virtio spec says that unless we set the MUST_TELL_HOST flag,
> > > > "deflation advice is merely a courtesy".
> > > >
> > > > I've noticed that if I don't process the deflate vq the guest doesn't
> > > > use the deflated pages at all - something which contradicts the feature
> > > > that lets him use the deflated pages without waiting for the deflate vq.
> > >
> > > OK, so you have a host that does not process the vq.
> > > As a result tell_host blocks forever.
> > > With your patch, guest will block after releasing the
> > > page, so the next deflate request will get stuck.
> > > This doesn't seem to be a huge improvement so
> > > we can conclude such a host is broken?
> > >
> >
> > No, the host is not doing anything wrong by not processing deflate vq.
> >
> > This just leads me to believe that we should either not notify the host,
> > or not wait_for_completion() when telling the host.
>
> Interesting. The spec says
>
> (a) The driver constructs an array of addresses of memory pages it has
> previously given to the balloon, as described above. This descriptor
> is added to the deflateq.
> (b) If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set,
> the guest may not use these requested pages until that descriptor in
> the deflateq has been used by the device.
> (c) Otherwise, the guest may begin to re-use pages previously given to
> the balloon before the device has acknowledged their withdrawl.
> 21 In this case, deflation advice is merely a courtesy
>
> This does not discuss the following issue: what happens
> if the device never uses the descriptor in the deflateq
> and VIRTIO_BALLOON_F_MUST_TELL_HOST is not set?
>
> Rusty, any comments?
>
> As far as I can tell, linux drivers assume that
> devices acknowledge all descriptors, and did this
> from the very beginning, so if you want the
> device not to process that queue at all,
> you will probably need a new flag for this rather than reuse
> MUST_TELL_HOST.
>
There are 2 questions here:
One is whether devices should acknowledge all descriptors. I agree with
what you said that if the assumption was that devices should acknowledge
all descriptors I should definitely work on processing deflate vq (maybe
then it's also worth a note in the spec to make it more than an
assumption).
The other is whether we should wait for the balloon device to
acknowledge deflated pages before using them with MUST_TELL_HOST not
set. Even if I do acknowledge deflate vq descriptors, I may want to do
it at set intervals (say once a minute for example) - theres no reason
for deflation to stall until this processing occurs.
>
> > >
> > >
> > >
> > > > >
> > > > > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > > > > Unless the host requires that requested pages won't be used until
> > > > > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > > > > deflating the balloon.
> > > > > >
> > > > > > This will avoid having to take an exit before actually using the pages.
> > > > > >
> > > > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > > Cc: kvm@vger.kernel.org
> > > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > > > > ---
> > > > > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > > > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > > > index e058ace..055f95d 100644
> > > > > > --- a/drivers/virtio/virtio_balloon.c
> > > > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > > > > vb->num_pages--;
> > > > > > }
> > > > > >
> > > > > > -
> > > > > > /*
> > > > > > - * Note that if
> > > > > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > > > > - * is true, we *have* to do it in this order
> > > > > > + * If the host doesn't require us to notify him before using
> > > > > > + * pages which belong to the balloon, update him only after
> > > > > > + * freeing those pages for guest use.
> > > > > > */
> > > > > > - tell_host(vb, vb->deflate_vq);
> > > > > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > > > > + tell_host(vb, vb->deflate_vq);
> > > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > + } else {
> > > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > > + tell_host(vb, vb->deflate_vq);
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > > > > --
> > > > > > 1.7.6
> > > >
> > > >
> > > > --
> > > >
> > > > Sasha.
> >
> >
> > --
> >
> > Sasha.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Michael S. Tsirkin @ 2011-07-03 12:11 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <1309690349.2250.21.camel@sasha>
On Sun, Jul 03, 2011 at 01:52:29PM +0300, Sasha Levin wrote:
> On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:
> > On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> > > On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > > > Doesn't this basically just revert
> > > > bf50e69f63d21091e525185c3ae761412be0ba72?
> > > >
> > >
> > > Yes. I haven't noticed that commit.
> > >
> > > Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> > > unnecessary delay due to having to kick and wait for the host to process
> > > the deflate vq before actually using the memory pages.
> >
> > OTOH, as the commit points out:
> > Without this feature, we might free a page (and have another
> > user touch it) while the hypervisor is unprepared for it
> >
> > Are you doing so many deflates that the speed is important?
> >
>
> In the case of kvm tools and qemu for example, the hypervisor doesn't
> need to be 'prepared' for it.
>
> When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
> which means that we can access those pages immediately when deflating
> without having to do anything special to prepare for it.
>
> I'm not saying that speed is critical here, I'm just saying that the
> feature was added to the spec for a reason - to prevent unnecessary
> overhead like in the case of kvm tools and qemu, and we just choose to
> ignore it completely.
>
> > > I've stumbled on it when writing the virtio_balloon driver for kvm
> > > tools. Initially my plan was to ignore the deflate vq completely, since
> > > the virtio spec says that unless we set the MUST_TELL_HOST flag,
> > > "deflation advice is merely a courtesy".
> > >
> > > I've noticed that if I don't process the deflate vq the guest doesn't
> > > use the deflated pages at all - something which contradicts the feature
> > > that lets him use the deflated pages without waiting for the deflate vq.
> >
> > OK, so you have a host that does not process the vq.
> > As a result tell_host blocks forever.
> > With your patch, guest will block after releasing the
> > page, so the next deflate request will get stuck.
> > This doesn't seem to be a huge improvement so
> > we can conclude such a host is broken?
> >
>
> No, the host is not doing anything wrong by not processing deflate vq.
>
> This just leads me to believe that we should either not notify the host,
> or not wait_for_completion() when telling the host.
Interesting. The spec says
(a) The driver constructs an array of addresses of memory pages it has
previously given to the balloon, as described above. This descriptor
is added to the deflateq.
(b) If the VIRTIO_BALLOON_F_MUST_TELL_HOST feature is set,
the guest may not use these requested pages until that descriptor in
the deflateq has been used by the device.
(c) Otherwise, the guest may begin to re-use pages previously given to
the balloon before the device has acknowledged their withdrawl.
21 In this case, deflation advice is merely a courtesy
This does not discuss the following issue: what happens
if the device never uses the descriptor in the deflateq
and VIRTIO_BALLOON_F_MUST_TELL_HOST is not set?
Rusty, any comments?
As far as I can tell, linux drivers assume that
devices acknowledge all descriptors, and did this
from the very beginning, so if you want the
device not to process that queue at all,
you will probably need a new flag for this rather than reuse
MUST_TELL_HOST.
> >
> >
> >
> > > >
> > > > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > > > Unless the host requires that requested pages won't be used until
> > > > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > > > deflating the balloon.
> > > > >
> > > > > This will avoid having to take an exit before actually using the pages.
> > > > >
> > > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > Cc: virtualization@lists.linux-foundation.org
> > > > > Cc: kvm@vger.kernel.org
> > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > > > ---
> > > > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > > index e058ace..055f95d 100644
> > > > > --- a/drivers/virtio/virtio_balloon.c
> > > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > > > vb->num_pages--;
> > > > > }
> > > > >
> > > > > -
> > > > > /*
> > > > > - * Note that if
> > > > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > > > - * is true, we *have* to do it in this order
> > > > > + * If the host doesn't require us to notify him before using
> > > > > + * pages which belong to the balloon, update him only after
> > > > > + * freeing those pages for guest use.
> > > > > */
> > > > > - tell_host(vb, vb->deflate_vq);
> > > > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > > > + tell_host(vb, vb->deflate_vq);
> > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > + } else {
> > > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > > + tell_host(vb, vb->deflate_vq);
> > > > > + }
> > > > > }
> > > > >
> > > > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > > > --
> > > > > 1.7.6
> > >
> > >
> > > --
> > >
> > > Sasha.
>
>
> --
>
> Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Sasha Levin @ 2011-07-03 10:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <20110703103051.GA15537@redhat.com>
On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:
> On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> > On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > > Doesn't this basically just revert
> > > bf50e69f63d21091e525185c3ae761412be0ba72?
> > >
> >
> > Yes. I haven't noticed that commit.
> >
> > Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> > unnecessary delay due to having to kick and wait for the host to process
> > the deflate vq before actually using the memory pages.
>
> OTOH, as the commit points out:
> Without this feature, we might free a page (and have another
> user touch it) while the hypervisor is unprepared for it
>
> Are you doing so many deflates that the speed is important?
>
In the case of kvm tools and qemu for example, the hypervisor doesn't
need to be 'prepared' for it.
When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
which means that we can access those pages immediately when deflating
without having to do anything special to prepare for it.
I'm not saying that speed is critical here, I'm just saying that the
feature was added to the spec for a reason - to prevent unnecessary
overhead like in the case of kvm tools and qemu, and we just choose to
ignore it completely.
> > I've stumbled on it when writing the virtio_balloon driver for kvm
> > tools. Initially my plan was to ignore the deflate vq completely, since
> > the virtio spec says that unless we set the MUST_TELL_HOST flag,
> > "deflation advice is merely a courtesy".
> >
> > I've noticed that if I don't process the deflate vq the guest doesn't
> > use the deflated pages at all - something which contradicts the feature
> > that lets him use the deflated pages without waiting for the deflate vq.
>
> OK, so you have a host that does not process the vq.
> As a result tell_host blocks forever.
> With your patch, guest will block after releasing the
> page, so the next deflate request will get stuck.
> This doesn't seem to be a huge improvement so
> we can conclude such a host is broken?
>
No, the host is not doing anything wrong by not processing deflate vq.
This just leads me to believe that we should either not notify the host,
or not wait_for_completion() when telling the host.
>
>
>
> > >
> > > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > > Unless the host requires that requested pages won't be used until
> > > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > > deflating the balloon.
> > > >
> > > > This will avoid having to take an exit before actually using the pages.
> > > >
> > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > Cc: virtualization@lists.linux-foundation.org
> > > > Cc: kvm@vger.kernel.org
> > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > > ---
> > > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > index e058ace..055f95d 100644
> > > > --- a/drivers/virtio/virtio_balloon.c
> > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > > vb->num_pages--;
> > > > }
> > > >
> > > > -
> > > > /*
> > > > - * Note that if
> > > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > > - * is true, we *have* to do it in this order
> > > > + * If the host doesn't require us to notify him before using
> > > > + * pages which belong to the balloon, update him only after
> > > > + * freeing those pages for guest use.
> > > > */
> > > > - tell_host(vb, vb->deflate_vq);
> > > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > > + tell_host(vb, vb->deflate_vq);
> > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > + } else {
> > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > + tell_host(vb, vb->deflate_vq);
> > > > + }
> > > > }
> > > >
> > > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > > --
> > > > 1.7.6
> >
> >
> > --
> >
> > Sasha.
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Michael S. Tsirkin @ 2011-07-03 10:30 UTC (permalink / raw)
To: Sasha Levin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <1309686419.2250.8.camel@sasha>
On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > Doesn't this basically just revert
> > bf50e69f63d21091e525185c3ae761412be0ba72?
> >
>
> Yes. I haven't noticed that commit.
>
> Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> unnecessary delay due to having to kick and wait for the host to process
> the deflate vq before actually using the memory pages.
OTOH, as the commit points out:
Without this feature, we might free a page (and have another
user touch it) while the hypervisor is unprepared for it
Are you doing so many deflates that the speed is important?
> I've stumbled on it when writing the virtio_balloon driver for kvm
> tools. Initially my plan was to ignore the deflate vq completely, since
> the virtio spec says that unless we set the MUST_TELL_HOST flag,
> "deflation advice is merely a courtesy".
>
> I've noticed that if I don't process the deflate vq the guest doesn't
> use the deflated pages at all - something which contradicts the feature
> that lets him use the deflated pages without waiting for the deflate vq.
OK, so you have a host that does not process the vq.
As a result tell_host blocks forever.
With your patch, guest will block after releasing the
page, so the next deflate request will get stuck.
This doesn't seem to be a huge improvement so
we can conclude such a host is broken?
> >
> > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > Unless the host requires that requested pages won't be used until
> > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > deflating the balloon.
> > >
> > > This will avoid having to take an exit before actually using the pages.
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > ---
> > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index e058ace..055f95d 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > vb->num_pages--;
> > > }
> > >
> > > -
> > > /*
> > > - * Note that if
> > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > - * is true, we *have* to do it in this order
> > > + * If the host doesn't require us to notify him before using
> > > + * pages which belong to the balloon, update him only after
> > > + * freeing those pages for guest use.
> > > */
> > > - tell_host(vb, vb->deflate_vq);
> > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > + tell_host(vb, vb->deflate_vq);
> > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > + } else {
> > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > + tell_host(vb, vb->deflate_vq);
> > > + }
> > > }
> > >
> > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > --
> > > 1.7.6
>
>
> --
>
> Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Sasha Levin @ 2011-07-03 9:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, linux-kernel, dave, Pekka Enberg, Amit Shah, virtualization
In-Reply-To: <20110703082748.GD13183@redhat.com>
On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> Doesn't this basically just revert
> bf50e69f63d21091e525185c3ae761412be0ba72?
>
Yes. I haven't noticed that commit.
Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
unnecessary delay due to having to kick and wait for the host to process
the deflate vq before actually using the memory pages.
I've stumbled on it when writing the virtio_balloon driver for kvm
tools. Initially my plan was to ignore the deflate vq completely, since
the virtio spec says that unless we set the MUST_TELL_HOST flag,
"deflation advice is merely a courtesy".
I've noticed that if I don't process the deflate vq the guest doesn't
use the deflated pages at all - something which contradicts the feature
that lets him use the deflated pages without waiting for the deflate vq.
>
> On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > Unless the host requires that requested pages won't be used until
> > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > deflating the balloon.
> >
> > This will avoid having to take an exit before actually using the pages.
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > 1 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index e058ace..055f95d 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > vb->num_pages--;
> > }
> >
> > -
> > /*
> > - * Note that if
> > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > - * is true, we *have* to do it in this order
> > + * If the host doesn't require us to notify him before using
> > + * pages which belong to the balloon, update him only after
> > + * freeing those pages for guest use.
> > */
> > - tell_host(vb, vb->deflate_vq);
> > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > + tell_host(vb, vb->deflate_vq);
> > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > + } else {
> > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > + tell_host(vb, vb->deflate_vq);
> > + }
> > }
> >
> > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > --
> > 1.7.6
--
Sasha.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Michael S. Tsirkin @ 2011-07-03 8:27 UTC (permalink / raw)
To: Sasha Levin; +Cc: kvm, linux-kernel, dave, Amit Shah, virtualization
In-Reply-To: <1309576016-22800-1-git-send-email-levinsasha928@gmail.com>
Doesn't this basically just revert
bf50e69f63d21091e525185c3ae761412be0ba72?
On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> Unless the host requires that requested pages won't be used until
> he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> deflating the balloon.
>
> This will avoid having to take an exit before actually using the pages.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
> drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index e058ace..055f95d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> vb->num_pages--;
> }
>
> -
> /*
> - * Note that if
> - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> - * is true, we *have* to do it in this order
> + * If the host doesn't require us to notify him before using
> + * pages which belong to the balloon, update him only after
> + * freeing those pages for guest use.
> */
> - tell_host(vb, vb->deflate_vq);
> - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> + tell_host(vb, vb->deflate_vq);
> + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + } else {
> + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + tell_host(vb, vb->deflate_vq);
> + }
> }
>
> static inline void update_stat(struct virtio_balloon *vb, int idx,
> --
> 1.7.6
^ permalink raw reply
* [PATCH] virtio_balloon: Notify guest only after deflating the balloon
From: Sasha Levin @ 2011-07-02 3:06 UTC (permalink / raw)
To: linux-kernel; +Cc: virtualization, Sasha Levin, kvm, Michael S. Tsirkin
Unless the host requires that requested pages won't be used until
he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
deflating the balloon.
This will avoid having to take an exit before actually using the pages.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
drivers/virtio/virtio_balloon.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index e058ace..055f95d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
vb->num_pages--;
}
-
/*
- * Note that if
- * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
- * is true, we *have* to do it in this order
+ * If the host doesn't require us to notify him before using
+ * pages which belong to the balloon, update him only after
+ * freeing those pages for guest use.
*/
- tell_host(vb, vb->deflate_vq);
- release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
+ tell_host(vb, vb->deflate_vq);
+ release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ } else {
+ release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ tell_host(vb, vb->deflate_vq);
+ }
}
static inline void update_stat(struct virtio_balloon *vb, int idx,
--
1.7.6
^ permalink raw reply related
* Re: [PATCH 7/7] [v6] drivers/virt: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-07-01 17:12 UTC (permalink / raw)
To: Tabi Timur-B04825
Cc: konrad.wilk@oracle.com, benh@kernel.crashing.org,
linux-kernel@vger.kernel.org, cmetcalf@tilera.com,
dsaxena@linaro.org, linux-console@vger.kernel.org, greg@kroah.com,
Gala Kumar-B11780, virtualization@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, alan@lxorguk.ukuu.org.uk
In-Reply-To: <BANLkTi=3r=UQYJVqRLRe-eu7rdkXQS-ESQ@mail.gmail.com>
On Friday 01 July 2011, Tabi Timur-B04825 wrote:
> On Thu, Jun 9, 2011 at 4:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 09 June 2011 22:52:06 Timur Tabi wrote:
> >> Add the drivers/virt directory, which houses drivers that support
> >> virtualization environments, and add the Freescale hypervisor management
> >> driver.
> ....
> >>
> >> Signed-off-by: Timur Tabi <timur@freescale.com>
> >
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>
> So I've made the changes that people have asked for, and Arnd has
> acked this driver. Who's going to pick it up? Who will be the
> maintainer for drivers/virt? I'd really like to see this driver in
> 3.1, and there's not much time left.
>
I'd say merge it through the powerpc tree, and list yourself for
the driver, but not the entire directory.
Arnd
^ permalink raw reply
* RE: [PATCH 00/40] Staging: hv: Driver cleanup
From: KY Srinivasan @ 2011-07-01 13:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110701082136.GE28322@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Friday, July 01, 2011 4:22 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup
>
> On Thu, Jun 30, 2011 at 11:28:27PM +0000, KY Srinivasan wrote:
> > > On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote:
> > > > Further cleanup of the hv drivers:
> > > >
> > > > 1) Cleanup the reference counting mess for both stor and net devices.
> > >
> > > I really don't understand the need for reference counting on the storage
> > > side, especially now that you only have a SCSI driver. The SCSI
> > > midlayer does proper counting on it's objects (Scsi_Host, scsi_device,
> > > scsi_cmnd), so you'll get that for free given that SCSI drivers just
> > > piggyback on the midlayer lifetime rules.
> >
> > The reference counting allows us to properly deal with messages coming back
> from the host
> > to the guest with a racing remove of the device. I am told these messages could
> potentially be
> > not a response to a message sent from the guest.
>
> To deal with that the scsi subsystem has a two stage teardown for the
> SCSI host. First you call scsi_remove_host, at which point no new I/O
> to it can be started. After that you can wait for all outstanding
> messages, and finally you do a scsi_host_put to drop the long-term
> reference to it, and eventually free it.
That is what I have currently implemented both on the stor as well
as the net side with regards to the device specific objects hanging off of
struct hv_device (the ext field). If it is ok with you, I want to do what you
had suggested earlier. Keep the patches as they are with regards to
reference counting for now. We can always simplify the code later.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 24/40] Staging: hv: storvsc: On I/O get the correct IDE device
From: Christoph Hellwig @ 2011-07-01 13:27 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD),
Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480816C460@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Fri, Jul 01, 2011 at 01:01:43PM +0000, KY Srinivasan wrote:
> Nothing I could see from whatever testing I did. However, the guest will have
> Scsi hosts that the user did not configure. This is certainly the case in both
> approaches - just the numbers are different!
If the IDE drivers were real IDE ones you'd also have one Scsi_Host for
each of the as that's how libata handles in. In a way you're much
closer to real ATA than with the previous block driver. If your host
side support the ATA passthrough SCSI cdbs you'll even be able to
pass taskfiles to them and use tools like hdparm in the guest.
^ permalink raw reply
* RE: [PATCH 00/40] Staging: hv: Driver cleanup
From: KY Srinivasan @ 2011-07-01 13:25 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Christoph Hellwig, devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20110630214512.1b9125b7@nehalam.ftrdhcpuser.net>
> -----Original Message-----
> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> Sent: Friday, July 01, 2011 12:45 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; devel@linuxdriverproject.org; gregkh@suse.de; linux-
> kernel@vger.kernel.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup
>
> On Fri, 1 Jul 2011 00:19:38 +0000
> KY Srinivasan <kys@microsoft.com> wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> > > Sent: Thursday, June 30, 2011 7:48 PM
> > > To: KY Srinivasan
> > > Cc: Christoph Hellwig; devel@linuxdriverproject.org; gregkh@suse.de; linux-
> > > kernel@vger.kernel.org; virtualization@lists.osdl.org
> > > Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup
> > >
> > > On Thu, 30 Jun 2011 23:32:34 +0000
> > > KY Srinivasan <kys@microsoft.com> wrote:
> > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Christoph Hellwig [mailto:hch@infradead.org]
> > > > > Sent: Thursday, June 30, 2011 3:34 PM
> > > > > To: KY Srinivasan
> > > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > > > Subject: Re: [PATCH 00/40] Staging: hv: Driver cleanup
> > > > >
> > > > > On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote:
> > > > > > Further cleanup of the hv drivers:
> > > > > >
> > > > > > 1) Cleanup the reference counting mess for both stor and net
> devices.
> > > > >
> > > > > I really don't understand the need for reference counting on the storage
> > > > > side, especially now that you only have a SCSI driver. The SCSI
> > > > > midlayer does proper counting on it's objects (Scsi_Host, scsi_device,
> > > > > scsi_cmnd), so you'll get that for free given that SCSI drivers just
> > > > > piggyback on the midlayer lifetime rules.
> > > > >
> > > > > For now your patches should probably go in as-is, but mid-term you
> > > > > should be able to completely remove that code on the storage side.
> > > > >
> > > >
> > > > Greg,
> > > >
> > > > I am thinking of going back to my original implementation where I had one
> scsi
> > > host
> > > > per IDE device. This will certainly simply the code. Let me know what you
> think.
> > > If you
> > > > agree with this approach, please drop this patch-set, I will send you a new
> set
> > > of patches.
> > >
> > > I think there ref counting on network devices is also unneeded
> > > as long as the unregister logic handles RCU correctly. The network layer
> > > calls the driver unregister routine after all packets are gone.
> > On the networking side, what about incoming packets that may be racing
> > with the device destruction. The current ref counting scheme deals with
> > that case.
>
> Not sure how HV driver tells hypervisor to stop sending packets. But the
> destructor is not called until after all other CPU's are done processing
> packets from that device.
The issue I was concerned is one where, on packet reception, we need
to de-reference the ext field in the struct hv_device (this is the pointer
to the net device and this could happen concurrently with the guest
trying to shut-down the device. In the current code we deal with this
condition.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 24/40] Staging: hv: storvsc: On I/O get the correct IDE device
From: KY Srinivasan @ 2011-07-01 13:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD),
Hank Janssen
In-Reply-To: <20110701081725.GC28322@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Friday, July 01, 2011 4:17 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 24/40] Staging: hv: storvsc: On I/O get the correct IDE device
>
> On Thu, Jun 30, 2011 at 09:15:54PM +0000, KY Srinivasan wrote:
> > That is what I did initially. Then looking at the way we were handling scsi
> devices
> > where each scsi controller configured for the guest results in an emulated HBA
> > (scsi host) in the guest and all the block devices under a given controller are
> > handled through this one host, I decided to mimic a similar structure - one
> > scsi host for all the block devices configured as an IDE device.
> >
> > I can go back to my earlier implementation with one host per disk.
>
> Was there any downside you noticed from using one host per emulated
> disk?
Nothing I could see from whatever testing I did. However, the guest will have
Scsi hosts that the user did not configure. This is certainly the case in both
approaches - just the numbers are different!
K. Y
^ permalink raw reply
* RE: [PATCH 34/40] Staging: hv: storvsc: Add the contents of hyperv_storage.h to storvsc_drv.c
From: KY Srinivasan @ 2011-07-01 12:48 UTC (permalink / raw)
To: Christoph Hellwig
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD),
Hank Janssen
In-Reply-To: <20110701081601.GB28322@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Friday, July 01, 2011 4:16 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> Abhishek Kane (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: Re: [PATCH 34/40] Staging: hv: storvsc: Add the contents of
> hyperv_storage.h to storvsc_drv.c
>
> On Thu, Jun 30, 2011 at 08:13:51PM +0000, KY Srinivasan wrote:
> > > > Add the contents of hyperv_storage.h to storvsc_drv.c and cleanup
> > > storvsc_drv.c.n
> > >
> > > I'd at least leave the first half of the header that defines the
> > > protocol around.
> >
> > I only got rid of the block comment at the start of hyperv_storage.h
> > and consolidated the include files. Nothing of substance was deleted.
>
> I meant to say keeping a separate header file for the protocol defintion
> might be a good idea. That way it is kept separate from the
> implementation details
>
Ok; I get it. Although, a while ago as I was cleaning up the header files
In preparation to exiting staging, Greg had indicated that each driver
should, if possible be a single file. I merged the relevant files to comply
with that directive.
Regards,
K. Y
^ permalink raw reply
* Re: virtio scsi host draft specification, v3
From: Paolo Bonzini @ 2011-07-01 8:35 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Christoph Hellwig, Stefan Hajnoczi, kvm, Michael S. Tsirkin,
qemu-devel, Linux Kernel Mailing List, Linux Virtualization
In-Reply-To: <4E0D73C4.5090608@suse.de>
On 07/01/2011 09:14 AM, Hannes Reinecke wrote:
> Actually, the kernel does _not_ do a LUN remapping.
Not the kernel, the in-kernel target. The in-kernel target can and will
map hardware LUNs (target_lun in drivers/target/*) to arbitrary LUNs
(mapped_lun).
>> Put in another way: the virtio-scsi device is itself a SCSI
>> target,
>
> Argl. No way. The virtio-scsi device has to map to a single LUN.
I think we are talking about different things. By "virtio-scsi device"
I meant the "virtio-scsi HBA". When I referred to a LUN as seen by the
guest, I was calling it a "virtual SCSI device". So yes, we were
calling things with different names. Perhaps from now on
we can call them virtio-scsi {initiator,target,LUN} and have no
ambiguity? I'll also modify the spec in this sense.
> The SCSI spec itself only deals with LUNs, so anything you'll read in
> there obviously will only handle the interaction between the
> initiator (read: host) and the LUN itself. However, the actual
> command is send via an intermediat target, hence you'll always see
> the reference to the ITL (initiator-target-lun) nexus.
Yes, this I understand.
> The SCSI spec details discovery of the individual LUNs presented by a
> given target, it does _NOT_ detail the discovery of the targets
> themselves. That is being delegated to the underlying transport
And in fact I have this in virtio-scsi too, since virtio-scsi _is_ a
transport:
When VIRTIO_SCSI_EVT_RESET_REMOVED or VIRTIO_SCSI_EVT_RESET_RESCAN
is sent for LUN 0, the driver should ask the initiator to rescan
the target, in order to detect the case when an entire target has
appeared or disappeared.
[If the device fails] to report an event due to missing buffers,
[...] the driver should poll the logical units for unit attention
conditions, and/or do whatever form of bus scan is appropriate for
the guest operating system.
> In the case of NPIV it would make sense to map the virtual SCSI host
> to the backend, so that all devices presented to the virtual SCSI
> host will be presented to the backend, too. However, when doing so
> these devices will normally be referenced by their original LUN, as
> these will be presented to the guest via eg 'REPORT LUNS'.
Right.
> The above thread now tries to figure out if we should remap those LUN
> numbers or just expose them as they are. If we decide on remapping,
> we have to emulate _all_ commands referring explicitely to those LUN
> numbers (persistent reservations, anyone?).
But it seems to me that commands referring explicitly to LUN numbers
most likely have to be reimplemented anyway for virtualization. I'm
thinking exactly of persistent reservations. If two guests on the same
host try a persistent reservation, they should conflict with each other.
If reservation commands were just passed through, they would be seen
as coming from the same initiator (the HBA driver or iSCSI initiator in
the host OS).
etc.
> If we don't, we would expose some hardware detail to the guest, but
> would save us _a lot_ of processing.
But can we afford it? And would the architecture allow that at all?
Paolo
^ permalink raw reply
* Re: [PATCH 00/40] Staging: hv: Driver cleanup
From: Christoph Hellwig @ 2011-07-01 8:21 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480816C36D@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Thu, Jun 30, 2011 at 11:28:27PM +0000, KY Srinivasan wrote:
> > On Wed, Jun 29, 2011 at 07:38:21AM -0700, K. Y. Srinivasan wrote:
> > > Further cleanup of the hv drivers:
> > >
> > > 1) Cleanup the reference counting mess for both stor and net devices.
> >
> > I really don't understand the need for reference counting on the storage
> > side, especially now that you only have a SCSI driver. The SCSI
> > midlayer does proper counting on it's objects (Scsi_Host, scsi_device,
> > scsi_cmnd), so you'll get that for free given that SCSI drivers just
> > piggyback on the midlayer lifetime rules.
>
> The reference counting allows us to properly deal with messages coming back from the host
> to the guest with a racing remove of the device. I am told these messages could potentially be
> not a response to a message sent from the guest.
To deal with that the scsi subsystem has a two stage teardown for the
SCSI host. First you call scsi_remove_host, at which point no new I/O
to it can be started. After that you can wait for all outstanding
messages, and finally you do a scsi_host_put to drop the long-term
reference to it, and eventually free it.
^ permalink raw reply
* Re: [PATCH 23/40] Staging: hv: storvsc: Introduce code to manage IDE devices using storvsc HBA
From: Christoph Hellwig @ 2011-07-01 8:19 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD),
Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480816C2EF@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Thu, Jun 30, 2011 at 09:38:34PM +0000, KY Srinivasan wrote:
> > > +#define HV_MAX_IDE_DEVICES 4
> > > +#define HV_IDE_BASE_CHANNEL 10
> > > +#define HV_IDE0_DEV1 HV_IDE_BASE_CHANNEL
> > > +#define HV_IDE0_DEV2 (HV_IDE_BASE_CHANNEL + 1)
> > > +#define HV_IDE1_DEV1 (HV_IDE_BASE_CHANNEL + 2)
> > > +#define HV_IDE1_DEV2 (HV_IDE_BASE_CHANNEL + 3)
> >
> > This at last needs a good explanation of why these devices are called
> > IDE if they actually aren't. I know you've explained the reason to me
> > before, but it should also be in the code.
>
> These devices are configured as IDE devices for the guest. The current
> emulator supports 2 IDE controllers for a total of potentially 4 devices.
> I did this to support all these 4 devices under one scsi host and used the
> channel information to get at the correct device in the I/O path.
> So, if you go to a model with one host per device, this would not be required.
Either way the driver should have a nice comment somewhere explaining
why you have disks that are named IDE but handled spoken to using SCSI
with explicit discovery.
> > but more importanly what does path actually stand for here? Opencoding
> > this into the caller and adding proper comments explaining the scheme
> > might be more readable.
>
> In the blkvsc driver, the path/target info was used to properly identify the
> device - (a) the device was under the first or second IDE controller and (b)
> whether it is the first or second device under the controller.
Yeah, that's what I got from reading the code. What confuses me is the
"path" terminology which doesn't really map to any normal nomenclature.
^ permalink raw reply
* Re: [PATCH 24/40] Staging: hv: storvsc: On I/O get the correct IDE device
From: Christoph Hellwig @ 2011-07-01 8:17 UTC (permalink / raw)
To: KY Srinivasan
Cc: Abhishek Kane (Mindtree Consulting PVT LTD), Haiyang Zhang,
gregkh@suse.de, linux-kernel@vger.kernel.org, Christoph Hellwig,
virtualization@lists.osdl.org, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480816C2CF@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Thu, Jun 30, 2011 at 09:15:54PM +0000, KY Srinivasan wrote:
> That is what I did initially. Then looking at the way we were handling scsi devices
> where each scsi controller configured for the guest results in an emulated HBA
> (scsi host) in the guest and all the block devices under a given controller are
> handled through this one host, I decided to mimic a similar structure - one
> scsi host for all the block devices configured as an IDE device.
>
> I can go back to my earlier implementation with one host per disk.
Was there any downside you noticed from using one host per emulated
disk?
^ permalink raw reply
* Re: [PATCH 34/40] Staging: hv: storvsc: Add the contents of hyperv_storage.h to storvsc_drv.c
From: Christoph Hellwig @ 2011-07-01 8:16 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD),
Hank Janssen
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480816C297@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Thu, Jun 30, 2011 at 08:13:51PM +0000, KY Srinivasan wrote:
> > > Add the contents of hyperv_storage.h to storvsc_drv.c and cleanup
> > storvsc_drv.c.n
> >
> > I'd at least leave the first half of the header that defines the
> > protocol around.
>
> I only got rid of the block comment at the start of hyperv_storage.h
> and consolidated the include files. Nothing of substance was deleted.
I meant to say keeping a separate header file for the protocol defintion
might be a good idea. That way it is kept separate from the
implementation details
^ 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