* Re: [PATCH 0/5] Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-17 11:26 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel, Paolo Bonzini
In-Reply-To: <CAJSP0QW0tPV3WASGOBRE4hL=8zzSFQ6T=bHbEwMAzmHR9wnwJg@mail.gmail.com>
On Tue, Jul 17, 2012 at 12:11:15PM +0100, Stefan Hajnoczi wrote:
> On Tue, Jul 17, 2012 at 10:21 AM, Asias He <asias@redhat.com> wrote:
> > On 07/17/2012 04:52 PM, Paolo Bonzini wrote:
> >>
> >> Il 17/07/2012 10:29, Asias He ha scritto:
> >>>
> >>> So, vhost-blk at least saves ~6 syscalls for us in each request.
> >>
> >>
> >> Are they really 6? If I/O is coalesced by a factor of 3, for example
> >> (i.e. each exit processes 3 requests), it's really 2 syscalls per request.
> >
> >
> > Well. I am counting the number of syscalls in one notify and response
> > process. Sure the IO can be coalesced.
>
> Linux AIO also supports batching in io_submit() and io_getevents().
> Depending on the request pattern in the vring when you process it, you
> should be able to do better than 1 set of syscalls per host I/O
> request.
>
> Are you taking advantage of that at the moment in your userspace benchmark?
>
> Stefan
Injecting an interrupt directly from kernel bypasses two context switches.
Yes some worloads can coalesce interrupts efficiently but others can't.
It is not really hard to speculate more.
Personally I don't understand where all this speculation leads us.
Are you guys disputing the measurements posted? If not would not
it be better if discussion focused on the amount of extra code versus
measured gain?
--
MST
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Stefan Hajnoczi @ 2012-07-17 11:36 UTC (permalink / raw)
To: Asias He
Cc: linux-aio, kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Benjamin LaHaise, Alexander Viro, linux-fsdevel
In-Reply-To: <50052276.2080906@redhat.com>
On Tue, Jul 17, 2012 at 9:29 AM, Asias He <asias@redhat.com> wrote:
> On 07/16/2012 07:58 PM, Stefan Hajnoczi wrote:
>> Does the vhost-blk implementation do anything fundamentally different
>> from userspace? Where is the overhead that userspace virtio-blk has?
>
>
>
> Currently, no. But we could play with bio directly in vhost-blk as Christoph
> suggested which could make the IO path from guest to host's real storage
> even shorter in vhost-blk.
Wait :). My point is that writing new code without systematically
investigating performance means that we're essentially throwing random
things and seeing what sticks.
Adding bio mode would make vhost-blk and kvmtool more different.
It'll probably make vhost-blk slightly faster but harder to compare
against kvmtool. It's easier to start profiling before making that
change.
The reason I said "special-purpose kernel module" is because kvmtool
could be suffering from a bottleneck that can be fixed. Other
userspace applications would also benefit from that fix - it would be
generally useful. Adding a vhost-blk kernel module works around this
but only benefits KVM specifically.
Stefan
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Stefan Hajnoczi @ 2012-07-17 11:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel, Paolo Bonzini
In-Reply-To: <20120717112645.GA9363@redhat.com>
On Tue, Jul 17, 2012 at 12:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Jul 17, 2012 at 12:11:15PM +0100, Stefan Hajnoczi wrote:
>> On Tue, Jul 17, 2012 at 10:21 AM, Asias He <asias@redhat.com> wrote:
>> > On 07/17/2012 04:52 PM, Paolo Bonzini wrote:
>> >>
>> >> Il 17/07/2012 10:29, Asias He ha scritto:
>> >>>
>> >>> So, vhost-blk at least saves ~6 syscalls for us in each request.
>> >>
>> >>
>> >> Are they really 6? If I/O is coalesced by a factor of 3, for example
>> >> (i.e. each exit processes 3 requests), it's really 2 syscalls per request.
>> >
>> >
>> > Well. I am counting the number of syscalls in one notify and response
>> > process. Sure the IO can be coalesced.
>>
>> Linux AIO also supports batching in io_submit() and io_getevents().
>> Depending on the request pattern in the vring when you process it, you
>> should be able to do better than 1 set of syscalls per host I/O
>> request.
>>
>> Are you taking advantage of that at the moment in your userspace benchmark?
>>
>> Stefan
>
> Injecting an interrupt directly from kernel bypasses two context switches.
> Yes some worloads can coalesce interrupts efficiently but others can't.
> It is not really hard to speculate more.
>
> Personally I don't understand where all this speculation leads us.
> Are you guys disputing the measurements posted? If not would not
> it be better if discussion focused on the amount of extra code versus
> measured gain?
5-15% is nice. But what causes the performance advantage?
Knowing the answer to that is important before anyone can say whether
this approach is good or not.
Stefan
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Stefan Hajnoczi @ 2012-07-17 11:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel, Paolo Bonzini
In-Reply-To: <CAJSP0QXPkrVvgA+1fhhwsB5J8oApOObK8D3fk_Lm0Bt3T+Wfuw@mail.gmail.com>
On Tue, Jul 17, 2012 at 12:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 17, 2012 at 12:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, Jul 17, 2012 at 12:11:15PM +0100, Stefan Hajnoczi wrote:
>>> On Tue, Jul 17, 2012 at 10:21 AM, Asias He <asias@redhat.com> wrote:
>>> > On 07/17/2012 04:52 PM, Paolo Bonzini wrote:
>>> >>
>>> >> Il 17/07/2012 10:29, Asias He ha scritto:
>>> >>>
>>> >>> So, vhost-blk at least saves ~6 syscalls for us in each request.
>>> >>
>>> >>
>>> >> Are they really 6? If I/O is coalesced by a factor of 3, for example
>>> >> (i.e. each exit processes 3 requests), it's really 2 syscalls per request.
>>> >
>>> >
>>> > Well. I am counting the number of syscalls in one notify and response
>>> > process. Sure the IO can be coalesced.
>>>
>>> Linux AIO also supports batching in io_submit() and io_getevents().
>>> Depending on the request pattern in the vring when you process it, you
>>> should be able to do better than 1 set of syscalls per host I/O
>>> request.
>>>
>>> Are you taking advantage of that at the moment in your userspace benchmark?
>>>
>>> Stefan
>>
>> Injecting an interrupt directly from kernel bypasses two context switches.
>> Yes some worloads can coalesce interrupts efficiently but others can't.
>> It is not really hard to speculate more.
>>
>> Personally I don't understand where all this speculation leads us.
>> Are you guys disputing the measurements posted? If not would not
>> it be better if discussion focused on the amount of extra code versus
>> measured gain?
>
> 5-15% is nice. But what causes the performance advantage?
To be clear, I suggest posting profiling results and explaining the
improvement over kvmtool.
Stefan
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-17 11:54 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel, Paolo Bonzini
In-Reply-To: <CAJSP0QXPkrVvgA+1fhhwsB5J8oApOObK8D3fk_Lm0Bt3T+Wfuw@mail.gmail.com>
On Tue, Jul 17, 2012 at 12:42:13PM +0100, Stefan Hajnoczi wrote:
> On Tue, Jul 17, 2012 at 12:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Jul 17, 2012 at 12:11:15PM +0100, Stefan Hajnoczi wrote:
> >> On Tue, Jul 17, 2012 at 10:21 AM, Asias He <asias@redhat.com> wrote:
> >> > On 07/17/2012 04:52 PM, Paolo Bonzini wrote:
> >> >>
> >> >> Il 17/07/2012 10:29, Asias He ha scritto:
> >> >>>
> >> >>> So, vhost-blk at least saves ~6 syscalls for us in each request.
> >> >>
> >> >>
> >> >> Are they really 6? If I/O is coalesced by a factor of 3, for example
> >> >> (i.e. each exit processes 3 requests), it's really 2 syscalls per request.
> >> >
> >> >
> >> > Well. I am counting the number of syscalls in one notify and response
> >> > process. Sure the IO can be coalesced.
> >>
> >> Linux AIO also supports batching in io_submit() and io_getevents().
> >> Depending on the request pattern in the vring when you process it, you
> >> should be able to do better than 1 set of syscalls per host I/O
> >> request.
> >>
> >> Are you taking advantage of that at the moment in your userspace benchmark?
> >>
> >> Stefan
> >
> > Injecting an interrupt directly from kernel bypasses two context switches.
> > Yes some worloads can coalesce interrupts efficiently but others can't.
> > It is not really hard to speculate more.
> >
> > Personally I don't understand where all this speculation leads us.
> > Are you guys disputing the measurements posted? If not would not
> > it be better if discussion focused on the amount of extra code versus
> > measured gain?
>
> 5-15% is nice. But what causes the performance advantage?
Well, check the number of interrupts. If it's high then that is
part of it.
> Knowing the answer to that is important before anyone can say whether
> this approach is good or not.
>
> Stefan
Why is it?
--
MST
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Stefan Hajnoczi @ 2012-07-17 12:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel, Paolo Bonzini
In-Reply-To: <20120717115450.GA9796@redhat.com>
On Tue, Jul 17, 2012 at 12:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> Knowing the answer to that is important before anyone can say whether
>> this approach is good or not.
>>
>> Stefan
>
> Why is it?
Because there might be a fix to kvmtool which closes the gap. It
would be embarassing if vhost-blk was pushed just because no one
looked into what is actually going on.
And on the flipside, hard evidence of an overhead that cannot be
resolved could be good reason to do more vhost devices in the future.
Either way, it's useful to do this before going further.
Stefan
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-17 12:48 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel, Paolo Bonzini
In-Reply-To: <CAJSP0QXwGuUaD9piktOptJasdpci_fA90d=vV4m0SuORMvc2=Q@mail.gmail.com>
On Tue, Jul 17, 2012 at 01:03:39PM +0100, Stefan Hajnoczi wrote:
> On Tue, Jul 17, 2012 at 12:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> Knowing the answer to that is important before anyone can say whether
> >> this approach is good or not.
> >>
> >> Stefan
> >
> > Why is it?
>
> Because there might be a fix to kvmtool which closes the gap. It
> would be embarassing if vhost-blk was pushed just because no one
> looked into what is actually going on.
Embarrasing to whom? Is someone working on an optimization that
makes the work in question redundant, with posting just around
the corner? Then maybe the thing to do is just wait a bit.
Or are there no specific plans even, just a vague "sometime
in the future someone might take Asias' patches and profile them see if
anything maybe sticks out"? Then maybe not making everyone
wait years for this will serve users better.
> And on the flipside, hard evidence of an overhead that cannot be
> resolved could be good reason to do more vhost devices in the future.
How can one have hard evidence of an overhead that cannot be resolved?
Any problem can be resolved in an infinite number of ways.
> Either way, it's useful to do this before going further.
>
> Stefan
I think each work should be discussed on its own merits. Maybe
vhost-blk is just well written. So? What is your conclusion?
--
MST
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Paolo Bonzini @ 2012-07-17 13:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel
In-Reply-To: <20120717124859.GA9883@redhat.com>
Il 17/07/2012 14:48, Michael S. Tsirkin ha scritto:
> On Tue, Jul 17, 2012 at 01:03:39PM +0100, Stefan Hajnoczi wrote:
>> On Tue, Jul 17, 2012 at 12:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> Knowing the answer to that is important before anyone can say whether
>>>> this approach is good or not.
>>>>
>>>> Stefan
>>>
>>> Why is it?
>>
>> Because there might be a fix to kvmtool which closes the gap. It
>> would be embarassing if vhost-blk was pushed just because no one
>> looked into what is actually going on.
>
> Embarrasing to whom? Is someone working on an optimization that
> makes the work in question redundant, with posting just around
> the corner? Then maybe the thing to do is just wait a bit.
Of course there is work going on to make QEMU perform better. Not sure
about lkvm.
>> And on the flipside, hard evidence of an overhead that cannot be
>> resolved could be good reason to do more vhost devices in the future.
>
> How can one have hard evidence of an overhead that cannot be resolved?
Since we do have two completely independent userspaces (lkvm and
data-plane QEMU), you can build up some compelling evidence of an
overhead that cannot be resolved in user space.
>> Either way, it's useful to do this before going further.
>
> I think each work should be discussed on its own merits. Maybe
> vhost-blk is just well written. So? What is your conclusion?
If it's just that vhost-blk is written well, my conclusion is that lkvm
people should look into improving their virtio-blk userspace. We take
hints from each other all the time, for example virtio-scsi will have
unlocked kick in 3.6.
Why can't vhost-* just get into staging, and we call it a day?
Paolo
^ permalink raw reply
* Re: [PATCH 0/5] Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-17 13:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-aio, kvm, linux-kernel, virtualization, Benjamin LaHaise,
Alexander Viro, linux-fsdevel
In-Reply-To: <50056275.6000407@redhat.com>
On Tue, Jul 17, 2012 at 03:02:45PM +0200, Paolo Bonzini wrote:
> Il 17/07/2012 14:48, Michael S. Tsirkin ha scritto:
> > On Tue, Jul 17, 2012 at 01:03:39PM +0100, Stefan Hajnoczi wrote:
> >> On Tue, Jul 17, 2012 at 12:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> Knowing the answer to that is important before anyone can say whether
> >>>> this approach is good or not.
> >>>>
> >>>> Stefan
> >>>
> >>> Why is it?
> >>
> >> Because there might be a fix to kvmtool which closes the gap. It
> >> would be embarassing if vhost-blk was pushed just because no one
> >> looked into what is actually going on.
> >
> > Embarrasing to whom? Is someone working on an optimization that
> > makes the work in question redundant, with posting just around
> > the corner? Then maybe the thing to do is just wait a bit.
>
> Of course there is work going on to make QEMU perform better.
> Not sure about lkvm.
>
> >> And on the flipside, hard evidence of an overhead that cannot be
> >> resolved could be good reason to do more vhost devices in the future.
> >
> > How can one have hard evidence of an overhead that cannot be resolved?
>
> Since we do have two completely independent userspaces (lkvm and
> data-plane QEMU), you can build up some compelling evidence of an
> overhead that cannot be resolved in user space.
OK, so what you are saying benchmark against data-plane QEMU?
I agree actually. Asias, any data?
> >> Either way, it's useful to do this before going further.
> >
> > I think each work should be discussed on its own merits. Maybe
> > vhost-blk is just well written. So? What is your conclusion?
>
> If it's just that vhost-blk is written well, my conclusion is that lkvm
> people should look into improving their virtio-blk userspace. We take
> hints from each other all the time, for example virtio-scsi will have
> unlocked kick in 3.6.
>
> Why can't vhost-* just get into staging, and we call it a day?
>
> Paolo
staging is not a destination. Even if we put a driver in staging it
won't stay there indefinitely if qemu-kvm is not using it,
something that doesn't seem to be addressed for vhost-blk yet.
--
MST
^ permalink raw reply
* Re: [Xen-devel] [PATCH-v2] xen: populate correct number of pages when across mem boundary
From: Konrad Rzeszutek Wilk @ 2012-07-17 14:45 UTC (permalink / raw)
To: zhenzhong.duan
Cc: jeremy, xen-devel, x86, Feng Jin, linux-kernel, virtualization,
mingo, David Vrabel, hpa, tglx
In-Reply-To: <4FFFDCD9.7030305@oracle.com>
On Fri, Jul 13, 2012 at 04:31:21PM +0800, zhenzhong.duan wrote:
> When populate pages across a mem boundary at bootup, the page count
> populated isn't correct. This is due to mem populated to non-mem
> region and ignored.
>
> Pfn range is also wrongly aligned when mem boundary isn't page aligned.
>
> -v2: If xen_do_chunk fail(populate), abort this chunk and any others.
> Suggested by David, thanks.
>
> For a dom0 booted with dom_mem=3368952K(0xcd9ff000-4k) dmesg diff is:
> [ 0.000000] Freeing 9e-100 pfn range: 98 pages freed
> [ 0.000000] 1-1 mapping on 9e->100
> [ 0.000000] 1-1 mapping on cd9ff->100000
> [ 0.000000] Released 98 pages of unused memory
> [ 0.000000] Set 206435 page(s) to 1-1 mapping
> -[ 0.000000] Populating cd9fe-cda00 pfn range: 1 pages added
> +[ 0.000000] Populating cd9fe-cd9ff pfn range: 1 pages added
> +[ 0.000000] Populating 100000-100061 pfn range: 97 pages added
> [ 0.000000] BIOS-provided physical RAM map:
> [ 0.000000] Xen: 0000000000000000 - 000000000009e000 (usable)
> [ 0.000000] Xen: 00000000000a0000 - 0000000000100000 (reserved)
> [ 0.000000] Xen: 0000000000100000 - 00000000cd9ff000 (usable)
> [ 0.000000] Xen: 00000000cd9ffc00 - 00000000cda53c00 (ACPI NVS)
> ...
> [ 0.000000] Xen: 0000000100000000 - 0000000100061000 (usable)
> [ 0.000000] Xen: 0000000100061000 - 000000012c000000 (unusable)
> ...
> [ 0.000000] MEMBLOCK configuration:
> ...
> -[ 0.000000] reserved[0x4] [0x000000cd9ff000-0x000000cd9ffbff], 0xc00 bytes
> -[ 0.000000] reserved[0x5] [0x00000100000000-0x00000100060fff], 0x61000 bytes
>
> Related xen memory layout:
> (XEN) Xen-e820 RAM map:
> (XEN) 0000000000000000 - 000000000009ec00 (usable)
> (XEN) 00000000000f0000 - 0000000000100000 (reserved)
> (XEN) 0000000000100000 - 00000000cd9ffc00 (usable)
>
> Signed-off-by: Zhenzhong Duan<zhenzhong.duan@oracle.com>
Please run this first through scripts/cleanpatch.pl and fix up the issues
and then resend.
Thanks!
> ---
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index a4790bf..ead8557 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -157,25 +157,24 @@ static unsigned long __init xen_populate_chunk(
> unsigned long dest_pfn;
>
> for (i = 0, entry = list; i< map_size; i++, entry++) {
> - unsigned long credits = credits_left;
> unsigned long s_pfn;
> unsigned long e_pfn;
> unsigned long pfns;
> long capacity;
>
> - if (credits<= 0)
> + if (credits_left<= 0)
> break;
>
> if (entry->type != E820_RAM)
> continue;
>
> - e_pfn = PFN_UP(entry->addr + entry->size);
> + e_pfn = PFN_DOWN(entry->addr + entry->size);
>
> /* We only care about E820 after the xen_start_info->nr_pages */
> if (e_pfn<= max_pfn)
> continue;
>
> - s_pfn = PFN_DOWN(entry->addr);
> + s_pfn = PFN_UP(entry->addr);
> /* If the E820 falls within the nr_pages, we want to start
> * at the nr_pages PFN.
> * If that would mean going past the E820 entry, skip it
> @@ -184,23 +183,19 @@ static unsigned long __init xen_populate_chunk(
> capacity = e_pfn - max_pfn;
> dest_pfn = max_pfn;
> } else {
> - /* last_pfn MUST be within E820_RAM regions */
> - if (*last_pfn&& e_pfn>= *last_pfn)
> - s_pfn = *last_pfn;
> capacity = e_pfn - s_pfn;
> dest_pfn = s_pfn;
> }
> - /* If we had filled this E820_RAM entry, go to the next one. */
> - if (capacity<= 0)
> - continue;
>
> - if (credits> capacity)
> - credits = capacity;
> + if (credits_left< capacity)
> + capacity = credits_left;
>
> - pfns = xen_do_chunk(dest_pfn, dest_pfn + credits, false);
> + pfns = xen_do_chunk(dest_pfn, dest_pfn + capacity, false);
> done += pfns;
> - credits_left -= pfns;
> *last_pfn = (dest_pfn + pfns);
> + if (pfns< capacity)
> + break;
> + credits_left -= pfns;
> }
> return done;
> }
> --
> 1.7.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Michael S. Tsirkin @ 2012-07-17 15:05 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Zhi Yong Wu,
Anthony Liguori, target-devel, linux-scsi, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1342041304-29728-1-git-send-email-nab@linux-iscsi.org>
On Wed, Jul 11, 2012 at 09:15:00PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Hi folks,
>
> The following is a RFC-v2 series of tcm_vhost target fabric driver code
> currently in-flight for-3.6 mainline code.
>
> After last week's developments along with the help of some new folks, the
> changelog v1 -> v2 so far looks like:
>
> *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
> *) Fix tv_cmd completion -> release SGL memory leak (nab)
> *) Fix sparse warnings for static variable usage (Fengguang Wu)
> *) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
> *) Convert to cmwq submission for I/O dispatch (nab + hch)
>
> Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
> scsi_host->max_target=0 that removes the need for virtio-scsi LLD to hardcode
> VirtIOSCSIConfig->max_id=1 in order to function with tcm_vhost.
>
> Note this series has been pushed into target-pending.git/for-next-merge, and
> should be getting picked up for tomorrow's linux-next build.
>
> Please let us know if you have any concerns and/or additional review feedback.
>
> Thank you!
It still seems not 100% clear whether this driver will have major
userspace using it. And if not, it would be very hard to support a driver
when recent userspace does not use it in the end.
I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
Then we don't commit to an ABI.
For this, you can add a separate Kconfig and source it from drivers/staging/Kconfig.
Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
> Nicholas Bellinger (2):
> vhost: Add vhost_scsi specific defines
> tcm_vhost: Initial merge for vhost level target fabric driver
>
> Stefan Hajnoczi (2):
> vhost: Separate vhost-net features from vhost features
> vhost: make vhost work queue visible
>
> drivers/vhost/Kconfig | 6 +
> drivers/vhost/Makefile | 1 +
> drivers/vhost/net.c | 4 +-
> drivers/vhost/tcm_vhost.c | 1609 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/tcm_vhost.h | 74 ++
> drivers/vhost/test.c | 4 +-
> drivers/vhost/vhost.c | 5 +-
> drivers/vhost/vhost.h | 6 +-
> include/linux/vhost.h | 9 +
> 9 files changed, 1710 insertions(+), 8 deletions(-)
> create mode 100644 drivers/vhost/tcm_vhost.c
> create mode 100644 drivers/vhost/tcm_vhost.h
>
> --
> 1.7.2.5
^ permalink raw reply
* Re: [PATCH RESEND 0/5] Add vhost-blk support
From: Michael S. Tsirkin @ 2012-07-17 15:09 UTC (permalink / raw)
To: Asias He
Cc: Jens Axboe, linux-aio, target-devel, linux-scsi, kvm,
linux-kernel, lf-virt, James Bottomley, Anthony Liguori,
Jeff Moyer, Benjamin LaHaise, Alexander Viro, linux-fsdevel,
Paolo Bonzini, Zhi Yong Wu, Christoph Hellwig, Stefan Hajnoczi
In-Reply-To: <1342169711-12386-1-git-send-email-asias@redhat.com>
On Fri, Jul 13, 2012 at 04:55:06PM +0800, Asias He wrote:
>
> Hi folks,
>
> [I am resending to fix the broken thread in the previous one.]
>
> This patchset adds vhost-blk support. vhost-blk is a in kernel virito-blk
> device accelerator. Compared to userspace virtio-blk implementation, vhost-blk
> gives about 5% to 15% performance improvement.
Same thing as tcm_host comment:
It seems not 100% clear whether this driver will have major
userspace using it. And if not, it would be very hard to support a
driver when recent userspace does not use it in the end.
I think a good idea for 3.6 would be to make it depend on
CONFIG_STAGING. Then we don't commit to an ABI. For this, you can add
a separate Kconfig and source it from drivers/staging/Kconfig. Maybe it
needs to be in a separate directory drivers/vhost/staging/Kconfig.
I Cc'd the list of tcm_host in the hope that you can cooperate on this.
--
MST
^ permalink raw reply
* Re: [PATCH 01/15] Drivers: hv: Format GUIDS as per MSFT standards
From: Greg KH @ 2012-07-17 16:04 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: linux-kernel, devel, virtualization, olaf, apw
In-Reply-To: <1342298060-12662-1-git-send-email-kys@microsoft.com>
On Sat, Jul 14, 2012 at 01:34:06PM -0700, K. Y. Srinivasan wrote:
> Format GUIDS as per MSFT standard. This makes interacting with MSFT
> tool stack easier.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/hv/vmbus_drv.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index a220e57..1f7e54a 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -147,7 +147,7 @@ static ssize_t vmbus_show_device_attr(struct device *dev,
>
> if (!strcmp(dev_attr->attr.name, "class_id")) {
> ret = sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
> - "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
> + "%02x%02x-%02x%02x%02x%02x%02x%02x}\n",
As Joe pointed out, please just use the printk modifier the kernel
already has for GUIDS, and don't roll your own here. That will work
properly for you, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] Drivers: hv: Format GUIDS as per MSFT standards
From: Joe Perches @ 2012-07-17 16:19 UTC (permalink / raw)
To: Greg KH; +Cc: olaf, linux-kernel, virtualization, apw, devel
In-Reply-To: <20120717160402.GA20481@kroah.com>
On Tue, 2012-07-17 at 09:04 -0700, Greg KH wrote:
> On Sat, Jul 14, 2012 at 01:34:06PM -0700, K. Y. Srinivasan wrote:
> > Format GUIDS as per MSFT standard. This makes interacting with MSFT
> > tool stack easier.
[]
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
[]
> > @@ -147,7 +147,7 @@ static ssize_t vmbus_show_device_attr(struct device *dev,
> >
> > if (!strcmp(dev_attr->attr.name, "class_id")) {
> > ret = sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
> > - "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
> > + "%02x%02x-%02x%02x%02x%02x%02x%02x}\n",
>
> As Joe pointed out, please just use the printk modifier the kernel
> already has for GUIDS, and don't roll your own here. That will work
> properly for you, right?
%pU became available in 2.6.33
I think one of their support targets is RHEL6 which
I believe is 2.6.32+.
^ permalink raw reply
* Re: [PATCH 01/15] Drivers: hv: Format GUIDS as per MSFT standards
From: Greg KH @ 2012-07-17 16:32 UTC (permalink / raw)
To: Joe Perches; +Cc: olaf, linux-kernel, virtualization, apw, devel
In-Reply-To: <1342541966.15318.4.camel@joe2Laptop>
On Tue, Jul 17, 2012 at 09:19:26AM -0700, Joe Perches wrote:
> On Tue, 2012-07-17 at 09:04 -0700, Greg KH wrote:
> > On Sat, Jul 14, 2012 at 01:34:06PM -0700, K. Y. Srinivasan wrote:
> > > Format GUIDS as per MSFT standard. This makes interacting with MSFT
> > > tool stack easier.
> []
> > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> []
> > > @@ -147,7 +147,7 @@ static ssize_t vmbus_show_device_attr(struct device *dev,
> > >
> > > if (!strcmp(dev_attr->attr.name, "class_id")) {
> > > ret = sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
> > > - "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
> > > + "%02x%02x-%02x%02x%02x%02x%02x%02x}\n",
> >
> > As Joe pointed out, please just use the printk modifier the kernel
> > already has for GUIDS, and don't roll your own here. That will work
> > properly for you, right?
>
> %pU became available in 2.6.33
>
> I think one of their support targets is RHEL6 which
> I believe is 2.6.32+.
That's nice, but it's nothing that I care about for in-kernel code, nor
should anyone else.
greg k-h
^ permalink raw reply
* [PATCH v4 0/3] make balloon pages movable by compaction
From: Rafael Aquini @ 2012-07-17 16:50 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton, Rafael Aquini
Memory fragmentation introduced by ballooning might reduce significantly
the number of 2MB contiguous memory blocks that can be used within a guest,
thus imposing performance penalties associated with the reduced number of
transparent huge pages that could be used by the guest workload.
This patch-set follows the main idea discussed at 2012 LSFMMS session:
"Ballooning for transparent huge pages" -- http://lwn.net/Articles/490114/
to introduce the required changes to the virtio_balloon driver, as well as
the changes to the core compaction & migration bits, in order to make those
subsystems aware of ballooned pages and allow memory balloon pages become
movable within a guest, thus avoiding the aforementioned fragmentation issue
Rafael Aquini (3):
mm: introduce compaction and migration for virtio ballooned pages
virtio_balloon: introduce migration primitives to balloon pages
mm: add vm event counters for balloon pages compaction
drivers/virtio/virtio_balloon.c | 146 ++++++++++++++++++++++++++++++++++++---
include/linux/mm.h | 15 ++++
include/linux/virtio_balloon.h | 4 ++
include/linux/vm_event_item.h | 2 +
mm/compaction.c | 127 +++++++++++++++++++++++++++++-----
mm/migrate.c | 32 ++++++++-
mm/vmstat.c | 4 ++
7 files changed, 302 insertions(+), 28 deletions(-)
Change log:
v4:
* address Rusty Russel's review comments on PATCH 02;
* re-base virtio_balloon patch on 9c378abc5c0c6fc8e3acf5968924d274503819b3;
V3:
* address reviewers nitpick suggestions on PATCH 01 (Mel, Minchan);
V2:
* address Mel Gorman's review comments on PATCH 01;
Preliminary test results:
(2 VCPU 1024mB RAM KVM guest running 3.5.0_rc7+ -- after a reboot)
* 64mB balloon:
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 0
compact_pages_moved 0
compact_pagemigrate_failed 0
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 0
compact_balloon_failed 0
compact_balloon_isolated 0
compact_balloon_freed 0
[root@localhost ~]#
[root@localhost ~]# for i in $(seq 1 4); do echo 1 > /proc/sys/vm/compact_memory & done &>/dev/null
[1] Done echo 1 > /proc/sys/vm/compact_memory
[2] Done echo 1 > /proc/sys/vm/compact_memory
[3]- Done echo 1 > /proc/sys/vm/compact_memory
[4]+ Done echo 1 > /proc/sys/vm/compact_memory
[root@localhost ~]#
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 2842
compact_pages_moved 48429
compact_pagemigrate_failed 81
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 16364
compact_balloon_failed 0
compact_balloon_isolated 16364
compact_balloon_freed 16364
* 128mB balloon:
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 0
compact_pages_moved 0
compact_pagemigrate_failed 0
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 0
compact_balloon_failed 0
compact_balloon_isolated 0
compact_balloon_freed 0
[root@localhost ~]#
[root@localhost ~]# for i in $(seq 1 4); do echo 1 > /proc/sys/vm/compact_memory & done &>/dev/null
[1] Done echo 1 > /proc/sys/vm/compact_memory
[2] Done echo 1 > /proc/sys/vm/compact_memory
[3]- Done echo 1 > /proc/sys/vm/compact_memory
[4]+ Done echo 1 > /proc/sys/vm/compact_memory
[root@localhost ~]#
[root@localhost ~]# awk '/compact/ {print}' /proc/vmstat
compact_blocks_moved 2162
compact_pages_moved 33735
compact_pagemigrate_failed 69
compact_stall 0
compact_fail 0
compact_success 0
compact_balloon_migrated 21731
compact_balloon_failed 0
compact_balloon_isolated 21731
compact_balloon_freed 21731
--
1.7.10.4
^ permalink raw reply
* [PATCH v4 1/3] mm: introduce compaction and migration for virtio ballooned pages
From: Rafael Aquini @ 2012-07-17 16:50 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton, Rafael Aquini
In-Reply-To: <cover.1342485774.git.aquini@redhat.com>
This patch introduces the helper functions as well as the necessary changes
to teach compaction and migration bits how to cope with pages which are
part of a guest memory balloon, in order to make them movable by memory
compaction procedures.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
include/linux/mm.h | 15 +++++++
mm/compaction.c | 126 ++++++++++++++++++++++++++++++++++++++++++++--------
mm/migrate.c | 30 ++++++++++++-
3 files changed, 151 insertions(+), 20 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..3112198 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1629,5 +1629,20 @@ static inline unsigned int debug_guardpage_minorder(void) { return 0; }
static inline bool page_is_guard(struct page *page) { return false; }
#endif /* CONFIG_DEBUG_PAGEALLOC */
+#if (defined(CONFIG_VIRTIO_BALLOON) || \
+ defined(CONFIG_VIRTIO_BALLOON_MODULE)) && defined(CONFIG_COMPACTION)
+extern bool putback_balloon_page(struct page *);
+extern struct address_space *balloon_mapping;
+
+static inline bool is_balloon_page(struct page *page)
+{
+ return (page->mapping == balloon_mapping) ? true : false;
+}
+#else
+static inline bool is_balloon_page(struct page *page) { return false; }
+static inline bool isolate_balloon_page(struct page *page) { return false; }
+static inline bool putback_balloon_page(struct page *page) { return false; }
+#endif /* (VIRTIO_BALLOON || VIRTIO_BALLOON_MODULE) && COMPACTION */
+
#endif /* __KERNEL__ */
#endif /* _LINUX_MM_H */
diff --git a/mm/compaction.c b/mm/compaction.c
index 2f42d95..51eac0c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -14,6 +14,7 @@
#include <linux/backing-dev.h>
#include <linux/sysctl.h>
#include <linux/sysfs.h>
+#include <linux/export.h>
#include "internal.h"
#if defined CONFIG_COMPACTION || defined CONFIG_CMA
@@ -21,6 +22,85 @@
#define CREATE_TRACE_POINTS
#include <trace/events/compaction.h>
+#if defined(CONFIG_VIRTIO_BALLOON) || defined(CONFIG_VIRTIO_BALLOON_MODULE)
+/*
+ * Balloon pages special page->mapping.
+ * Users must properly allocate and initialize an instance of balloon_mapping,
+ * and set it as the page->mapping for balloon enlisted page instances.
+ * There is no need on utilizing struct address_space locking schemes for
+ * balloon_mapping as, once it gets initialized at balloon driver, it will
+ * remain just like a static reference that helps us on identifying a guest
+ * ballooned page by its mapping, as well as it will keep the 'a_ops' callback
+ * pointers to the functions that will execute the balloon page mobility tasks.
+ *
+ * address_space_operations necessary methods for ballooned pages:
+ * .migratepage - used to perform balloon's page migration (as is)
+ * .invalidatepage - used to isolate a page from balloon's page list
+ * .freepage - used to reinsert an isolated page to balloon's page list
+ */
+struct address_space *balloon_mapping;
+EXPORT_SYMBOL_GPL(balloon_mapping);
+
+static inline void __isolate_balloon_page(struct page *page)
+{
+ page->mapping->a_ops->invalidatepage(page, 0);
+}
+
+static inline void __putback_balloon_page(struct page *page)
+{
+ page->mapping->a_ops->freepage(page);
+}
+
+/* __isolate_lru_page() counterpart for a ballooned page */
+static bool isolate_balloon_page(struct page *page)
+{
+ if (WARN_ON(!is_balloon_page(page)))
+ return false;
+
+ if (likely(get_page_unless_zero(page))) {
+ /*
+ * We can race against move_to_new_page() & __unmap_and_move().
+ * If we stumble across a locked balloon page and succeed on
+ * isolating it, the result tends to be disastrous.
+ */
+ if (likely(trylock_page(page))) {
+ /*
+ * A ballooned page, by default, has just one refcount.
+ * Prevent concurrent compaction threads from isolating
+ * an already isolated balloon page.
+ */
+ if (is_balloon_page(page) && (page_count(page) == 2)) {
+ __isolate_balloon_page(page);
+ unlock_page(page);
+ return true;
+ }
+ unlock_page(page);
+ }
+ /* Drop refcount taken for this already isolated page */
+ put_page(page);
+ }
+ return false;
+}
+
+/* putback_lru_page() counterpart for a ballooned page */
+bool putback_balloon_page(struct page *page)
+{
+ if (WARN_ON(!is_balloon_page(page)))
+ return false;
+
+ if (likely(trylock_page(page))) {
+ if (is_balloon_page(page)) {
+ __putback_balloon_page(page);
+ put_page(page);
+ unlock_page(page);
+ return true;
+ }
+ unlock_page(page);
+ }
+ return false;
+}
+#endif /* CONFIG_VIRTIO_BALLOON || CONFIG_VIRTIO_BALLOON_MODULE */
+
static unsigned long release_freepages(struct list_head *freelist)
{
struct page *page, *next;
@@ -312,32 +392,40 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
continue;
}
- if (!PageLRU(page))
- continue;
-
/*
- * PageLRU is set, and lru_lock excludes isolation,
- * splitting and collapsing (collapsing has already
- * happened if PageLRU is set).
+ * It is possible to migrate LRU pages and balloon pages.
+ * Skip any other type of page.
*/
- if (PageTransHuge(page)) {
- low_pfn += (1 << compound_order(page)) - 1;
- continue;
- }
+ if (PageLRU(page)) {
+ /*
+ * PageLRU is set, and lru_lock excludes isolation,
+ * splitting and collapsing (collapsing has already
+ * happened if PageLRU is set).
+ */
+ if (PageTransHuge(page)) {
+ low_pfn += (1 << compound_order(page)) - 1;
+ continue;
+ }
- if (!cc->sync)
- mode |= ISOLATE_ASYNC_MIGRATE;
+ if (!cc->sync)
+ mode |= ISOLATE_ASYNC_MIGRATE;
- lruvec = mem_cgroup_page_lruvec(page, zone);
+ lruvec = mem_cgroup_page_lruvec(page, zone);
- /* Try isolate the page */
- if (__isolate_lru_page(page, mode) != 0)
- continue;
+ /* Try isolate the page */
+ if (__isolate_lru_page(page, mode) != 0)
+ continue;
+
+ VM_BUG_ON(PageTransCompound(page));
- VM_BUG_ON(PageTransCompound(page));
+ /* Successfully isolated */
+ del_page_from_lru_list(page, lruvec, page_lru(page));
+ } else if (is_balloon_page(page)) {
+ if (!isolate_balloon_page(page))
+ continue;
+ } else
+ continue;
- /* Successfully isolated */
- del_page_from_lru_list(page, lruvec, page_lru(page));
list_add(&page->lru, migratelist);
cc->nr_migratepages++;
nr_isolated++;
diff --git a/mm/migrate.c b/mm/migrate.c
index be26d5c..59c7bc5 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -78,7 +78,10 @@ void putback_lru_pages(struct list_head *l)
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
page_is_file_cache(page));
- putback_lru_page(page);
+ if (unlikely(is_balloon_page(page)))
+ WARN_ON(!putback_balloon_page(page));
+ else
+ putback_lru_page(page);
}
}
@@ -783,6 +786,17 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
}
}
+ if (is_balloon_page(page)) {
+ /*
+ * A ballooned page does not need any special attention from
+ * physical to virtual reverse mapping procedures.
+ * Skip any attempt to unmap PTEs or to remap swap cache,
+ * in order to avoid burning cycles at rmap level.
+ */
+ remap_swapcache = 0;
+ goto skip_unmap;
+ }
+
/*
* Corner case handling:
* 1. When a new swap-cache page is read into, it is added to the LRU
@@ -852,6 +866,20 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
goto out;
rc = __unmap_and_move(page, newpage, force, offlining, mode);
+
+ if (is_balloon_page(newpage)) {
+ /*
+ * A ballooned page has been migrated already. Now, it is the
+ * time to wrap-up counters, handle the old page back to Buddy
+ * and return.
+ */
+ list_del(&page->lru);
+ dec_zone_page_state(page, NR_ISOLATED_ANON +
+ page_is_file_cache(page));
+ put_page(page);
+ __free_page(page);
+ return rc;
+ }
out:
if (rc != -EAGAIN) {
/*
--
1.7.10.4
^ permalink raw reply related
* [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages
From: Rafael Aquini @ 2012-07-17 16:50 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton, Rafael Aquini
In-Reply-To: <cover.1342485774.git.aquini@redhat.com>
Besides making balloon pages movable at allocation time and introducing
the necessary primitives to perform balloon page migration/compaction,
this patch also introduces the following locking scheme to provide the
proper synchronization and protection for struct virtio_balloon elements
against concurrent accesses due to parallel operations introduced by
memory compaction / page migration.
- balloon_lock (mutex) : synchronizes the access demand to elements of
struct virtio_balloon and its queue operations;
- pages_lock (spinlock): special protection to balloon pages list against
concurrent list handling operations;
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
drivers/virtio/virtio_balloon.c | 145 ++++++++++++++++++++++++++++++++++++---
include/linux/virtio_balloon.h | 4 ++
2 files changed, 141 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0908e60..7669bc8 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -27,6 +27,7 @@
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/fs.h>
/*
* Balloon device works in 4K page units. So each page is pointed to by
@@ -35,6 +36,9 @@
*/
#define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
+/* Synchronize accesses/updates to the struct virtio_balloon elements */
+DEFINE_MUTEX(balloon_lock);
+
struct virtio_balloon
{
struct virtio_device *vdev;
@@ -51,6 +55,10 @@ struct virtio_balloon
/* Number of balloon pages we've told the Host we're not using. */
unsigned int num_pages;
+
+ /* Protect 'pages' list against concurrent handling */
+ spinlock_t pages_lock;
+
/*
* The pages we've told the Host we're not using.
* Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
@@ -125,10 +133,12 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ mutex_lock(&balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
- __GFP_NOMEMALLOC | __GFP_NOWARN);
+ struct page *page = alloc_page(GFP_HIGHUSER_MOVABLE |
+ __GFP_NORETRY | __GFP_NOWARN |
+ __GFP_NOMEMALLOC);
if (!page) {
if (printk_ratelimit())
dev_printk(KERN_INFO, &vb->vdev->dev,
@@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
totalram_pages--;
+ spin_lock(&vb->pages_lock);
list_add(&page->lru, &vb->pages);
+ page->mapping = balloon_mapping;
+ spin_unlock(&vb->pages_lock);
}
/* Didn't get any? Oh well. */
@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
return;
tell_host(vb, vb->inflate_vq);
+ mutex_unlock(&balloon_lock);
}
static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
@@ -169,10 +183,22 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
+ mutex_lock(&balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+ /*
+ * We can race against virtballoon_isolatepage() and end up
+ * stumbling across a _temporarily_ empty 'pages' list.
+ */
+ spin_lock(&vb->pages_lock);
+ if (unlikely(list_empty(&vb->pages))) {
+ spin_unlock(&vb->pages_lock);
+ break;
+ }
page = list_first_entry(&vb->pages, struct page, lru);
+ page->mapping = NULL;
list_del(&page->lru);
+ spin_unlock(&vb->pages_lock);
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
}
@@ -182,8 +208,11 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
* virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
* is true, we *have* to do it in this order
*/
- tell_host(vb, vb->deflate_vq);
- release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ if (vb->num_pfns > 0) {
+ tell_host(vb, vb->deflate_vq);
+ release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ }
+ mutex_unlock(&balloon_lock);
}
static inline void update_stat(struct virtio_balloon *vb, int idx,
@@ -239,6 +268,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
struct scatterlist sg;
unsigned int len;
+ mutex_lock(&balloon_lock);
vb->need_stats_update = 0;
update_balloon_stats(vb);
@@ -249,6 +279,7 @@ static void stats_handle_request(struct virtio_balloon *vb)
if (virtqueue_add_buf(vq, &sg, 1, 0, vb, GFP_KERNEL) < 0)
BUG();
virtqueue_kick(vq);
+ mutex_unlock(&balloon_lock);
}
static void virtballoon_changed(struct virtio_device *vdev)
@@ -261,22 +292,27 @@ static void virtballoon_changed(struct virtio_device *vdev)
static inline s64 towards_target(struct virtio_balloon *vb)
{
__le32 v;
- s64 target;
+ s64 target, actual;
+ mutex_lock(&balloon_lock);
+ actual = vb->num_pages;
vb->vdev->config->get(vb->vdev,
offsetof(struct virtio_balloon_config, num_pages),
&v, sizeof(v));
target = le32_to_cpu(v);
- return target - vb->num_pages;
+ mutex_unlock(&balloon_lock);
+ return target - actual;
}
static void update_balloon_size(struct virtio_balloon *vb)
{
- __le32 actual = cpu_to_le32(vb->num_pages);
-
+ __le32 actual;
+ mutex_lock(&balloon_lock);
+ actual = cpu_to_le32(vb->num_pages);
vb->vdev->config->set(vb->vdev,
offsetof(struct virtio_balloon_config, actual),
&actual, sizeof(actual));
+ mutex_unlock(&balloon_lock);
}
static int balloon(void *_vballoon)
@@ -339,6 +375,82 @@ static int init_vqs(struct virtio_balloon *vb)
return 0;
}
+/*
+ * Populate balloon_mapping->a_ops->migratepage method to perform the balloon
+ * page migration task.
+ *
+ * After a ballooned page gets isolated by compaction procedures, this is the
+ * function that performs the page migration on behalf of move_to_new_page(),
+ * when the last calls (page)->mapping->a_ops->migratepage.
+ *
+ * Page migration for virtio balloon is done in a simple swap fashion which
+ * follows these two steps:
+ * 1) insert newpage into vb->pages list and update the host about it;
+ * 2) update the host about the removed old page from vb->pages list;
+ */
+int virtballoon_migratepage(struct address_space *mapping,
+ struct page *newpage, struct page *page, enum migrate_mode mode)
+{
+ struct virtio_balloon *vb = (void *)mapping->backing_dev_info;
+
+ mutex_lock(&balloon_lock);
+
+ /* balloon's page migration 1st step */
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ spin_lock(&vb->pages_lock);
+ list_add(&newpage->lru, &vb->pages);
+ spin_unlock(&vb->pages_lock);
+ set_page_pfns(vb->pfns, newpage);
+ tell_host(vb, vb->inflate_vq);
+
+ /* balloon's page migration 2nd step */
+ vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
+ set_page_pfns(vb->pfns, page);
+ tell_host(vb, vb->deflate_vq);
+
+ mutex_unlock(&balloon_lock);
+
+ return 0;
+}
+
+/*
+ * Populate balloon_mapping->a_ops->invalidatepage method to help compaction on
+ * isolating a page from the balloon page list.
+ */
+void virtballoon_isolatepage(struct page *page, unsigned long mode)
+{
+ /*
+ * struct address_space.backing_dev_info specially holds the pointer
+ * to our struct virtio_balloon *vb
+ */
+ struct virtio_balloon *vb = (void *)page->mapping->backing_dev_info;
+ spin_lock(&vb->pages_lock);
+ list_del(&page->lru);
+ spin_unlock(&vb->pages_lock);
+}
+
+/*
+ * 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)
+{
+ /*
+ * struct address_space.backing_dev_info specially holds the pointer
+ * to our struct virtio_balloon *vb
+ */
+ struct virtio_balloon *vb = (void *)page->mapping->backing_dev_info;
+ spin_lock(&vb->pages_lock);
+ list_add(&page->lru, &vb->pages);
+ spin_unlock(&vb->pages_lock);
+}
+
+static const struct address_space_operations virtio_balloon_aops = {
+ .migratepage = virtballoon_migratepage,
+ .invalidatepage = virtballoon_isolatepage,
+ .freepage = virtballoon_putbackpage,
+};
+
static int virtballoon_probe(struct virtio_device *vdev)
{
struct virtio_balloon *vb;
@@ -351,12 +463,27 @@ static int virtballoon_probe(struct virtio_device *vdev)
}
INIT_LIST_HEAD(&vb->pages);
+ spin_lock_init(&vb->pages_lock);
+
vb->num_pages = 0;
init_waitqueue_head(&vb->config_change);
init_waitqueue_head(&vb->acked);
vb->vdev = vdev;
vb->need_stats_update = 0;
+ /* Init the ballooned page->mapping special balloon_mapping */
+ balloon_mapping = kmalloc(sizeof(*balloon_mapping), GFP_KERNEL);
+ if (!balloon_mapping) {
+ err = -ENOMEM;
+ goto out_free_vb;
+ }
+
+ INIT_RADIX_TREE(&balloon_mapping->page_tree, GFP_ATOMIC | __GFP_NOWARN);
+ INIT_LIST_HEAD(&balloon_mapping->i_mmap_nonlinear);
+ spin_lock_init(&balloon_mapping->tree_lock);
+ balloon_mapping->a_ops = &virtio_balloon_aops;
+ balloon_mapping->backing_dev_info = (void *)vb;
+
err = init_vqs(vb);
if (err)
goto out_free_vb;
@@ -373,6 +500,7 @@ out_del_vqs:
vdev->config->del_vqs(vdev);
out_free_vb:
kfree(vb);
+ kfree(balloon_mapping);
out:
return err;
}
@@ -397,6 +525,7 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
kthread_stop(vb->thread);
remove_common(vb);
kfree(vb);
+ kfree(balloon_mapping);
}
#ifdef CONFIG_PM
diff --git a/include/linux/virtio_balloon.h b/include/linux/virtio_balloon.h
index 652dc8b..930f1b7 100644
--- a/include/linux/virtio_balloon.h
+++ b/include/linux/virtio_balloon.h
@@ -56,4 +56,8 @@ struct virtio_balloon_stat {
u64 val;
} __attribute__((packed));
+#if !defined(CONFIG_COMPACTION)
+struct address_space *balloon_mapping;
+#endif
+
#endif /* _LINUX_VIRTIO_BALLOON_H */
--
1.7.10.4
^ permalink raw reply related
* [PATCH v4 3/3] mm: add vm event counters for balloon pages compaction
From: Rafael Aquini @ 2012-07-17 16:50 UTC (permalink / raw)
To: linux-mm
Cc: Rik van Riel, Rafael Aquini, Konrad Rzeszutek Wilk,
Michael S. Tsirkin, linux-kernel, virtualization, Minchan Kim,
Andi Kleen, Andrew Morton, Rafael Aquini
In-Reply-To: <cover.1342485774.git.aquini@redhat.com>
This patch is only for testing report purposes and shall be dropped in case of
the rest of this patchset getting accepted for merging.
Signed-off-by: Rafael Aquini <aquini@redhat.com>
---
drivers/virtio/virtio_balloon.c | 1 +
include/linux/vm_event_item.h | 2 ++
mm/compaction.c | 1 +
mm/migrate.c | 6 ++++--
mm/vmstat.c | 4 ++++
5 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 7669bc8..c4ddcd7 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -410,6 +410,7 @@ int virtballoon_migratepage(struct address_space *mapping,
mutex_unlock(&balloon_lock);
+ count_vm_event(COMPACTBALLOONMIGRATED);
return 0;
}
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 06f8e38..e330c5a 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -40,6 +40,8 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
#ifdef CONFIG_COMPACTION
COMPACTBLOCKS, COMPACTPAGES, COMPACTPAGEFAILED,
COMPACTSTALL, COMPACTFAIL, COMPACTSUCCESS,
+ COMPACTBALLOONMIGRATED, COMPACTBALLOONFAILED,
+ COMPACTBALLOONISOLATED, COMPACTBALLOONFREED,
#endif
#ifdef CONFIG_HUGETLB_PAGE
HTLB_BUDDY_PGALLOC, HTLB_BUDDY_PGALLOC_FAIL,
diff --git a/mm/compaction.c b/mm/compaction.c
index 51eac0c..d212dde 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -72,6 +72,7 @@ static bool isolate_balloon_page(struct page *page)
if (is_balloon_page(page) && (page_count(page) == 2)) {
__isolate_balloon_page(page);
unlock_page(page);
+ count_vm_event(COMPACTBALLOONISOLATED);
return true;
}
unlock_page(page);
diff --git a/mm/migrate.c b/mm/migrate.c
index 59c7bc5..5838719 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -78,9 +78,10 @@ void putback_lru_pages(struct list_head *l)
list_del(&page->lru);
dec_zone_page_state(page, NR_ISOLATED_ANON +
page_is_file_cache(page));
- if (unlikely(is_balloon_page(page)))
+ if (unlikely(is_balloon_page(page))) {
+ count_vm_event(COMPACTBALLOONFAILED);
WARN_ON(!putback_balloon_page(page));
- else
+ } else
putback_lru_page(page);
}
}
@@ -878,6 +879,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
page_is_file_cache(page));
put_page(page);
__free_page(page);
+ count_vm_event(COMPACTBALLOONFREED);
return rc;
}
out:
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 1bbbbd9..3b7109f 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -767,6 +767,10 @@ const char * const vmstat_text[] = {
"compact_stall",
"compact_fail",
"compact_success",
+ "compact_balloon_migrated",
+ "compact_balloon_failed",
+ "compact_balloon_isolated",
+ "compact_balloon_freed",
#endif
#ifdef CONFIG_HUGETLB_PAGE
--
1.7.10.4
^ permalink raw reply related
* RE: [PATCH 01/15] Drivers: hv: Format GUIDS as per MSFT standards
From: KY Srinivasan @ 2012-07-17 17:42 UTC (permalink / raw)
To: Greg KH
Cc: apw@canonical.com, devel@linuxdriverproject.org, olaf@aepfle.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20120717160402.GA20481@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, July 17, 2012 12:04 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; olaf@aepfle.de; apw@canonical.com
> Subject: Re: [PATCH 01/15] Drivers: hv: Format GUIDS as per MSFT standards
>
> On Sat, Jul 14, 2012 at 01:34:06PM -0700, K. Y. Srinivasan wrote:
> > Format GUIDS as per MSFT standard. This makes interacting with MSFT
> > tool stack easier.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > drivers/hv/vmbus_drv.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index a220e57..1f7e54a 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -147,7 +147,7 @@ static ssize_t vmbus_show_device_attr(struct device
> *dev,
> >
> > if (!strcmp(dev_attr->attr.name, "class_id")) {
> > ret = sprintf(buf, "{%02x%02x%02x%02x-%02x%02x-%02x%02x-"
> > - "%02x%02x%02x%02x%02x%02x%02x%02x}\n",
> > + "%02x%02x-%02x%02x%02x%02x%02x%02x}\n",
>
> As Joe pointed out, please just use the printk modifier the kernel
> already has for GUIDS, and don't roll your own here. That will work
> properly for you, right?
Will do. Please drop the patch-set; I will post new patches soon.
K. Y
^ permalink raw reply
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Anthony Liguori @ 2012-07-17 18:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, lf-virt, Anthony Liguori,
target-devel, linux-scsi, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <20120717150548.GA11587@redhat.com>
On 07/17/2012 10:05 AM, Michael S. Tsirkin wrote:
> On Wed, Jul 11, 2012 at 09:15:00PM +0000, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger<nab@linux-iscsi.org>
>>
>> Hi folks,
>>
>> The following is a RFC-v2 series of tcm_vhost target fabric driver code
>> currently in-flight for-3.6 mainline code.
>>
>> After last week's developments along with the help of some new folks, the
>> changelog v1 -> v2 so far looks like:
>>
>> *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
>> *) Fix tv_cmd completion -> release SGL memory leak (nab)
>> *) Fix sparse warnings for static variable usage (Fengguang Wu)
>> *) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
>> *) Convert to cmwq submission for I/O dispatch (nab + hch)
>>
>> Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
>> scsi_host->max_target=0 that removes the need for virtio-scsi LLD to hardcode
>> VirtIOSCSIConfig->max_id=1 in order to function with tcm_vhost.
>>
>> Note this series has been pushed into target-pending.git/for-next-merge, and
>> should be getting picked up for tomorrow's linux-next build.
>>
>> Please let us know if you have any concerns and/or additional review feedback.
>>
>> Thank you!
>
>
> It still seems not 100% clear whether this driver will have major
> userspace using it. And if not, it would be very hard to support a driver
> when recent userspace does not use it in the end.
I don't think this is a good reason to exclude something from the kernel.
However, there are good reasons why this doesn't make sense for something like
QEMU--specifically because we have a large number of features in our block layer
that tcm_vhost would bypass.
But perhaps it makes sense for something like native kvm tool. And if it did go
into the kernel, we would certainly support it in QEMU.
But I do think the kernel should carefully consider whether it wants to support
an interface like this. This an extremely complicated ABI with a lot of subtle
details around state and compatibility.
Are you absolutely confident that you can support a userspace application that
expects to get exactly the same response from all possible commands in 20 kernel
versions from now? Virtualization requires absolutely precise compatibility in
terms of bugs and features. This is probably not something the TCM stack has
had to consider yet.
> I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
> Then we don't commit to an ABI.
I think this is a good idea. Even if it goes in, a really clear policy would be
needed wrt the userspace ABI.
While tcm_vhost is probably more useful than vhost_blk, it's a much more complex
ABI to maintain.
Regards,
Anthony Liguori
> For this, you can add a separate Kconfig and source it from drivers/staging/Kconfig.
> Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
>
>
>> Nicholas Bellinger (2):
>> vhost: Add vhost_scsi specific defines
>> tcm_vhost: Initial merge for vhost level target fabric driver
>>
>> Stefan Hajnoczi (2):
>> vhost: Separate vhost-net features from vhost features
>> vhost: make vhost work queue visible
>>
>> drivers/vhost/Kconfig | 6 +
>> drivers/vhost/Makefile | 1 +
>> drivers/vhost/net.c | 4 +-
>> drivers/vhost/tcm_vhost.c | 1609 +++++++++++++++++++++++++++++++++++++++++++++
>> drivers/vhost/tcm_vhost.h | 74 ++
>> drivers/vhost/test.c | 4 +-
>> drivers/vhost/vhost.c | 5 +-
>> drivers/vhost/vhost.h | 6 +-
>> include/linux/vhost.h | 9 +
>> 9 files changed, 1710 insertions(+), 8 deletions(-)
>> create mode 100644 drivers/vhost/tcm_vhost.c
>> create mode 100644 drivers/vhost/tcm_vhost.h
>>
>> --
>> 1.7.2.5
>
^ permalink raw reply
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Michael S. Tsirkin @ 2012-07-17 19:00 UTC (permalink / raw)
To: Anthony Liguori
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, lf-virt, Anthony Liguori,
target-devel, linux-scsi, Paolo Bonzini, Zhi Yong Wu,
Christoph Hellwig
In-Reply-To: <5005B52E.20509@us.ibm.com>
On Tue, Jul 17, 2012 at 01:55:42PM -0500, Anthony Liguori wrote:
> On 07/17/2012 10:05 AM, Michael S. Tsirkin wrote:
> >On Wed, Jul 11, 2012 at 09:15:00PM +0000, Nicholas A. Bellinger wrote:
> >>From: Nicholas Bellinger<nab@linux-iscsi.org>
> >>
> >>Hi folks,
> >>
> >>The following is a RFC-v2 series of tcm_vhost target fabric driver code
> >>currently in-flight for-3.6 mainline code.
> >>
> >>After last week's developments along with the help of some new folks, the
> >>changelog v1 -> v2 so far looks like:
> >>
> >>*) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
> >>*) Fix tv_cmd completion -> release SGL memory leak (nab)
> >>*) Fix sparse warnings for static variable usage (Fengguang Wu)
> >>*) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
> >>*) Convert to cmwq submission for I/O dispatch (nab + hch)
> >>
> >>Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
> >>scsi_host->max_target=0 that removes the need for virtio-scsi LLD to hardcode
> >>VirtIOSCSIConfig->max_id=1 in order to function with tcm_vhost.
> >>
> >>Note this series has been pushed into target-pending.git/for-next-merge, and
> >>should be getting picked up for tomorrow's linux-next build.
> >>
> >>Please let us know if you have any concerns and/or additional review feedback.
> >>
> >>Thank you!
> >
> >
> >It still seems not 100% clear whether this driver will have major
> >userspace using it. And if not, it would be very hard to support a driver
> >when recent userspace does not use it in the end.
>
> I don't think this is a good reason to exclude something from the
> kernel. However, there are good reasons why this doesn't make sense
> for something like QEMU--specifically because we have a large number
> of features in our block layer that tcm_vhost would bypass.
>
> But perhaps it makes sense for something like native kvm tool. And
> if it did go into the kernel, we would certainly support it in QEMU.
>
> But I do think the kernel should carefully consider whether it wants
> to support an interface like this. This an extremely complicated
> ABI with a lot of subtle details around state and compatibility.
>
> Are you absolutely confident that you can support a userspace
> application that expects to get exactly the same response from all
> possible commands in 20 kernel versions from now? Virtualization
> requires absolutely precise compatibility in terms of bugs and
> features. This is probably not something the TCM stack has had to
> consider yet.
>
> >I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
> >Then we don't commit to an ABI.
>
> I think this is a good idea. Even if it goes in, a really clear
> policy would be needed wrt the userspace ABI.
>
> While tcm_vhost is probably more useful than vhost_blk, it's a much
> more complex ABI to maintain.
>
> Regards,
>
> Anthony Liguori
Maybe something like a whitelist of features will help?
Might even be a good idea to make it user controllable.
> >For this, you can add a separate Kconfig and source it from drivers/staging/Kconfig.
> >Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
> >
> >
> >>Nicholas Bellinger (2):
> >> vhost: Add vhost_scsi specific defines
> >> tcm_vhost: Initial merge for vhost level target fabric driver
> >>
> >>Stefan Hajnoczi (2):
> >> vhost: Separate vhost-net features from vhost features
> >> vhost: make vhost work queue visible
> >>
> >> drivers/vhost/Kconfig | 6 +
> >> drivers/vhost/Makefile | 1 +
> >> drivers/vhost/net.c | 4 +-
> >> drivers/vhost/tcm_vhost.c | 1609 +++++++++++++++++++++++++++++++++++++++++++++
> >> drivers/vhost/tcm_vhost.h | 74 ++
> >> drivers/vhost/test.c | 4 +-
> >> drivers/vhost/vhost.c | 5 +-
> >> drivers/vhost/vhost.h | 6 +-
> >> include/linux/vhost.h | 9 +
> >> 9 files changed, 1710 insertions(+), 8 deletions(-)
> >> create mode 100644 drivers/vhost/tcm_vhost.c
> >> create mode 100644 drivers/vhost/tcm_vhost.h
> >>
> >>--
> >>1.7.2.5
> >
^ permalink raw reply
* Re: [PATCH RESEND 5/5] vhost-blk: Add vhost-blk support
From: Jeff Moyer @ 2012-07-17 19:10 UTC (permalink / raw)
To: Asias He; +Cc: virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <1342169711-12386-6-git-send-email-asias@redhat.com>
Asias He <asias@redhat.com> writes:
> vhost-blk is a in kernel virito-blk device accelerator.
>
> This patch is based on Liu Yuan's implementation with various
> improvements and bug fixes. Notably, this patch makes guest notify and
> host completion processing in parallel which gives about 60% performance
> improvement compared to Liu Yuan's implementation.
So, first off, some basic questions. Is it correct to assume that you
tested this with buffered I/O (files opened *without* O_DIRECT)? I'm
pretty sure that if you used O_DIRECT, you'd run into problems (which
are solved by the patch set posted by Shaggy, based on Zach Brown's work
of many moons ago). Note that, with buffered I/O, the submission path
is NOT asynchronous. So, any speedups you've reported are extremely
suspect. ;-)
Next, did you look at Shaggy's patch set? I think it would be best to
focus your efforts on testing *that*, and implementing your work on top
of it.
Having said that, I did do some review of this patch, inlined below.
> +static int vhost_blk_setup(struct vhost_blk *blk)
> +{
> + struct kioctx *ctx;
> +
> + if (blk->ioctx)
> + return 0;
> +
> + blk->ioevent_nr = blk->vq.num;
> + ctx = ioctx_alloc(blk->ioevent_nr);
> + if (IS_ERR(ctx)) {
> + pr_err("Failed to ioctx_alloc");
> + return PTR_ERR(ctx);
> + }
> + put_ioctx(ctx);
> + blk->ioctx = ctx;
> +
> + blk->ioevent = kmalloc(sizeof(struct io_event) * blk->ioevent_nr,
> + GFP_KERNEL);
> + if (!blk->ioevent) {
> + pr_err("Failed to allocate memory for io_events");
> + return -ENOMEM;
You've just leaked blk->ioctx.
> + }
> +
> + blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->ioevent_nr,
> + GFP_KERNEL);
> + if (!blk->reqs) {
> + pr_err("Failed to allocate memory for vhost_blk_req");
> + return -ENOMEM;
And here.
> + }
> +
> + return 0;
> +}
> +
[snip]
> +static int vhost_blk_io_submit(struct vhost_blk *blk, struct file *file,
> + struct vhost_blk_req *req,
> + struct iovec *iov, u64 nr_vecs, loff_t offset,
> + int opcode)
> +{
> + struct kioctx *ioctx = blk->ioctx;
> + mm_segment_t oldfs = get_fs();
> + struct kiocb_batch batch;
> + struct blk_plug plug;
> + struct kiocb *iocb;
> + int ret;
> +
> + if (!try_get_ioctx(ioctx)) {
> + pr_info("Failed to get ioctx");
> + return -EAGAIN;
> + }
Using try_get_ioctx directly gives me a slightly uneasy feeling. I
understand that you don't need to do the lookup, but at least wrap it
and check for ->dead.
> +
> + atomic_long_inc_not_zero(&file->f_count);
> + eventfd_ctx_get(blk->ectx);
> +
> + /* TODO: batch to 1 is not good! */
Agreed. You should setup the batching in vhost_blk_handle_guest_kick.
The way you've written the code, the batching is not at all helpful.
> + kiocb_batch_init(&batch, 1);
> + blk_start_plug(&plug);
> +
> + iocb = aio_get_req(ioctx, &batch);
> + if (unlikely(!iocb)) {
> + ret = -EAGAIN;
> + goto out;
> + }
> +
> + iocb->ki_filp = file;
> + iocb->ki_pos = offset;
> + iocb->ki_buf = (void *)iov;
> + iocb->ki_left = nr_vecs;
> + iocb->ki_nbytes = nr_vecs;
> + iocb->ki_opcode = opcode;
> + iocb->ki_obj.user = req;
> + iocb->ki_eventfd = blk->ectx;
> +
> + set_fs(KERNEL_DS);
> + ret = aio_setup_iocb(iocb, false);
> + set_fs(oldfs);
> + if (unlikely(ret))
> + goto out_put_iocb;
> +
> + spin_lock_irq(&ioctx->ctx_lock);
> + if (unlikely(ioctx->dead)) {
> + spin_unlock_irq(&ioctx->ctx_lock);
> + ret = -EINVAL;
> + goto out_put_iocb;
> + }
> + aio_run_iocb(iocb);
> + spin_unlock_irq(&ioctx->ctx_lock);
> +
> + aio_put_req(iocb);
> +
> + blk_finish_plug(&plug);
> + kiocb_batch_free(ioctx, &batch);
> + put_ioctx(ioctx);
> +
> + return ret;
> +out_put_iocb:
> + aio_put_req(iocb); /* Drop extra ref to req */
> + aio_put_req(iocb); /* Drop I/O ref to req */
> +out:
> + put_ioctx(ioctx);
> + return ret;
> +}
> +
You've duplicated a lot of io_submit_one. I'd rather see that factored
out than to have to maintain two copies.
Again, what I'd *really* like to see is you rebase on top of Shaggy's
work.
Cheers,
Jeff
^ permalink raw reply
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Nicholas A. Bellinger @ 2012-07-17 21:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Zhi Yong Wu,
Anthony Liguori, target-devel, linux-scsi, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <20120717150548.GA11587@redhat.com>
On Tue, 2012-07-17 at 18:05 +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 11, 2012 at 09:15:00PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > Hi folks,
> >
> > The following is a RFC-v2 series of tcm_vhost target fabric driver code
> > currently in-flight for-3.6 mainline code.
> >
> > After last week's developments along with the help of some new folks, the
> > changelog v1 -> v2 so far looks like:
> >
> > *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
> > *) Fix tv_cmd completion -> release SGL memory leak (nab)
> > *) Fix sparse warnings for static variable usage (Fengguang Wu)
> > *) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
> > *) Convert to cmwq submission for I/O dispatch (nab + hch)
> >
> > Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
> > scsi_host->max_target=0 that removes the need for virtio-scsi LLD to hardcode
> > VirtIOSCSIConfig->max_id=1 in order to function with tcm_vhost.
> >
> > Note this series has been pushed into target-pending.git/for-next-merge, and
> > should be getting picked up for tomorrow's linux-next build.
> >
> > Please let us know if you have any concerns and/or additional review feedback.
> >
> > Thank you!
>
>
> It still seems not 100% clear whether this driver will have major
> userspace using it. And if not, it would be very hard to support a driver
> when recent userspace does not use it in the end.
>
I'm happy to commit to working with QEMU + kvm-tool folks to get to a
series that can (eventually) see vhost-scsi support merged into upstream
userspace code.
It took roughly 2 years to get the megasas HBA emulation from Dr. Hannes
merged, but certainly vhost-scsi has alot less moving pieces and
hopefully alot less controversial bits than the buffer -> SGL
conversion.. The key word being here 'hopefully'.. ;)
> I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
> Then we don't commit to an ABI.
> For this, you can add a separate Kconfig and source it from drivers/staging/Kconfig.
> Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
>
So tcm_vhost has been marked as Experimental following virtio-scsi.
Wrt to staging, I'd like to avoid mucking with staging because:
*) The code has been posted for review
*) The code has been converted to use the latest target-core primitives
*) The code does not require cleanups between staging -> merge
*) The code has been stable the last 7 days since RFC-v2 with heavy
Also, tcm_vhost has been marked as Experimental following virtio-scsi.
I'd much rather leave it at Experimental until we merge upstream
userspace support. If userspace support never ends up materializing,
I'm fine with dropping it all together.
However at this point given that there is a 3x performance gap between
virtio-scsi-raw + virtio-scsi+tcm_vhost for random mixed small block
I/O, and we still need the latter to do proper SCSI CDB passthrough for
non TYPE_DISK devices I'm hoping that we can agree on userspace bits
once tcm_vhost is merged.
--nab
^ permalink raw reply
* Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6
From: Michael S. Tsirkin @ 2012-07-17 21:34 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Jens Axboe, Stefan Hajnoczi, kvm-devel, Zhi Yong Wu,
Anthony Liguori, target-devel, linux-scsi, Paolo Bonzini, lf-virt,
Christoph Hellwig
In-Reply-To: <1342559842.18004.440.camel@haakon2.linux-iscsi.org>
On Tue, Jul 17, 2012 at 02:17:22PM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2012-07-17 at 18:05 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 11, 2012 at 09:15:00PM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > >
> > > Hi folks,
> > >
> > > The following is a RFC-v2 series of tcm_vhost target fabric driver code
> > > currently in-flight for-3.6 mainline code.
> > >
> > > After last week's developments along with the help of some new folks, the
> > > changelog v1 -> v2 so far looks like:
> > >
> > > *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
> > > *) Fix tv_cmd completion -> release SGL memory leak (nab)
> > > *) Fix sparse warnings for static variable usage (Fengguang Wu)
> > > *) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
> > > *) Convert to cmwq submission for I/O dispatch (nab + hch)
> > >
> > > Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
> > > scsi_host->max_target=0 that removes the need for virtio-scsi LLD to hardcode
> > > VirtIOSCSIConfig->max_id=1 in order to function with tcm_vhost.
> > >
> > > Note this series has been pushed into target-pending.git/for-next-merge, and
> > > should be getting picked up for tomorrow's linux-next build.
> > >
> > > Please let us know if you have any concerns and/or additional review feedback.
> > >
> > > Thank you!
> >
> >
> > It still seems not 100% clear whether this driver will have major
> > userspace using it. And if not, it would be very hard to support a driver
> > when recent userspace does not use it in the end.
> >
>
> I'm happy to commit to working with QEMU + kvm-tool folks to get to a
> series that can (eventually) see vhost-scsi support merged into upstream
> userspace code.
>
> It took roughly 2 years to get the megasas HBA emulation from Dr. Hannes
> merged, but certainly vhost-scsi has alot less moving pieces and
> hopefully alot less controversial bits than the buffer -> SGL
> conversion.. The key word being here 'hopefully'.. ;)
>
> > I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
> > Then we don't commit to an ABI.
> > For this, you can add a separate Kconfig and source it from drivers/staging/Kconfig.
> > Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
> >
>
> So tcm_vhost has been marked as Experimental following virtio-scsi.
>
> Wrt to staging, I'd like to avoid mucking with staging because:
>
> *) The code has been posted for review
> *) The code has been converted to use the latest target-core primitives
> *) The code does not require cleanups between staging -> merge
> *) The code has been stable the last 7 days since RFC-v2 with heavy
staging is not just for code that needs cleanups.
It's for anything that does not guarantee ABI stability yet.
And I think it's a bit early to guarantee ABI stability - 7 days
is not all that long.
See for example Anthony's comments that raise exactly the ABI
issues.
> Also, tcm_vhost has been marked as Experimental following virtio-scsi.
> I'd much rather leave it at Experimental until we merge upstream
> userspace support. If userspace support never ends up materializing,
> I'm fine with dropping it all together.
Once it's in kernel you never know who will use this driver.
Experimental does not mean driver can be dropped, staging does.
> However at this point given that there is a 3x performance gap between
> virtio-scsi-raw + virtio-scsi+tcm_vhost for random mixed small block
> I/O, and we still need the latter to do proper SCSI CDB passthrough for
> non TYPE_DISK devices I'm hoping that we can agree on userspace bits
> once tcm_vhost is merged.
>
> --nab
I do think upstream kernel would help you nail userspace issues too
but at this point it looks like either staging meterial or 3.6 is too
early.
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox