Re: [PATCH] kvm: don't call mmu_shrinker w/o used_mmu_pages
On 04/23/2012 07:48 PM, Rik van Riel wrote: > On 04/23/2012 12:40 PM, Ying Han wrote: > >> Avi, does this patch help the case as you mentioned above, where kvm >> module is loaded but no virtual machines are present ? Why we have to >> walk the empty while holding the spinlock? > > It might help marginally, but rather than defending > the patch, it might be more useful to try finding a > solution to the problem Mike and Eric are triggering. > > How can we prevent the code from taking the lock and > throwing out EPT/NPT pages when there is no real need > to, and they will be faulted back in soon anyway? We need to split the lru into an active and inactive list, just like the linux mm. The inactive list might be global (rather than vm local). Then we can just examine the inactive list, see that it's empty, and quit. Perhaps signal the guests to move some pages from the active to inactive list. This is hindered by lack of accessed bits on EPT page tables. That means we must resort to faults to reactivate shadow pages. -- 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: don't call mmu_shrinker w/o used_mmu_pages
On 04/23/2012 12:40 PM, Ying Han wrote: Avi, does this patch help the case as you mentioned above, where kvm module is loaded but no virtual machines are present ? Why we have to walk the empty while holding the spinlock? It might help marginally, but rather than defending the patch, it might be more useful to try finding a solution to the problem Mike and Eric are triggering. How can we prevent the code from taking the lock and throwing out EPT/NPT pages when there is no real need to, and they will be faulted back in soon anyway? -- All rights reversed -- 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: don't call mmu_shrinker w/o used_mmu_pages
On Sun, Apr 22, 2012 at 2:35 AM, Avi Kivity wrote: > On 04/21/2012 01:11 AM, Andrew Morton wrote: >> On Fri, 13 Apr 2012 15:38:41 -0700 >> Ying Han wrote: >> >> > The mmu_shrink() is heavy by itself by iterating all kvms and holding >> > the kvm_lock. spotted the code w/ Rik during LSF, and it turns out we >> > don't need to call the shrinker if nothing to shrink. >> > >> >> We should probably tell the kvm maintainers about this ;) >> > > > Andrew, I see you added this to -mm. First, it should go through the > kvm tree. Second, unless we misunderstand something, the patch does > nothing, so I don't think it should be added at all. Avi, does this patch help the case as you mentioned above, where kvm module is loaded but no virtual machines are present ? Why we have to walk the empty while holding the spinlock? --Ying > > -- > 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: don't call mmu_shrinker w/o used_mmu_pages
On 04/22/2012 10:05 PM, Eric Northup wrote: > On Sun, Apr 22, 2012 at 2:16 AM, Avi Kivity wrote: > > On 04/21/2012 05:15 AM, Mike Waychison wrote: > [...] > > There is no mmu_list_lock. Do you mean kvm_lock or kvm->mmu_lock? > > > > If the former, then we could easily fix this by dropping kvm_lock while > > the work is being done. If the latter, then it's more difficult. > > > > (kvm_lock being contended implies that mmu_shrink is called concurrently?) > > On a 32-core system experiencing memory pressure, mmu_shrink was often > being called concurrently (before we turned it off). > > With just one, or a small number of VMs on a host, when the > mmu_shrinker contents on the kvm_lock, that's just a proxy for the > contention on kvm->mmu_lock. It is the one that gets reported, > though, since it gets acquired first. > > The contention on mmu_lock would indeed be difficult to remove. Our > case was perhaps unusual, because of the use of memory containers. So > some cgroups were under memory pressure (thus calling the shrinker) > but the various VCPU threads (whose guest page tables were being > evicted by the shrinker) could immediately turn around and > successfully re-allocate them. That made the kvm->mmu_lock really > hot. There are two flaws at work here: first, kvm doesn't really maintain the pages in LRU or even approximate LRU order, so a hot page can be evicted. Second, the shrinker subsystem has no way to tell whether the items are recently used or not. If you're using EPT and the guest is active, likely all mmu pages are in 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
Re: [PATCH] kvm: don't call mmu_shrinker w/o used_mmu_pages
On Sun, Apr 22, 2012 at 2:16 AM, Avi Kivity wrote: > On 04/21/2012 05:15 AM, Mike Waychison wrote: [...] > There is no mmu_list_lock. Do you mean kvm_lock or kvm->mmu_lock? > > If the former, then we could easily fix this by dropping kvm_lock while > the work is being done. If the latter, then it's more difficult. > > (kvm_lock being contended implies that mmu_shrink is called concurrently?) On a 32-core system experiencing memory pressure, mmu_shrink was often being called concurrently (before we turned it off). With just one, or a small number of VMs on a host, when the mmu_shrinker contents on the kvm_lock, that's just a proxy for the contention on kvm->mmu_lock. It is the one that gets reported, though, since it gets acquired first. The contention on mmu_lock would indeed be difficult to remove. Our case was perhaps unusual, because of the use of memory containers. So some cgroups were under memory pressure (thus calling the shrinker) but the various VCPU threads (whose guest page tables were being evicted by the shrinker) could immediately turn around and successfully re-allocate them. That made the kvm->mmu_lock really hot. -- 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: don't call mmu_shrinker w/o used_mmu_pages
On 04/21/2012 01:11 AM, Andrew Morton wrote: > On Fri, 13 Apr 2012 15:38:41 -0700 > Ying Han wrote: > > > The mmu_shrink() is heavy by itself by iterating all kvms and holding > > the kvm_lock. spotted the code w/ Rik during LSF, and it turns out we > > don't need to call the shrinker if nothing to shrink. > > > > We should probably tell the kvm maintainers about this ;) > Andrew, I see you added this to -mm. First, it should go through the kvm tree. Second, unless we misunderstand something, the patch does nothing, so I don't think it should be added at all. -- 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: don't call mmu_shrinker w/o used_mmu_pages
On 04/21/2012 05:15 AM, Mike Waychison wrote: > On Fri, Apr 20, 2012 at 6:56 PM, Takuya Yoshikawa > wrote: > > On Fri, 20 Apr 2012 16:07:41 -0700 > > Ying Han wrote: > > > >> My understanding of the real pain is the poor implementation of the > >> mmu_shrinker. It iterates all the registered mmu_shrink callbacks for > >> each kvm and only does little work at a time while holding two big > >> locks. I learned from mikew@ (also ++cc-ed) that is causing latency > >> spikes and unfairness among kvm instance in some of the experiment > >> we've seen. > > The pains we have with mmu_shrink are twofold: > > - Memory pressure against the shinker applies globally. Any task can > cause pressure within their own environment (using numa or memcg) and > cause the global shrinker to shrink all shadowed tables on the system > (regardless of how memory is isolated between tasks). > - Massive lock contention when all these CPUs are hitting the global > lock (which backs everybody on the system up). > > In our situation, we simple disable the shrinker altogether. My > understanding is that we EPT or NPT, the amount of memory used by > these tables is bounded by the size of guest physical memory, whereas > with software shadowed tables, it is bounded by the addresses spaces > in the guest. There is also a 2% (default) bound enforced on a per-vm basis. > This bound makes it reasonable to not do any reclaim > and charge it as a "system overhead tax". > > As for data, the most impressive result was a massive improvement in > round-trip latency to a webserver running in a guest while another > process on the system was thrashing through page-cache (on a dozen or > so spinning disks iirc). We were using fake-numa, and would otherwise > not expect the antagonist to drastrically affect the latency-sensitive > task (as per a lot of effort into making that work). Unfortunately, > we saw the 99th%ile latency riding at the 140ms timeout cut-off (they > were likely tailing out much longer), with the 95%ile at over 40ms. > With the mmu_shrinker disabled, the 99th%ile latency quickly dropped > down to about 20ms. > > CPU profiles were showing 30% of cpu time wasted on spinlocks, all the > mmu_list_lock iirc. > > In our case, I'm much happier just disabling the damned thing altogether. > There is no mmu_list_lock. Do you mean kvm_lock or kvm->mmu_lock? If the former, then we could easily fix this by dropping kvm_lock while the work is being done. If the latter, then it's more difficult. (kvm_lock being contended implies that mmu_shrink is called concurrently?) -- 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: don't call mmu_shrinker w/o used_mmu_pages
On 04/21/2012 01:11 AM, Andrew Morton wrote:
> On Fri, 13 Apr 2012 15:38:41 -0700
> Ying Han wrote:
>
> > The mmu_shrink() is heavy by itself by iterating all kvms and holding
> > the kvm_lock. spotted the code w/ Rik during LSF, and it turns out we
> > don't need to call the shrinker if nothing to shrink.
> >
>
> We should probably tell the kvm maintainers about this ;)
>
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -188,6 +188,11 @@ static u64 __read_mostly shadow_mmio_mask;
> >
> > static void mmu_spte_set(u64 *sptep, u64 spte);
> >
> > +static inline int get_kvm_total_used_mmu_pages()
> > +{
> > + return percpu_counter_read_positive(&kvm_total_used_mmu_pages);
> > +}
> > +
> > void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask)
> > {
> > shadow_mmio_mask = mmio_mask;
> > @@ -3900,6 +3905,9 @@ static int mmu_shrink(struct shrinker *shrink, struct
> > shrink_control *sc)
> > if (nr_to_scan == 0)
> > goto out;
> >
> > + if (!get_kvm_total_used_mmu_pages())
> > + return 0;
> > +
> > raw_spin_lock(&kvm_lock);
> >
> > list_for_each_entry(kvm, &vm_list, vm_list) {
> > @@ -3926,7 +3934,7 @@ static int mmu_shrink(struct shrinker *shrink, struct
> > shrink_control *sc)
> > raw_spin_unlock(&kvm_lock);
> >
> > out:
> > - return percpu_counter_read_positive(&kvm_total_used_mmu_pages);
> > + return get_kvm_total_used_mmu_pages();
> > }
> >
> > static struct shrinker mmu_shrinker = {
>
> There's a small functional change: percpu_counter_read_positive() is an
> approximate thing, so there will be cases where there will be some
> pages which are accounted for only in the percpu_counter's per-cpu
> accumulators. In that case mmu_shrink() will bale out when there are
> in fact some freeable pages available. This is hopefully unimportant.
>
> Do we actually know that this patch helps anything? Any measurements? Is
> kvm_total_used_mmu_pages==0 at all common?
>
It's very common - this corresponds to the case where the kvm module is
loaded but no virtual machines are present. But in that case the
shrinker loop is not at all heavy - take a lock, iterate over an empty
list, release lock.
--
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: don't call mmu_shrinker w/o used_mmu_pages
On Fri, Apr 20, 2012 at 7:29 PM, Takuya Yoshikawa wrote: > On Fri, 20 Apr 2012 19:15:24 -0700 > Mike Waychison wrote: > >> In our situation, we simple disable the shrinker altogether. My >> understanding is that we EPT or NPT, the amount of memory used by >> these tables is bounded by the size of guest physical memory, whereas >> with software shadowed tables, it is bounded by the addresses spaces >> in the guest. This bound makes it reasonable to not do any reclaim >> and charge it as a "system overhead tax". > > IIRC, KVM's mmu_shrink is mainly for protecting the host from pathological > guest without EPT or NPT. > > You can see Avi's summary: -- http://www.spinics.net/lists/kvm/msg65671.html > === > We should aim for the following: > - normal operation causes very little shrinks (some are okay) > - high pressure mostly due to kvm results in kvm being shrunk (this is a > pathological case caused by a starting a guest with a huge amount of > memory, and mapping it all to /dev/zero (or ksm), and getting the guest > the create shadow mappings for all of it) > - general high pressure is shared among other caches like dcache and icache > > The cost of reestablishing an mmu page can be as high as half a > millisecond of cpu time, which is the reason I want to be conservative. To add to that, on these systems (32-way), the fault itself isn't as heavy-handed as a global lock in everyone's reclaim path :) I'd be very happy if this stuff was memcg aware, but until that happens, this code is disabled in our production builds. 30% of CPU time lost to a spinlock when mixing VMs with IO is worth paying the < 1% of system ram these pages cost if it means tighter/more-deterministic service latencies. > === > > Thanks, > 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
Re: [PATCH] kvm: don't call mmu_shrinker w/o used_mmu_pages
On Fri, 20 Apr 2012 19:15:24 -0700 Mike Waychison wrote: > In our situation, we simple disable the shrinker altogether. My > understanding is that we EPT or NPT, the amount of memory used by > these tables is bounded by the size of guest physical memory, whereas > with software shadowed tables, it is bounded by the addresses spaces > in the guest. This bound makes it reasonable to not do any reclaim > and charge it as a "system overhead tax". IIRC, KVM's mmu_shrink is mainly for protecting the host from pathological guest without EPT or NPT. You can see Avi's summary: -- http://www.spinics.net/lists/kvm/msg65671.html === We should aim for the following: - normal operation causes very little shrinks (some are okay) - high pressure mostly due to kvm results in kvm being shrunk (this is a pathological case caused by a starting a guest with a huge amount of memory, and mapping it all to /dev/zero (or ksm), and getting the guest the create shadow mappings for all of it) - general high pressure is shared among other caches like dcache and icache The cost of reestablishing an mmu page can be as high as half a millisecond of cpu time, which is the reason I want to be conservative. === Thanks, 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
Re: [PATCH] kvm: don't call mmu_shrinker w/o used_mmu_pages
On Fri, Apr 20, 2012 at 6:56 PM, Takuya Yoshikawa wrote: > On Fri, 20 Apr 2012 16:07:41 -0700 > Ying Han wrote: > >> My understanding of the real pain is the poor implementation of the >> mmu_shrinker. It iterates all the registered mmu_shrink callbacks for >> each kvm and only does little work at a time while holding two big >> locks. I learned from mikew@ (also ++cc-ed) that is causing latency >> spikes and unfairness among kvm instance in some of the experiment >> we've seen. The pains we have with mmu_shrink are twofold: - Memory pressure against the shinker applies globally. Any task can cause pressure within their own environment (using numa or memcg) and cause the global shrinker to shrink all shadowed tables on the system (regardless of how memory is isolated between tasks). - Massive lock contention when all these CPUs are hitting the global lock (which backs everybody on the system up). In our situation, we simple disable the shrinker altogether. My understanding is that we EPT or NPT, the amount of memory used by these tables is bounded by the size of guest physical memory, whereas with software shadowed tables, it is bounded by the addresses spaces in the guest. This bound makes it reasonable to not do any reclaim and charge it as a "system overhead tax". As for data, the most impressive result was a massive improvement in round-trip latency to a webserver running in a guest while another process on the system was thrashing through page-cache (on a dozen or so spinning disks iirc). We were using fake-numa, and would otherwise not expect the antagonist to drastrically affect the latency-sensitive task (as per a lot of effort into making that work). Unfortunately, we saw the 99th%ile latency riding at the 140ms timeout cut-off (they were likely tailing out much longer), with the 95%ile at over 40ms. With the mmu_shrinker disabled, the 99th%ile latency quickly dropped down to about 20ms. CPU profiles were showing 30% of cpu time wasted on spinlocks, all the mmu_list_lock iirc. In our case, I'm much happier just disabling the damned thing altogether. > > Last year, I discussed the mmu_shrink issues on kvm ML: > > [PATCH 0/4] KVM: Make mmu_shrink() scan nr_to_scan shadow pages > http://www.spinics.net/lists/kvm/msg65231.html > > Sadly, we could not find any good way at that time. > > Thanks, > 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
Re: [PATCH] kvm: don't call mmu_shrinker w/o used_mmu_pages
On Fri, 20 Apr 2012 16:07:41 -0700 Ying Han wrote: > My understanding of the real pain is the poor implementation of the > mmu_shrinker. It iterates all the registered mmu_shrink callbacks for > each kvm and only does little work at a time while holding two big > locks. I learned from mikew@ (also ++cc-ed) that is causing latency > spikes and unfairness among kvm instance in some of the experiment > we've seen. Last year, I discussed the mmu_shrink issues on kvm ML: [PATCH 0/4] KVM: Make mmu_shrink() scan nr_to_scan shadow pages http://www.spinics.net/lists/kvm/msg65231.html Sadly, we could not find any good way at that time. Thanks, 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
Re: [PATCH] kvm: don't call mmu_shrinker w/o used_mmu_pages
On Fri, Apr 20, 2012 at 3:53 PM, Rik van Riel wrote: > On 04/20/2012 06:11 PM, Andrew Morton wrote: >> >> On Fri, 13 Apr 2012 15:38:41 -0700 >> Ying Han wrote: >> >>> The mmu_shrink() is heavy by itself by iterating all kvms and holding >>> the kvm_lock. spotted the code w/ Rik during LSF, and it turns out we >>> don't need to call the shrinker if nothing to shrink. > > >>> @@ -3900,6 +3905,9 @@ static int mmu_shrink(struct shrinker *shrink, >>> struct shrink_control *sc) >>> if (nr_to_scan == 0) >>> goto out; >>> >>> + if (!get_kvm_total_used_mmu_pages()) >>> + return 0; >>> + > > >> Do we actually know that this patch helps anything? Any measurements? Is >> kvm_total_used_mmu_pages==0 at all common? >> > > On re-reading mmu.c, it looks like even with EPT or NPT, > we end up creating mmu pages for the nested page tables. I think you are right here. So the patch doesn't help the real pain. My understanding of the real pain is the poor implementation of the mmu_shrinker. It iterates all the registered mmu_shrink callbacks for each kvm and only does little work at a time while holding two big locks. I learned from mikew@ (also ++cc-ed) that is causing latency spikes and unfairness among kvm instance in some of the experiment we've seen. Mike might tell more on that. --Ying > > I have not had the time to look into it more, but it would > be nice to know if the patch has any effect at all. > > -- > All rights reversed -- 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: don't call mmu_shrinker w/o used_mmu_pages
On 04/20/2012 06:11 PM, Andrew Morton wrote: On Fri, 13 Apr 2012 15:38:41 -0700 Ying Han wrote: The mmu_shrink() is heavy by itself by iterating all kvms and holding the kvm_lock. spotted the code w/ Rik during LSF, and it turns out we don't need to call the shrinker if nothing to shrink. @@ -3900,6 +3905,9 @@ static int mmu_shrink(struct shrinker *shrink, struct shrink_control *sc) if (nr_to_scan == 0) goto out; + if (!get_kvm_total_used_mmu_pages()) + return 0; + Do we actually know that this patch helps anything? Any measurements? Is kvm_total_used_mmu_pages==0 at all common? On re-reading mmu.c, it looks like even with EPT or NPT, we end up creating mmu pages for the nested page tables. I have not had the time to look into it more, but it would be nice to know if the patch has any effect at all. -- All rights reversed -- 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: don't call mmu_shrinker w/o used_mmu_pages
On Fri, 13 Apr 2012 15:38:41 -0700
Ying Han wrote:
> The mmu_shrink() is heavy by itself by iterating all kvms and holding
> the kvm_lock. spotted the code w/ Rik during LSF, and it turns out we
> don't need to call the shrinker if nothing to shrink.
>
We should probably tell the kvm maintainers about this ;)
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -188,6 +188,11 @@ static u64 __read_mostly shadow_mmio_mask;
>
> static void mmu_spte_set(u64 *sptep, u64 spte);
>
> +static inline int get_kvm_total_used_mmu_pages()
> +{
> + return percpu_counter_read_positive(&kvm_total_used_mmu_pages);
> +}
> +
> void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask)
> {
> shadow_mmio_mask = mmio_mask;
> @@ -3900,6 +3905,9 @@ static int mmu_shrink(struct shrinker *shrink, struct
> shrink_control *sc)
> if (nr_to_scan == 0)
> goto out;
>
> + if (!get_kvm_total_used_mmu_pages())
> + return 0;
> +
> raw_spin_lock(&kvm_lock);
>
> list_for_each_entry(kvm, &vm_list, vm_list) {
> @@ -3926,7 +3934,7 @@ static int mmu_shrink(struct shrinker *shrink, struct
> shrink_control *sc)
> raw_spin_unlock(&kvm_lock);
>
> out:
> - return percpu_counter_read_positive(&kvm_total_used_mmu_pages);
> + return get_kvm_total_used_mmu_pages();
> }
>
> static struct shrinker mmu_shrinker = {
There's a small functional change: percpu_counter_read_positive() is an
approximate thing, so there will be cases where there will be some
pages which are accounted for only in the percpu_counter's per-cpu
accumulators. In that case mmu_shrink() will bale out when there are
in fact some freeable pages available. This is hopefully unimportant.
Do we actually know that this patch helps anything? Any measurements? Is
kvm_total_used_mmu_pages==0 at all common?
--
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
