On Wed, Jul 24, 2013 at 12:25:18PM +0200, Alexander Graf wrote:
>
> On 24.07.2013, at 12:19, Gleb Natapov wrote:
>
> > On Wed, Jul 24, 2013 at 12:09:42PM +0200, Alexander Graf wrote:
> >>
> >> On 24.07.2013, at 12:01, Gleb Natapov wrote:
> >>
> >>> Copying Andrea for him to verify that I am not talking nonsense :)
> >>>
> >>> On Wed, Jul 24, 2013 at 10:25:20AM +0200, Alexander Graf wrote:
> >>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >>>>> index 1580dd4..5e8635b 100644
> >>>>> --- a/virt/kvm/kvm_main.c
> >>>>> +++ b/virt/kvm/kvm_main.c
> >>>>> @@ -102,6 +102,10 @@ static bool largepages_enabled = true;
> >>>>>
> >>>>> bool kvm_is_mmio_pfn(pfn_t pfn)
> >>>>> {
> >>>>> +#ifdef CONFIG_MEMORY_HOTPLUG
> >>>>
> >>>> I'd feel safer if we narrow this down to e500.
> >>>>
> >>>>> + /*
> >>>>> + * Currently only in memory hot remove case we may still need this.
> >>>>> + */
> >>>>> if (pfn_valid(pfn)) {
> >>>>
> >>>> We still have to check for pfn_valid, no? So the #ifdef should be down here.
> >>>>
> >>>>> int reserved;
> >>>>> struct page *tail = pfn_to_page(pfn);
> >>>>> @@ -124,6 +128,7 @@ bool kvm_is_mmio_pfn(pfn_t pfn)
> >>>>> }
> >>>>> return PageReserved(tail);
> >>>>> }
> >>>>> +#endif
> >>>>>
> >>>>> return true;
> >>>>> }
> >>>>>
> >>>>> Before apply this change:
> >>>>>
> >>>>> real (1m19.954s + 1m20.918s + 1m22.740s + 1m21.146s + 1m22.120s)/5= 1m21.376s
> >>>>> user (0m23.181s + 0m23.550s + 0m23.506s + 0m23.410s + 0m23.520s)/5= 0m23.433s
> >>>>> sys (0m49.087s + 0m49.563s + 0m51.758s + 0m50.290s + 0m51.047s)/5= 0m50.349s
> >>>>>
> >>>>> After apply this change:
> >>>>>
> >>>>> real (1m19.507s + 1m20.919s + 1m21.436s + 1m21.179s + 1m20.293s)/5= 1m20.667s
> >>>>> user (0m22.595s + 0m22.719s + 0m22.484s + 0m22.811s + 0m22.467s)/5= 0m22.615s
> >>>>> sys (0m48.841s + 0m49.929s + 0m50.310s + 0m49.813s + 0m48.587s)/5= 0m49.496s
> >>>>>
> >>>>> So,
> >>>>>
> >>>>> real (1m20.667s - 1m21.376s)/1m21.376s x 100% = -0.6%
> >>>>> user (0m22.615s - 0m23.433s)/0m23.433s x 100% = -3.5%
> >>>>> sys (0m49.496s - 0m50.349s)/0m50.349s x 100% = -1.7%
> >>>>
> >>>> Very nice, so there is a real world performance benefit to doing this. Then yes, I think it would make sense to change the global helper function to be fast on e500 and use that one from e500_shadow_mas2_attrib() instead.
> >>>>
> >>>> Gleb, Paolo, any hard feelings?
> >>>>
> >>> I do not see how can we break the function in such a way and get
> >>> away with it. Not all valid pfns point to memory. Physical address can
> >>> be sparse (due to PCI hole, framebuffer or just because).
> >>
> >> But we don't check for sparseness today in here either. We merely check for incomplete huge pages.
> >>
> > That's not how I read the code. The code checks for reserved flag set.
> > It should be set on pfns that point to memory holes. As far as I
>
> I couldn't find any traces of code that sets the reserved bits on e500 chips though. I've only seen it getting set for memory hotplug and memory incoherent DMA code which doesn't get used on e500.
>
> But I'd be more than happy to get proven wrong :).
>
Can you write a module that scans all page structures? AFAIK all pages
are marked as reserved and then those that become regular memory are
marked as unreserved. Hope Andrea will chime in here :)
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
