All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Graf <agraf@suse.de>
To: Gleb Natapov <gleb@redhat.com>
Cc: "\"“tiejun.chen”\"" <tiejun.chen@windriver.com>,
	"Bhushan Bharat-R65777" <R65777@freescale.com>,
	kvm-ppc@vger.kernel.org,
	"kvm@vger.kernel.org list" <kvm@vger.kernel.org>,
	"Wood Scott-B07421" <B07421@freescale.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Andrea Arcangeli" <aarcange@redhat.com>
Subject: Re: [PATCH 2/2] kvm: powerpc: set cache coherency only for kernel managed pages
Date: Wed, 24 Jul 2013 12:25:18 +0200	[thread overview]
Message-ID: <64EAFC75-2C30-4DCF-9DD2-132D5F286667@suse.de> (raw)
In-Reply-To: <20130724101932.GA14229@redhat.com>


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 :).


Alex

> understand huge page tricks they are there to guaranty that THP does not
> change flags under us, Andrea?
> 
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


WARNING: multiple messages have this Message-ID (diff)
From: Alexander Graf <agraf@suse.de>
To: Gleb Natapov <gleb@redhat.com>
Cc: "\"“tiejun.chen”\"" <tiejun.chen@windriver.com>,
	"Bhushan Bharat-R65777" <R65777@freescale.com>,
	kvm-ppc@vger.kernel.org,
	"kvm@vger.kernel.org list" <kvm@vger.kernel.org>,
	"Wood Scott-B07421" <B07421@freescale.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Andrea Arcangeli" <aarcange@redhat.com>
Subject: Re: [PATCH 2/2] kvm: powerpc: set cache coherency only for kernel managed pages
Date: Wed, 24 Jul 2013 10:25:18 +0000	[thread overview]
Message-ID: <64EAFC75-2C30-4DCF-9DD2-132D5F286667@suse.de> (raw)
In-Reply-To: <20130724101932.GA14229@redhat.com>


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 :).


Alex

> understand huge page tricks they are there to guaranty that THP does not
> change flags under us, Andrea?
> 
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


  reply	other threads:[~2013-07-24 10:25 UTC|newest]

Thread overview: 82+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-18  6:04 [PATCH 1/2] kvm: powerpc: Do not ignore "E" attribute in mas2 Bharat Bhushan
2013-07-18  6:16 ` Bharat Bhushan
2013-07-18  6:04 ` [PATCH 2/2] kvm: powerpc: set cache coherency only for kernel managed pages Bharat Bhushan
2013-07-18  6:16   ` Bharat Bhushan
2013-07-18  6:26   ` "“tiejun.chen”"
2013-07-18  6:26     ` "“tiejun.chen”"
2013-07-18  7:12     ` Bhushan Bharat-R65777
2013-07-18  7:12       ` Bhushan Bharat-R65777
2013-07-18  7:31       ` "“tiejun.chen”"
2013-07-18  7:31         ` "“tiejun.chen”"
2013-07-18  8:08         ` Bhushan Bharat-R65777
2013-07-18  8:08           ` Bhushan Bharat-R65777
2013-07-18  8:21           ` "“tiejun.chen”"
2013-07-18  8:21             ` "“tiejun.chen”"
2013-07-18  8:22             ` Bhushan Bharat-R65777
2013-07-18  8:22               ` Bhushan Bharat-R65777
2013-07-18  8:25             ` Bhushan Bharat-R65777
2013-07-18  8:25               ` Bhushan Bharat-R65777
2013-07-18  8:55               ` "“tiejun.chen”"
2013-07-18  8:55                 ` "“tiejun.chen”"
2013-07-18  9:44                 ` Alexander Graf
2013-07-18  9:44                   ` Alexander Graf
2013-07-18  9:56                   ` "“tiejun.chen”"
2013-07-18  9:56                     ` "“tiejun.chen”"
2013-07-18 10:00                     ` Alexander Graf
2013-07-18 10:00                       ` Alexander Graf
2013-07-18 10:14                       ` "“tiejun.chen”"
2013-07-18 10:14                         ` "“tiejun.chen”"
2013-07-18 16:11                       ` Scott Wood
2013-07-18 16:11                         ` Scott Wood
2013-07-18  9:48               ` Alexander Graf
2013-07-18  9:48                 ` Alexander Graf
2013-07-18  9:51                 ` Bhushan Bharat-R65777
2013-07-18 10:08                 ` "“tiejun.chen”"
2013-07-18 10:08                   ` "“tiejun.chen”"
2013-07-18 10:12                   ` Alexander Graf
2013-07-18 10:12                     ` Alexander Graf
2013-07-18 10:19                     ` "“tiejun.chen”"
2013-07-18 10:19                       ` "“tiejun.chen”"
2013-07-18 10:27                       ` Alexander Graf
2013-07-18 10:27                         ` Alexander Graf
2013-07-24  2:26                         ` "“tiejun.chen”"
2013-07-24  2:26                           ` "“tiejun.chen”"
2013-07-24  8:25                           ` Alexander Graf
2013-07-24  8:25                             ` Alexander Graf
2013-07-24  9:11                             ` Bhushan Bharat-R65777
2013-07-24  9:11                               ` Bhushan Bharat-R65777
2013-07-24  9:21                               ` Alexander Graf
2013-07-24  9:21                                 ` Alexander Graf
2013-07-24  9:35                                 ` Gleb Natapov
2013-07-24  9:35                                   ` Gleb Natapov
2013-07-24  9:39                                   ` Alexander Graf
2013-07-24  9:39                                     ` Alexander Graf
2013-07-24 20:32                                     ` Scott Wood
2013-07-24 20:32                                       ` Scott Wood
2013-07-24 20:32                                       ` Scott Wood
2013-07-25  8:50                                       ` Gleb Natapov
2013-07-25  8:50                                         ` Gleb Natapov
2013-07-25  8:50                                         ` Gleb Natapov
2013-07-25 16:07                                         ` Alexander Graf
2013-07-25 16:07                                           ` Alexander Graf
2013-07-25 16:07                                           ` Alexander Graf
2013-07-25 16:14                                           ` Gleb Natapov
2013-07-25 16:14                                             ` Gleb Natapov
2013-07-25 16:14                                             ` Gleb Natapov
2013-07-26 22:27                                         ` Scott Wood
2013-07-26 22:27                                           ` Scott Wood
2013-07-26 22:27                                           ` Scott Wood
2013-07-24 10:01                             ` Gleb Natapov
2013-07-24 10:01                               ` Gleb Natapov
2013-07-24 10:09                               ` Alexander Graf
2013-07-24 10:09                                 ` Alexander Graf
2013-07-24 10:19                                 ` Gleb Natapov
2013-07-24 10:19                                   ` Gleb Natapov
2013-07-24 10:25                                   ` Alexander Graf [this message]
2013-07-24 10:25                                     ` Alexander Graf
2013-07-24 10:30                                     ` Gleb Natapov
2013-07-24 10:30                                       ` Gleb Natapov
2013-07-25  1:04                                       ` Andrea Arcangeli
2013-07-25  1:04                                         ` Andrea Arcangeli
2013-07-18  8:27   ` "“tiejun.chen”"
2013-07-18  8:27     ` "“tiejun.chen”"

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=64EAFC75-2C30-4DCF-9DD2-132D5F286667@suse.de \
    --to=agraf@suse.de \
    --cc=B07421@freescale.com \
    --cc=R65777@freescale.com \
    --cc=aarcange@redhat.com \
    --cc=gleb@redhat.com \
    --cc=kvm-ppc@vger.kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=tiejun.chen@windriver.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.