Re: [PATCH 3/3] Provide control over unmapped pages (v5)
* Andrew Morton [2011-03-30 16:35:45]:
> On Wed, 30 Mar 2011 11:02:38 +0530
> Balbir Singh wrote:
>
> > Changelog v4
> > 1. Added documentation for max_unmapped_pages
> > 2. Better #ifdef'ing of max_unmapped_pages and min_unmapped_pages
> >
> > Changelog v2
> > 1. Use a config option to enable the code (Andrew Morton)
> > 2. Explain the magic tunables in the code or at-least attempt
> >to explain them (General comment)
> > 3. Hint uses of the boot parameter with unlikely (Andrew Morton)
> > 4. Use better names (balanced is not a good naming convention)
> >
> > Provide control using zone_reclaim() and a boot parameter. The
> > code reuses functionality from zone_reclaim() to isolate unmapped
> > pages and reclaim them as a priority, ahead of other mapped pages.
> >
>
> This:
>
> akpm:/usr/src/25> grep '^+#'
> patches/provide-control-over-unmapped-pages-v5.patch
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#else
> +#endif
> +#ifdef CONFIG_NUMA
> +#else
> +#define zone_reclaim_mode 0
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#else /* !CONFIG_UNMAPPED_PAGECACHE_CONTROL */
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
> +#endif
> +#endif
> +#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
>
> is getting out of control. What happens if we just make the feature
> non-configurable?
>
I added the configuration based on review comments I received. If the
feature is made non-configurable, it should be easy to remove them or
just set the default value to "y" in the config.
> > +static int __init unmapped_page_control_parm(char *str)
> > +{
> > + unmapped_page_control = 1;
> > + /*
> > +* XXX: Should we tweak swappiness here?
> > +*/
> > + return 1;
> > +}
> > +__setup("unmapped_page_control", unmapped_page_control_parm);
>
> That looks like a pain - it requires a reboot to change the option,
> which makes testing harder and slower. Methinks you're being a bit
> virtualization-centric here!
:-) The reason for the boot parameter is to ensure that people know
what they are doing.
>
> > +#else /* !CONFIG_UNMAPPED_PAGECACHE_CONTROL */
> > +static inline void reclaim_unmapped_pages(int priority,
> > + struct zone *zone, struct scan_control *sc)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > static struct zone_reclaim_stat *get_reclaim_stat(struct zone *zone,
> > struct scan_control *sc)
> > {
> > @@ -2371,6 +2394,12 @@ loop_again:
> > shrink_active_list(SWAP_CLUSTER_MAX, zone,
> > &sc, priority, 0);
> >
> > + /*
> > +* We do unmapped page reclaim once here and once
> > +* below, so that we don't lose out
> > +*/
> > + reclaim_unmapped_pages(priority, zone, &sc);
>
> Doing this here seems wrong. balance_pgdat() does two passes across
> the zones. The first pass is a read-only work-out-what-to-do pass and
> the second pass is a now-reclaim-some-stuff pass. But here we've stuck
> a do-some-reclaiming operation inside the first, work-out-what-to-do pass.
>
The reason is primarily for balancing, zone_watermark's do not give us
a good idea of whether unmapped pages are balanced, hence the code.
>
> > @@ -2408,6 +2437,11 @@ loop_again:
> > continue;
> >
> > sc.nr_scanned = 0;
> > + /*
> > +* Reclaim unmapped pages upfront, this should be
> > +* really cheap
>
> Comment is mysterious. Why is it cheap?
Cheap because we do a quick check to see if unmapped pages exceed a
threshold. If selective users enable this functionality (which is
expected), the use case is primarily for embedded and virtualization
folks, this should be a simple check.
>
> > +*/
> > + reclaim_unmapped_pages(priority, zone, &sc);
>
>
> I dunno, the whole thing seems rather nasty to me.
>
> It sticks a magical reclaim-unmapped-pages operation right in the
> middle of regular page reclaim. This means that
Re: [PATCH 0/3] Unmapped page cache control (v5)
> > The following series implements page cache control, > this is a split out version of patch 1 of version 3 of the > page cache optimization patches posted earlier at > Previous posting http://lwn.net/Articles/425851/ and analysis > at http://lwn.net/Articles/419713/ > > Detailed Description > > This patch implements unmapped page cache control via preferred > page cache reclaim. The current patch hooks into kswapd and reclaims > page cache if the user has requested for unmapped page control. > This is useful in the following scenario > - In a virtualized environment with cache=writethrough, we see > double caching - (one in the host and one in the guest). As > we try to scale guests, cache usage across the system grows. > The goal of this patch is to reclaim page cache when Linux is running > as a guest and get the host to hold the page cache and manage it. > There might be temporary duplication, but in the long run, memory > in the guests would be used for mapped pages. > > - The option is controlled via a boot option and the administrator > can selectively turn it on, on a need to use basis. > > A lot of the code is borrowed from zone_reclaim_mode logic for > __zone_reclaim(). One might argue that the with ballooning and > KSM this feature is not very useful, but even with ballooning, > we need extra logic to balloon multiple VM machines and it is hard > to figure out the correct amount of memory to balloon. With these > patches applied, each guest has a sufficient amount of free memory > available, that can be easily seen and reclaimed by the balloon driver. > The additional memory in the guest can be reused for additional > applications or used to start additional guests/balance memory in > the host. If anyone think this series works, They are just crazy. This patch reintroduce two old issues. 1) zone reclaim doesn't work if the system has multiple node and the workload is file cache oriented (eg file server, web server, mail server, et al). because zone recliam make some much free pages than zone->pages_min and then new page cache request consume nearest node memory and then it bring next zone reclaim. Then, memory utilization is reduced and unnecessary LRU discard is increased dramatically. SGI folks added CPUSET specific solution in past. (cpuset.memory_spread_page) But global recliam still have its issue. zone recliam is HPC workload specific feature and HPC folks has no motivation to don't use CPUSET. 2) Before 2.6.27, VM has only one LRU and calc_reclaim_mapped() is used to decide to filter out mapped pages. It made a lot of problems for DB servers and large application servers. Because, if the system has a lot of mapped pages, 1) LRU was churned and then reclaim algorithm become lotree one. 2) reclaim latency become terribly slow and hangup detectors misdetect its state and start to force reboot. That was big problem of RHEL5 based banking system. So, sc->may_unmap should be killed in future. Don't increase uses. And, this patch introduce new allocator fast path overhead. I haven't seen any justification for it. In other words, you have to kill following three for getting ack 1) zone reclaim oriented reclaim 2) filter based LRU scanning (eg sc->may_unmap) 3) fastpath overhead. In other words, If you want a feature for vm guest, Any hardcoded machine configration assumption and/or workload assumption are wrong. But, I agree that now we have to concern slightly large VM change parhaps (or parhaps not). Ok, it's good opportunity to fill out some thing. Historically, Linux MM has "free memory are waste memory" policy, and It worked completely fine. But now we have a few exceptions. 1) RT, embedded and finance systems. They really hope to avoid reclaim latency (ie avoid foreground reclaim completely) and they can accept to make slightly much free pages before memory shortage. 2) VM guest VM host and VM guest naturally makes two level page cache model. and Linux page cache + two level don't work fine. It has two issues 1) hard to visualize real memory consumption. That makes harder to works baloon fine. And google want to visualize memory utilization to pack in more jobs. 2) hard to make in kernel memory utilization improvement mechanism. And, now we have four proposal of utilization related issues. 1) cleancache (from Oracle) 2) VirtFS (from IBM) 3) kstaled (from Google) 4) unmapped page reclaim (from you) Probably, we can't merge all of them and we need to consolidate some requirement and implementations. cleancache seems most straight forward two level cache handling for virtalization. but it has soem xen specific mess and, currently, don't fit RT usage. VirtFS has another interesting de-duplication idea. But filesystem based implemenation naturally inherit some vfs interface limitations. Google approach is more unique. memcg don't have double cache issue, t
Re: [PATCH 0/3] Unmapped page cache control (v5)
On Thu, 31 Mar 2011 10:57:03 +0530 Balbir Singh wrote: > * Andrew Morton [2011-03-30 16:36:07]: > > > On Wed, 30 Mar 2011 11:00:26 +0530 > > Balbir Singh wrote: > > > > > Data from the previous patchsets can be found at > > > https://lkml.org/lkml/2010/11/30/79 > > > > It would be nice if the data for the current patchset was present in > > the current patchset's changelog! > > > > Sure, since there were no major changes, I put in a URL. The main > change was the documentation update. Well some poor schmuck has to copy and paste the data into the changelog so it's still there in five years time. It's better to carry this info around in the patch's own metedata, and to maintain and update it. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Unmapped page cache control (v5)
* Andrew Morton [2011-03-30 16:36:07]: > On Wed, 30 Mar 2011 11:00:26 +0530 > Balbir Singh wrote: > > > Data from the previous patchsets can be found at > > https://lkml.org/lkml/2010/11/30/79 > > It would be nice if the data for the current patchset was present in > the current patchset's changelog! > Sure, since there were no major changes, I put in a URL. The main change was the documentation update. -- Three Cheers, Balbir -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Unmapped page cache control (v5)
On Wed, 30 Mar 2011 11:00:26 +0530 Balbir Singh wrote: > Data from the previous patchsets can be found at > https://lkml.org/lkml/2010/11/30/79 It would be nice if the data for the current patchset was present in the current patchset's changelog! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] Provide control over unmapped pages (v5)
On Wed, 30 Mar 2011 11:02:38 +0530
Balbir Singh wrote:
> Changelog v4
> 1. Added documentation for max_unmapped_pages
> 2. Better #ifdef'ing of max_unmapped_pages and min_unmapped_pages
>
> Changelog v2
> 1. Use a config option to enable the code (Andrew Morton)
> 2. Explain the magic tunables in the code or at-least attempt
>to explain them (General comment)
> 3. Hint uses of the boot parameter with unlikely (Andrew Morton)
> 4. Use better names (balanced is not a good naming convention)
>
> Provide control using zone_reclaim() and a boot parameter. The
> code reuses functionality from zone_reclaim() to isolate unmapped
> pages and reclaim them as a priority, ahead of other mapped pages.
>
This:
akpm:/usr/src/25> grep '^+#'
patches/provide-control-over-unmapped-pages-v5.patch
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#else
+#endif
+#ifdef CONFIG_NUMA
+#else
+#define zone_reclaim_mode 0
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#else /* !CONFIG_UNMAPPED_PAGECACHE_CONTROL */
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL) || defined(CONFIG_NUMA)
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
+#endif
+#endif
+#if defined(CONFIG_UNMAPPED_PAGECACHE_CONTROL)
is getting out of control. What happens if we just make the feature
non-configurable?
> +static int __init unmapped_page_control_parm(char *str)
> +{
> + unmapped_page_control = 1;
> + /*
> + * XXX: Should we tweak swappiness here?
> + */
> + return 1;
> +}
> +__setup("unmapped_page_control", unmapped_page_control_parm);
That looks like a pain - it requires a reboot to change the option,
which makes testing harder and slower. Methinks you're being a bit
virtualization-centric here!
> +#else /* !CONFIG_UNMAPPED_PAGECACHE_CONTROL */
> +static inline void reclaim_unmapped_pages(int priority,
> + struct zone *zone, struct scan_control *sc)
> +{
> + return 0;
> +}
> +#endif
> +
> static struct zone_reclaim_stat *get_reclaim_stat(struct zone *zone,
> struct scan_control *sc)
> {
> @@ -2371,6 +2394,12 @@ loop_again:
> shrink_active_list(SWAP_CLUSTER_MAX, zone,
> &sc, priority, 0);
>
> + /*
> + * We do unmapped page reclaim once here and once
> + * below, so that we don't lose out
> + */
> + reclaim_unmapped_pages(priority, zone, &sc);
Doing this here seems wrong. balance_pgdat() does two passes across
the zones. The first pass is a read-only work-out-what-to-do pass and
the second pass is a now-reclaim-some-stuff pass. But here we've stuck
a do-some-reclaiming operation inside the first, work-out-what-to-do pass.
> @@ -2408,6 +2437,11 @@ loop_again:
> continue;
>
> sc.nr_scanned = 0;
> + /*
> + * Reclaim unmapped pages upfront, this should be
> + * really cheap
Comment is mysterious. Why is it cheap?
> + */
> + reclaim_unmapped_pages(priority, zone, &sc);
I dunno, the whole thing seems rather nasty to me.
It sticks a magical reclaim-unmapped-pages operation right in the
middle of regular page reclaim. This means that reclaim will walk the
LRU looking at mapped and unmapped pages. Then it will walk some more,
looking at only unmapped pages and moving the mapped ones to the head
of the LRU. Then it goes back to looking at mapped and unmapped pages.
So it rather screws up the LRU ordering and page aging, does it not?
Also, the special-case handling sticks out like a sore thumb. Would it
not be better to manage the mapped/unmapped bias within the core of the
regular scanning? ie: in shrink_page_list().
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
On 03/30/2011 03:21 PM, Jiri Kosina wrote:
On Wed, 30 Mar 2011, Avi Kivity wrote:
The below patch changes base_addresss to base_address.
Note: I have grepped for base_addresss and nothing shows up,
grepping for base_address gets me lots of output, telling me that
this is a typo, but could be wrong.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..c2fa48b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,7 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
+ unsigned long base_address;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
Why not remove the variable completely?
didnt even think to completely remove the variable(figured it was used
somewhere).I will look at that and resend with removal of the variable for
you..
Well if it was used, you ought to have changed all of the users, no?
I am afraid Justin is not trying to compile-test his patches (I got this
suspicion after last patchset trying to remove all the includes of
version.h).
I do remember to do that, but I will be honest there are ones that I
totally forgot, then remembered after sending out the patch(I admit it I
am guilty of that)
Think having a checklist is the best thing to follow when doing a patch
(telling yourself "yeah Ill remember to do that", never is the best way.
Justin P. Mattock
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
On Wed, 30 Mar 2011, Avi Kivity wrote:
> > > > The below patch changes base_addresss to base_address.
> > > > Note: I have grepped for base_addresss and nothing shows up,
> > > > grepping for base_address gets me lots of output, telling me that
> > > > this is a typo, but could be wrong.
> > > >
> > > > Signed-off-by: Justin P. Mattock
> > > >
> > > > ---
> > > > arch/x86/kvm/i8254.h | 2 +-
> > > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> > > > index 46d08ca..c2fa48b 100644
> > > > --- a/arch/x86/kvm/i8254.h
> > > > +++ b/arch/x86/kvm/i8254.h
> > > > @@ -33,7 +33,7 @@ struct kvm_kpit_state {
> > > > };
> > > >
> > > > struct kvm_pit {
> > > > - unsigned long base_addresss;
> > > > + unsigned long base_address;
> > > > struct kvm_io_device dev;
> > > > struct kvm_io_device speaker_dev;
> > > > struct kvm *kvm;
> > >
> > > Why not remove the variable completely?
> > >
> >
> > didnt even think to completely remove the variable(figured it was used
> > somewhere).I will look at that and resend with removal of the variable for
> > you..
>
> Well if it was used, you ought to have changed all of the users, no?
I am afraid Justin is not trying to compile-test his patches (I got this
suspicion after last patchset trying to remove all the includes of
version.h).
--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 07:16:34PM +0200, Avi Kivity wrote:
> On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
> >Based on Gleb's idea, fix race between nmi injection and enabling
> >nmi window in a simpler way.
> >
> >Signed-off-by: Marcelo Tosatti
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index a6a129f..9a7cc1be 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -5152,6 +5152,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
> > static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > {
> > int r;
> >+int nmi_pending;
> > bool req_int_win = !irqchip_in_kernel(vcpu->kvm)&&
> > vcpu->run->request_interrupt_window;
> >
> >@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > if (unlikely(r))
> > goto out;
> >
> >+nmi_pending = ACCESS_ONCE(vcpu->arch.nmi_pending);
> >+
> > if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> > inject_pending_event(vcpu);
> >
> > /* enable NMI/IRQ window open exits if needed */
> >-if (vcpu->arch.nmi_pending)
> >+if (nmi_pending)
> > kvm_x86_ops->enable_nmi_window(vcpu);
> > else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> > kvm_x86_ops->enable_irq_window(vcpu);
> >
>
> What about the check in inject_pending_events()?
>
Didn't we decide that this check is not a problem? Worst that can happen
is NMI injection will be delayed till next exit.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
On 03/30/2011 10:17 AM, Avi Kivity wrote:
On 03/30/2011 06:30 PM, Justin P. Mattock wrote:
On 03/30/2011 09:26 AM, Avi Kivity wrote:
On 03/30/2011 06:19 PM, Justin P. Mattock wrote:
The below patch changes base_addresss to base_address.
Note: I have grepped for base_addresss and nothing shows up,
grepping for base_address gets me lots of output, telling me that
this is a typo, but could be wrong.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..c2fa48b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,7 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
+ unsigned long base_address;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
Why not remove the variable completely?
didnt even think to completely remove the variable(figured it was used
somewhere).I will look at that and resend with removal of the variable
for you..
Well if it was used, you ought to have changed all of the users, no?
at the moment I see:
(keep in mind my reading skills only go so far!)
grep -Re base_address kvm/* -n
kvm/ioapic.c:276: return ((addr >= ioapic->base_address &&
kvm/ioapic.c:277:(addr < ioapic->base_address +
IOAPIC_MEM_LENGTH)));
kvm/ioapic.c:371: ioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
kvm/ioapic.h:38:u64 base_address;
so changing base_addresss; to base_address; gets kvm_ioapic_reset to
function correctly as well as ioapic_in_range?
(but could be wrong)
Justin P. Mattock
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
On 03/30/2011 06:30 PM, Justin P. Mattock wrote:
On 03/30/2011 09:26 AM, Avi Kivity wrote:
On 03/30/2011 06:19 PM, Justin P. Mattock wrote:
The below patch changes base_addresss to base_address.
Note: I have grepped for base_addresss and nothing shows up,
grepping for base_address gets me lots of output, telling me that
this is a typo, but could be wrong.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..c2fa48b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,7 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
+ unsigned long base_address;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
Why not remove the variable completely?
didnt even think to completely remove the variable(figured it was used
somewhere).I will look at that and resend with removal of the variable
for you..
Well if it was used, you ought to have changed all of the users, no?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAT networking from guest not working
OK, I''ll go ask them. Thanks! Marc On Wed, Mar 30, 2011 at 12:44 PM, David Mair wrote: > Hi Marc, > > On 03/30/2011 08:46 AM, Marc Boorshtein wrote: >> >> I apologize if this is the wrong list. I have just installed Fedora >> 14 and gotten KVM up and running. I installed a Windows 7 guest >> without issue using NAT for networking. The guest can ping the >> default gateway, but can't reach the internet or the rest of the >> network. Here's the really odd thing, DNS resolution works. I've had >> no issues with VMWare images so I don't think its an issue with my >> networking infrastructure. Here's my uname: >> >> Linux r2d2.tremolo.lan 2.6.38-1.fc15.x86_64 #1 SMP Tue Mar 15 05:29:00 >> UTC 2011 x86_64 x86_64 x86_64 GNU/Linux >> >> libvirtd and libvirtd-guests are both running. Any help (including >> where I should take this query if this isn't the right place) would be >> greatly appreciated. > > You'd need to provide more information but to be fair kvm provides no part > of any nat implementation I'm aware of that can be used by kvm hosted vms. > That's usually done by qemu or iptables on the host. Which case is defined > by qemu command line so at the very least, your problem probably also exists > with a qemu command line that includes -no-kvm and in that case you'd be > better asking on a qemu list. > > -- > David Mair. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On 03/30/2011 06:30 PM, Marcelo Tosatti wrote:
Based on Gleb's idea, fix race between nmi injection and enabling
nmi window in a simpler way.
Signed-off-by: Marcelo Tosatti
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a6a129f..9a7cc1be 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5152,6 +5152,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
{
int r;
+ int nmi_pending;
bool req_int_win = !irqchip_in_kernel(vcpu->kvm)&&
vcpu->run->request_interrupt_window;
@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
if (unlikely(r))
goto out;
+ nmi_pending = ACCESS_ONCE(vcpu->arch.nmi_pending);
+
if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
inject_pending_event(vcpu);
/* enable NMI/IRQ window open exits if needed */
- if (vcpu->arch.nmi_pending)
+ if (nmi_pending)
kvm_x86_ops->enable_nmi_window(vcpu);
else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
kvm_x86_ops->enable_irq_window(vcpu);
What about the check in inject_pending_events()?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 06:33:22PM +0200, Gleb Natapov wrote: > On Wed, Mar 30, 2011 at 01:30:28PM -0300, Marcelo Tosatti wrote: > > > > Based on Gleb's idea, fix race between nmi injection and enabling > > nmi window in a simpler way. > > > > Signed-off-by: Marcelo Tosatti > > > But we need to revert the patch that introduced use of request for NMI > first. Otherwise NMI will not be delivered, no? Yes, pushed, can you verify please? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2]arch:x86:kvm:i8254.h Remove base_addresss in kvm_pit since it is unused.
The patch below removes unsigned long base_addresss; in i8254.h
since it is unused.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..b681a9f 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,6 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: NAT networking from guest not working
Hi Marc, On 03/30/2011 08:46 AM, Marc Boorshtein wrote: I apologize if this is the wrong list. I have just installed Fedora 14 and gotten KVM up and running. I installed a Windows 7 guest without issue using NAT for networking. The guest can ping the default gateway, but can't reach the internet or the rest of the network. Here's the really odd thing, DNS resolution works. I've had no issues with VMWare images so I don't think its an issue with my networking infrastructure. Here's my uname: Linux r2d2.tremolo.lan 2.6.38-1.fc15.x86_64 #1 SMP Tue Mar 15 05:29:00 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux libvirtd and libvirtd-guests are both running. Any help (including where I should take this query if this isn't the right place) would be greatly appreciated. You'd need to provide more information but to be fair kvm provides no part of any nat implementation I'm aware of that can be used by kvm hosted vms. That's usually done by qemu or iptables on the host. Which case is defined by qemu command line so at the very least, your problem probably also exists with a qemu command line that includes -no-kvm and in that case you'd be better asking on a qemu list. -- David Mair. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: KVM: x86: better fix for race between nmi injection and enabling nmi window
On Wed, Mar 30, 2011 at 01:30:28PM -0300, Marcelo Tosatti wrote:
>
> Based on Gleb's idea, fix race between nmi injection and enabling
> nmi window in a simpler way.
>
> Signed-off-by: Marcelo Tosatti
>
But we need to revert the patch that introduced use of request for NMI
first. Otherwise NMI will not be delivered, no?
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a6a129f..9a7cc1be 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5152,6 +5152,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
> static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> {
> int r;
> + int nmi_pending;
> bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
> vcpu->run->request_interrupt_window;
>
> @@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> if (unlikely(r))
> goto out;
>
> + nmi_pending = ACCESS_ONCE(vcpu->arch.nmi_pending);
> +
> if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
> inject_pending_event(vcpu);
>
> /* enable NMI/IRQ window open exits if needed */
> - if (vcpu->arch.nmi_pending)
> + if (nmi_pending)
> kvm_x86_ops->enable_nmi_window(vcpu);
> else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
> kvm_x86_ops->enable_irq_window(vcpu);
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
On 03/30/2011 09:26 AM, Avi Kivity wrote:
On 03/30/2011 06:19 PM, Justin P. Mattock wrote:
The below patch changes base_addresss to base_address.
Note: I have grepped for base_addresss and nothing shows up,
grepping for base_address gets me lots of output, telling me that
this is a typo, but could be wrong.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..c2fa48b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,7 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
+ unsigned long base_address;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
Why not remove the variable completely?
didnt even think to completely remove the variable(figured it was used
somewhere).I will look at that and resend with removal of the variable
for you..
Justin P. Mattock
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
KVM: x86: better fix for race between nmi injection and enabling nmi window
Based on Gleb's idea, fix race between nmi injection and enabling
nmi window in a simpler way.
Signed-off-by: Marcelo Tosatti
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a6a129f..9a7cc1be 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5152,6 +5152,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
{
int r;
+ int nmi_pending;
bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&
vcpu->run->request_interrupt_window;
@@ -5195,11 +5196,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
if (unlikely(r))
goto out;
+ nmi_pending = ACCESS_ONCE(vcpu->arch.nmi_pending);
+
if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
inject_pending_event(vcpu);
/* enable NMI/IRQ window open exits if needed */
- if (vcpu->arch.nmi_pending)
+ if (nmi_pending)
kvm_x86_ops->enable_nmi_window(vcpu);
else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
kvm_x86_ops->enable_irq_window(vcpu);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
On 03/30/2011 06:19 PM, Justin P. Mattock wrote:
The below patch changes base_addresss to base_address.
Note: I have grepped for base_addresss and nothing shows up,
grepping for base_address gets me lots of output, telling me that
this is a typo, but could be wrong.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..c2fa48b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,7 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
+ unsigned long base_address;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
Why not remove the variable completely?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]arch:x86:kvm:i8254.h Fix typo in kvm_pit
The below patch changes base_addresss to base_address.
Note: I have grepped for base_addresss and nothing shows up,
grepping for base_address gets me lots of output, telling me that
this is a typo, but could be wrong.
Signed-off-by: Justin P. Mattock
---
arch/x86/kvm/i8254.h |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 46d08ca..c2fa48b 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -33,7 +33,7 @@ struct kvm_kpit_state {
};
struct kvm_pit {
- unsigned long base_addresss;
+ unsigned long base_address;
struct kvm_io_device dev;
struct kvm_io_device speaker_dev;
struct kvm *kvm;
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
NAT networking from guest not working
I apologize if this is the wrong list. I have just installed Fedora 14 and gotten KVM up and running. I installed a Windows 7 guest without issue using NAT for networking. The guest can ping the default gateway, but can't reach the internet or the rest of the network. Here's the really odd thing, DNS resolution works. I've had no issues with VMWare images so I don't think its an issue with my networking infrastructure. Here's my uname: Linux r2d2.tremolo.lan 2.6.38-1.fc15.x86_64 #1 SMP Tue Mar 15 05:29:00 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux libvirtd and libvirtd-guests are both running. Any help (including where I should take this query if this isn't the right place) would be greatly appreciated. Thanks! Marc -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 03:43 PM, Gleb Natapov wrote: On Wed, Mar 30, 2011 at 03:41:09PM +0200, Avi Kivity wrote: > On 03/30/2011 03:36 PM, Gleb Natapov wrote: > >> > >> It's wierd. Do you get perf hits in the copying? > >> > >How can I check. The memcpy is inlined. > > > > perf annotate x86_emulate_instruction > > (newer perf allows you to get there interactively from 'perf report') > > >> Copying a couple of hot cache lines shouldn't take any measurable Ah, forgot about it: First one: 27.71 : 1179f: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) Second one: 32.68 : 11888: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) Okay, I'm convinced. (looks like a missed optimization, should use rep movsq there) -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 03:41:09PM +0200, Avi Kivity wrote: > On 03/30/2011 03:36 PM, Gleb Natapov wrote: > >> > >> It's wierd. Do you get perf hits in the copying? > >> > >How can I check. The memcpy is inlined. > > > > perf annotate x86_emulate_instruction > > (newer perf allows you to get there interactively from 'perf report') > > >> Copying a couple of hot cache lines shouldn't take any measurable Ah, forgot about it: First one: 27.71 : 1179f: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) Second one: 32.68 : 11888: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) > >> time compared to a heavyweight exit. > >> > >The whole function takes only 1.5% CPU. Perf measures how much this > >function become faster and heavyweight exit is not part of the function. > > It's still relative to exit cost. If the total exit was 2 us, then > a 1% decrease in cost translates to 40 ns. > > (well, that's not unlikely for a 256 byte memcpy, but let's be sure). > > -- > error compiling committee.c: too many arguments to function -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 03:36 PM, Gleb Natapov wrote: > > It's wierd. Do you get perf hits in the copying? > How can I check. The memcpy is inlined. perf annotate x86_emulate_instruction (newer perf allows you to get there interactively from 'perf report') > Copying a couple of hot cache lines shouldn't take any measurable > time compared to a heavyweight exit. > The whole function takes only 1.5% CPU. Perf measures how much this function become faster and heavyweight exit is not part of the function. It's still relative to exit cost. If the total exit was 2 us, then a 1% decrease in cost translates to 40 ns. (well, that's not unlikely for a 256 byte memcpy, but let's be sure). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 03:29:02PM +0200, Avi Kivity wrote: > On 03/30/2011 03:26 PM, Gleb Natapov wrote: > >On Wed, Mar 30, 2011 at 02:48:28PM +0200, Gleb Natapov wrote: > >> On Wed, Mar 30, 2011 at 02:17:55PM +0200, Avi Kivity wrote: > >> > On 03/30/2011 01:43 PM, Gleb Natapov wrote: > >> > >After reboot perf started to work. I ran modified emulator.flat unit > >> > >test. It was modified to run test_cmps() in an endless loop. > >> > > > >> > >Without patch: > >> > >1.71% qemu-system-x86 [kvm] [k] > >> x86_emulate_instruction > >> > >1.51% qemu-system-x86 [kvm] [k] > >> x86_emulate_instruction > >> > >1.68% qemu-system-x86 [kvm] [k] > >> x86_emulate_instruction > >> > > > >> > >With patch: > >> > >0.84% qemu-system-x86 [kvm] [k] > >> x86_emulate_instruction > >> > >0.96% qemu-system-x86 [kvm] [k] > >> x86_emulate_instruction > >> > >0.89% qemu-system-x86 [kvm] [k] > >> x86_emulate_instruction > >> > > > >> > > >> > The cause might be kvm_rip_write() using vmwrite. Can you use perf > >> > to see where the hits are in x86_emulate_instruction? > >> > > >> > If that's the case, we may be able to do local optimizations to > >> > kvm_rip_write(), kvm_set_rflags(), and toggle_interruptiblity() > >> > instead of this global change. > >> > > >> I can leave copying there and eliminate only kvm_rip_write and see > >> perf data. > >> > > > >1.75% qemu-system-x86 [kvm] [k] x86_emulate_instruction > >1.60% qemu-system-x86 [kvm] [k] x86_emulate_instruction > >1.42% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > >This is with copy in place, but those are under if (writeback): > > toggle_interruptibility(vcpu, > > vcpu->arch.emulate_ctxt.interruptibility); > > kvm_set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags); > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > vcpu->arch.emulate_regs_need_sync_to_vcpu = false; > > kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip); > > > > It's wierd. Do you get perf hits in the copying? > How can I check. The memcpy is inlined. > Copying a couple of hot cache lines shouldn't take any measurable > time compared to a heavyweight exit. > The whole function takes only 1.5% CPU. Perf measures how much this function become faster and heavyweight exit is not part of the function. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries
On 03/30/2011 03:26 PM, Avi Kivity wrote: This behaviour is mandated by the spec (looking at the Intel one), though it is implemented incorrectly - should always return largest basic leaf, and ignore the kvm leaves. I think the correct behaviour is: if (e->function < 1 && (!best || e->function > best->function)) best = e; Oh, and it should honor ecx.. what a great interface. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 03:26 PM, Gleb Natapov wrote: On Wed, Mar 30, 2011 at 02:48:28PM +0200, Gleb Natapov wrote: > On Wed, Mar 30, 2011 at 02:17:55PM +0200, Avi Kivity wrote: > > On 03/30/2011 01:43 PM, Gleb Natapov wrote: > > >After reboot perf started to work. I ran modified emulator.flat unit > > >test. It was modified to run test_cmps() in an endless loop. > > > > > >Without patch: > > >1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > > > >With patch: > > >0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > > > > > The cause might be kvm_rip_write() using vmwrite. Can you use perf > > to see where the hits are in x86_emulate_instruction? > > > > If that's the case, we may be able to do local optimizations to > > kvm_rip_write(), kvm_set_rflags(), and toggle_interruptiblity() > > instead of this global change. > > > I can leave copying there and eliminate only kvm_rip_write and see > perf data. > 1.75% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.60% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.42% qemu-system-x86 [kvm] [k] x86_emulate_instruction This is with copy in place, but those are under if (writeback): toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility); kvm_set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags); kvm_make_request(KVM_REQ_EVENT, vcpu); vcpu->arch.emulate_regs_need_sync_to_vcpu = false; kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip); It's wierd. Do you get perf hits in the copying? Copying a couple of hot cache lines shouldn't take any measurable time compared to a heavyweight exit. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 02:48:28PM +0200, Gleb Natapov wrote: > On Wed, Mar 30, 2011 at 02:17:55PM +0200, Avi Kivity wrote: > > On 03/30/2011 01:43 PM, Gleb Natapov wrote: > > >After reboot perf started to work. I ran modified emulator.flat unit > > >test. It was modified to run test_cmps() in an endless loop. > > > > > >Without patch: > > >1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > > > >With patch: > > >0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > >0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > > > > > The cause might be kvm_rip_write() using vmwrite. Can you use perf > > to see where the hits are in x86_emulate_instruction? > > > > If that's the case, we may be able to do local optimizations to > > kvm_rip_write(), kvm_set_rflags(), and toggle_interruptiblity() > > instead of this global change. > > > I can leave copying there and eliminate only kvm_rip_write and see > perf data. > 1.75% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.60% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.42% qemu-system-x86 [kvm] [k] x86_emulate_instruction This is with copy in place, but those are under if (writeback): toggle_interruptibility(vcpu, vcpu->arch.emulate_ctxt.interruptibility); kvm_set_rflags(vcpu, vcpu->arch.emulate_ctxt.eflags); kvm_make_request(KVM_REQ_EVENT, vcpu); vcpu->arch.emulate_regs_need_sync_to_vcpu = false; kvm_rip_write(vcpu, vcpu->arch.emulate_ctxt.eip); -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries
On 03/30/2011 03:01 PM, Andre Przywara wrote: If KVM cannot find an exact match for a requested CPUID leaf, the code will try to find the closest match instead of simply confessing it's failure. The heuristic is on one hand wrong nowadays, since it does not take the KVM CPUID leaves (0x40xx) into account. On the other hand the callers of this function can all deal with the no-match situation. So lets remove this code, as it serves no purpose. This fixes a crash of newer Linux kernels as KVM guests on AMD Bulldozer CPUs, where bogus values were returned in response to a CPUID intercept. @@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu, best = e; break; } - /* -* Both basic or both extended? -*/ - if (((e->function ^ function)& 0x8000) == 0) - if (!best || e->function> best->function) - best = e; } return best; } This behaviour is mandated by the spec (looking at the Intel one), though it is implemented incorrectly - should always return largest basic leaf, and ignore the kvm leaves. I think the correct behaviour is: if (e->function < 1 && (!best || e->function > best->function)) best = e; We probably need a find_exact_cpuid_entry() that returns NULL if it doesn't find a match, for internal use. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries
If KVM cannot find an exact match for a requested CPUID leaf, the code will try to find the closest match instead of simply confessing it's failure. The heuristic is on one hand wrong nowadays, since it does not take the KVM CPUID leaves (0x40xx) into account. On the other hand the callers of this function can all deal with the no-match situation. So lets remove this code, as it serves no purpose. This fixes a crash of newer Linux kernels as KVM guests on AMD Bulldozer CPUs, where bogus values were returned in response to a CPUID intercept. CC: [2.6.38] Signed-off-by: Andre Przywara --- arch/x86/kvm/x86.c |6 -- 1 files changed, 0 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 6e86cec..625143f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu, best = e; break; } - /* -* Both basic or both extended? -*/ - if (((e->function ^ function) & 0x8000) == 0) - if (!best || e->function > best->function) - best = e; } return best; } -- 1.6.4 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] kvm/x86: fix XSAVE bit scanning
When KVM scans the 0xD CPUID leaf for propagating the XSAVE save area
leaves, it assumes that the leaves are contigious and stops at the
first zero one. On AMD hardware there is a gap, though, as LWP uses
leaf 62 to announce it's state save area.
So lets iterate through all 64 possible leaves and simply skip zero
ones to also cover later features.
CC: [2.6.38]
Signed-off-by: Andre Przywara
---
arch/x86/kvm/x86.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bfd7763..6e86cec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2395,9 +2395,9 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry,
u32 function,
int i;
entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
- for (i = 1; *nent < maxnent; ++i) {
- if (entry[i - 1].eax == 0 && i != 2)
- break;
+ for (i = 1; *nent < maxnent && i < 64; ++i) {
+ if (entry[i].eax == 0)
+ continue;
do_cpuid_1_ent(&entry[i], function, i);
entry[i].flags |=
KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
--
1.6.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 02:17:55PM +0200, Avi Kivity wrote: > On 03/30/2011 01:43 PM, Gleb Natapov wrote: > >After reboot perf started to work. I ran modified emulator.flat unit > >test. It was modified to run test_cmps() in an endless loop. > > > >Without patch: > >1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction > >1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction > >1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > >With patch: > >0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction > >0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction > >0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction > > > > The cause might be kvm_rip_write() using vmwrite. Can you use perf > to see where the hits are in x86_emulate_instruction? > > If that's the case, we may be able to do local optimizations to > kvm_rip_write(), kvm_set_rflags(), and toggle_interruptiblity() > instead of this global change. > I can leave copying there and eliminate only kvm_rip_write and see perf data. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 01:43 PM, Gleb Natapov wrote: After reboot perf started to work. I ran modified emulator.flat unit test. It was modified to run test_cmps() in an endless loop. Without patch: 1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction With patch: 0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction The cause might be kvm_rip_write() using vmwrite. Can you use perf to see where the hits are in x86_emulate_instruction? If that's the case, we may be able to do local optimizations to kvm_rip_write(), kvm_set_rflags(), and toggle_interruptiblity() instead of this global change. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 02:12 PM, Avi Kivity wrote: On 03/30/2011 01:43 PM, Gleb Natapov wrote: > > The patch saves copying of 256 bytes on each MMIO/PIO read. It may not > save a lot comparing to time it takes to do one MMIO to userspace, but > do 1 million of those and you saved a lot of CPU cycles. I do not think > we should abandon the optimization so easily. Unfortunately I can't run > perf on 2.6.38 kernel right now. It gives me strange errors and when it > doesn't it makes kernel OOPS. > After reboot perf started to work. I ran modified emulator.flat unit test. It was modified to run test_cmps() in an endless loop. Without patch: 1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction With patch: 0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction Well, that is probably significant. Please rebase the patch after sse-mmio goes in. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 01:43 PM, Gleb Natapov wrote: > > The patch saves copying of 256 bytes on each MMIO/PIO read. It may not > save a lot comparing to time it takes to do one MMIO to userspace, but > do 1 million of those and you saved a lot of CPU cycles. I do not think > we should abandon the optimization so easily. Unfortunately I can't run > perf on 2.6.38 kernel right now. It gives me strange errors and when it > doesn't it makes kernel OOPS. > After reboot perf started to work. I ran modified emulator.flat unit test. It was modified to run test_cmps() in an endless loop. Without patch: 1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction With patch: 0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction Well, that is probably significant. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 01:22:43PM +0200, Gleb Natapov wrote: > On Wed, Mar 30, 2011 at 12:50:53PM +0200, Avi Kivity wrote: > > On 03/30/2011 12:47 PM, Gleb Natapov wrote: > > >On Wed, Mar 30, 2011 at 12:38:53PM +0200, Avi Kivity wrote: > > >> On 03/29/2011 02:08 PM, Gleb Natapov wrote: > > >> >Currently we sync registers back and forth before/after exiting > > >> >to userspace for IO, but during IO device model shouldn't need to > > >> >read/write the registers, so we can as well skip those sync points. The > > >> >only exaception is broken vmware backdor interface. The new code sync > > >> >registers content during IO only if registers are read from/written to > > >> >by userspace in the middle of the IO operation and this almost never > > >> >happens in practise. > > >> > > >> While this is a nice idea, how much does it save in practice? It > > >> does introduce more complexity. > > >> > > > > > >I haven't measured, but can try to do so. It eliminates two copies of > > >all registers on each MMIO/PIO read. I expect this to be measurable in > > >workloads that do many such reads. > > > > > > > I don't, especially if these are mmios to userspace. Perhaps it's > > better to remove the copy on kernel mmio, since it's much faster, if > > the result is simpler (there can be no KVM_SET_REGS in that > > context). > > > > The patch saves copying of 256 bytes on each MMIO/PIO read. It may not > save a lot comparing to time it takes to do one MMIO to userspace, but > do 1 million of those and you saved a lot of CPU cycles. I do not think > we should abandon the optimization so easily. Unfortunately I can't run > perf on 2.6.38 kernel right now. It gives me strange errors and when it > doesn't it makes kernel OOPS. > After reboot perf started to work. I ran modified emulator.flat unit test. It was modified to run test_cmps() in an endless loop. Without patch: 1.71% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.51% qemu-system-x86 [kvm] [k] x86_emulate_instruction 1.68% qemu-system-x86 [kvm] [k] x86_emulate_instruction With patch: 0.84% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.96% qemu-system-x86 [kvm] [k] x86_emulate_instruction 0.89% qemu-system-x86 [kvm] [k] x86_emulate_instruction -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 12:50:53PM +0200, Avi Kivity wrote: > On 03/30/2011 12:47 PM, Gleb Natapov wrote: > >On Wed, Mar 30, 2011 at 12:38:53PM +0200, Avi Kivity wrote: > >> On 03/29/2011 02:08 PM, Gleb Natapov wrote: > >> >Currently we sync registers back and forth before/after exiting > >> >to userspace for IO, but during IO device model shouldn't need to > >> >read/write the registers, so we can as well skip those sync points. The > >> >only exaception is broken vmware backdor interface. The new code sync > >> >registers content during IO only if registers are read from/written to > >> >by userspace in the middle of the IO operation and this almost never > >> >happens in practise. > >> > >> While this is a nice idea, how much does it save in practice? It > >> does introduce more complexity. > >> > > > >I haven't measured, but can try to do so. It eliminates two copies of > >all registers on each MMIO/PIO read. I expect this to be measurable in > >workloads that do many such reads. > > > > I don't, especially if these are mmios to userspace. Perhaps it's > better to remove the copy on kernel mmio, since it's much faster, if > the result is simpler (there can be no KVM_SET_REGS in that > context). > The patch saves copying of 256 bytes on each MMIO/PIO read. It may not save a lot comparing to time it takes to do one MMIO to userspace, but do 1 million of those and you saved a lot of CPU cycles. I do not think we should abandon the optimization so easily. Unfortunately I can't run perf on 2.6.38 kernel right now. It gives me strange errors and when it doesn't it makes kernel OOPS. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/30/2011 12:47 PM, Gleb Natapov wrote: On Wed, Mar 30, 2011 at 12:38:53PM +0200, Avi Kivity wrote: > On 03/29/2011 02:08 PM, Gleb Natapov wrote: > >Currently we sync registers back and forth before/after exiting > >to userspace for IO, but during IO device model shouldn't need to > >read/write the registers, so we can as well skip those sync points. The > >only exaception is broken vmware backdor interface. The new code sync > >registers content during IO only if registers are read from/written to > >by userspace in the middle of the IO operation and this almost never > >happens in practise. > > While this is a nice idea, how much does it save in practice? It > does introduce more complexity. > I haven't measured, but can try to do so. It eliminates two copies of all registers on each MMIO/PIO read. I expect this to be measurable in workloads that do many such reads. I don't, especially if these are mmios to userspace. Perhaps it's better to remove the copy on kernel mmio, since it's much faster, if the result is simpler (there can be no KVM_SET_REGS in that context). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On Wed, Mar 30, 2011 at 12:38:53PM +0200, Avi Kivity wrote: > On 03/29/2011 02:08 PM, Gleb Natapov wrote: > >Currently we sync registers back and forth before/after exiting > >to userspace for IO, but during IO device model shouldn't need to > >read/write the registers, so we can as well skip those sync points. The > >only exaception is broken vmware backdor interface. The new code sync > >registers content during IO only if registers are read from/written to > >by userspace in the middle of the IO operation and this almost never > >happens in practise. > > While this is a nice idea, how much does it save in practice? It > does introduce more complexity. > I haven't measured, but can try to do so. It eliminates two copies of all registers on each MMIO/PIO read. I expect this to be measurable in workloads that do many such reads. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] numa: Don't limit node count by smp count
On 03/29/2011 02:56 AM, Sasha Levin wrote: It is possible to create CPU-less NUMA nodes, node amount shouldn't be limited by amount of CPUs. Patch seems fine; but please send to qemu-de...@nongnu.org. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: emulator: do not needlesly sync registers from emulator ctxt to vcpu
On 03/29/2011 02:08 PM, Gleb Natapov wrote: Currently we sync registers back and forth before/after exiting to userspace for IO, but during IO device model shouldn't need to read/write the registers, so we can as well skip those sync points. The only exaception is broken vmware backdor interface. The new code sync registers content during IO only if registers are read from/written to by userspace in the middle of the IO operation and this almost never happens in practise. While this is a nice idea, how much does it save in practice? It does introduce more complexity. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: virtio-blk.c handling of i/o which is not a 512 multiple
On Wed, Mar 30, 2011 at 9:15 AM, Conor Murphy wrote: > I'm trying to write a virtio-blk driver for Solaris. I've gotten it to the > point > where Solaris can see the device and create a ZFS file system on it. > > However when I try and create a UFS filesystem on the device, the VM crashed > with the error > *** glibc detected *** /usr/bin/qemu-kvm: double free or corruption (!prev): > 0x7f2d38000a00 *** This is a bug in QEMU. A guest must not be able to trigger a crash. > I can reproduce the problem with a simple dd, i.e. > dd if=/dev/zero of=/dev/rdsk/c2d10p0 bs=5000 count=1 I think this a raw character device, which is why you're even able to perform non-blocksize accesses? Have you looked at how other drivers (like the Xen pv blkfront) handle this? > My driver will create a virtio-blk request with two elements in the sg list, > one > for the first 4096 byes and the other for the remaining 904. > > From stepping through with gdb, virtio_blk_handle_write will sets n_sectors > to 9 > (5000 / 512). Later on the code, n_sectors is used the calculate the size of > the > buffer required but 9 * 512 is too small and so when the request is process it > ends up writing past the end of the buffer and I guest this triggers the glibc > error. We need to validate that (qiov->size % BDRV_SECTOR_SIZE) == 0 and reject invalid requests. > Is there a requirement for virtio-blk guest drivers that all i/o requests are > sized in multiples of 512 bytes? There is no strict requirement according to the virtio specification, but maybe there should be: http://ozlabs.org/~rusty/virtio-spec/virtio-spec-0.8.9.pdf Stefan -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: virtio-blk.c handling of i/o which is not a 512 multiple
On 03/30/2011 10:41 AM, Alexander Graf wrote: On 30.03.2011, at 10:15, Conor Murphy wrote: > Hi, > > I'm trying to write a virtio-blk driver for Solaris. I've gotten it to the point > where Solaris can see the device and create a ZFS file system on it. > > However when I try and create a UFS filesystem on the device, the VM crashed > with the error > *** glibc detected *** /usr/bin/qemu-kvm: double free or corruption (!prev): > 0x7f2d38000a00 *** Ouch. > > I can reproduce the problem with a simple dd, i.e. > dd if=/dev/zero of=/dev/rdsk/c2d10p0 bs=5000 count=1 > > My driver will create a virtio-blk request with two elements in the sg list, one > for the first 4096 byes and the other for the remaining 904. > > From stepping through with gdb, virtio_blk_handle_write will sets n_sectors to 9 > (5000 / 512). Later on the code, n_sectors is used the calculate the size of the > buffer required but 9 * 512 is too small and so when the request is process it > ends up writing past the end of the buffer and I guest this triggers the glibc > error. > > Is there a requirement for virtio-blk guest drivers that all i/o requests are > sized in multiples of 512 bytes? There should be some documentation on virtio-blk, but I can't seem to find it atm. Either way, you're posting this on the wrong mailing list. The correct ones are: Qemu: QEMU-devel Developers Virtio: virtualizat...@lists.linux-foundation.org Please repost it there and hope for someone to reply. I'd personally find it very odd to see that any HBA would accept data that's not aligned to sectors, but let's get some comments from the inventors of the protocol :) The data should be aligned; but of course a misalignment shouldn't kill qemu, only fail the request. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: virtio-blk.c handling of i/o which is not a 512 multiple
On 30.03.2011, at 10:15, Conor Murphy wrote: > Hi, > > I'm trying to write a virtio-blk driver for Solaris. I've gotten it to the > point > where Solaris can see the device and create a ZFS file system on it. > > However when I try and create a UFS filesystem on the device, the VM crashed > with the error > *** glibc detected *** /usr/bin/qemu-kvm: double free or corruption (!prev): > 0x7f2d38000a00 *** Ouch. > > I can reproduce the problem with a simple dd, i.e. > dd if=/dev/zero of=/dev/rdsk/c2d10p0 bs=5000 count=1 > > My driver will create a virtio-blk request with two elements in the sg list, > one > for the first 4096 byes and the other for the remaining 904. > > From stepping through with gdb, virtio_blk_handle_write will sets n_sectors > to 9 > (5000 / 512). Later on the code, n_sectors is used the calculate the size of > the > buffer required but 9 * 512 is too small and so when the request is process it > ends up writing past the end of the buffer and I guest this triggers the glibc > error. > > Is there a requirement for virtio-blk guest drivers that all i/o requests are > sized in multiples of 512 bytes? There should be some documentation on virtio-blk, but I can't seem to find it atm. Either way, you're posting this on the wrong mailing list. The correct ones are: Qemu: QEMU-devel Developers Virtio: virtualizat...@lists.linux-foundation.org Please repost it there and hope for someone to reply. I'd personally find it very odd to see that any HBA would accept data that's not aligned to sectors, but let's get some comments from the inventors of the protocol :) Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
virtio-blk.c handling of i/o which is not a 512 multiple
Hi, I'm trying to write a virtio-blk driver for Solaris. I've gotten it to the point where Solaris can see the device and create a ZFS file system on it. However when I try and create a UFS filesystem on the device, the VM crashed with the error *** glibc detected *** /usr/bin/qemu-kvm: double free or corruption (!prev): 0x7f2d38000a00 *** I can reproduce the problem with a simple dd, i.e. dd if=/dev/zero of=/dev/rdsk/c2d10p0 bs=5000 count=1 My driver will create a virtio-blk request with two elements in the sg list, one for the first 4096 byes and the other for the remaining 904. >From stepping through with gdb, virtio_blk_handle_write will sets n_sectors to >9 (5000 / 512). Later on the code, n_sectors is used the calculate the size of the buffer required but 9 * 512 is too small and so when the request is process it ends up writing past the end of the buffer and I guest this triggers the glibc error. Is there a requirement for virtio-blk guest drivers that all i/o requests are sized in multiples of 512 bytes? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/2] KVM: x86 emulator: Cleanup emulate_push() writebacks
Takuya Yoshikawa wrote:
> @@ -1265,22 +1263,19 @@ int emulate_int_real(struct x86_emulate_ctxt *ctxt,
>
> /* TODO: Add limit checks */
> c->src.val = ctxt->eflags;
> - emulate_push(ctxt, ops);
> - rc = writeback(ctxt, ops);
> + rc = emulate_push(ctxt, ops);
> if (rc != X86EMUL_CONTINUE)
> return rc;
>
> ctxt->eflags &= ~(EFLG_IF | EFLG_TF | EFLG_AC);
>
> c->src.val = ops->get_segment_selector(VCPU_SREG_CS, ctxt->vcpu);
> - emulate_push(ctxt, ops);
> - rc = writeback(ctxt, ops);
> + rc = emulate_push(ctxt, ops);
> if (rc != X86EMUL_CONTINUE)
> return rc;
>
> c->src.val = c->eip;
> - emulate_push(ctxt, ops);
> - rc = writeback(ctxt, ops);
> + rc = emulate_push(ctxt, ops);
> if (rc != X86EMUL_CONTINUE)
> return rc;
I could also delete "c->dst.type = OP_NONE;" line here.
...
>
> static int em_push(struct x86_emulate_ctxt *ctxt)
> {
> - emulate_push(ctxt, ctxt->ops);
> - return X86EMUL_CONTINUE;
> + return emulate_push(ctxt, ctxt->ops);
> }
em_push() can be used instead of emulate_push() if we rearrange
the place to put em_push().
There seems to be some conflicting patches around. So If I get
some other comments, I'll rebase and resend later!
Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
