From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH for-4.5 v6 10/17] xen/arm: p2m type definitions and changes Date: Tue, 16 Sep 2014 23:52:57 +0200 Message-ID: References: <1410789775-24197-1-git-send-email-tklengyel@sec.in.tum.de> <1410789775-24197-11-git-send-email-tklengyel@sec.in.tum.de> <1410820555.6908.14.camel@hastur.hellion.org.uk> <1410874037.18008.2.camel@hastur.hellion.org.uk> <54189FAC.50308@linaro.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8738085515524607666==" Return-path: In-Reply-To: <54189FAC.50308@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: Ian Campbell , Tim Deegan , Ian Jackson , "xen-devel@lists.xen.org" , Stefano Stabellini , Andres Lagar-Cavilla , Jan Beulich , Daniel De Graaf , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org --===============8738085515524607666== Content-Type: multipart/alternative; boundary=001a113a53d2373273050335c6e4 --001a113a53d2373273050335c6e4 Content-Type: text/plain; charset=ISO-8859-1 On Tue, Sep 16, 2014 at 10:38 PM, Julien Grall wrote: > Hi Ian, > > > On 16/09/14 06:27, Ian Campbell wrote: > >> On Tue, 2014-09-16 at 10:49 +0200, Tamas K Lengyel wrote: >> >>> >>> >>> On Tue, Sep 16, 2014 at 12:35 AM, Ian Campbell >>> wrote: >>> On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrote: >>> > Define p2m_access_t in ARM and add necessary changes for >>> page table >>> > construction routines to pass the default access >>> information. Also, >>> > define the Radix tree that will hold access permission >>> settings as >>> > the PTE's don't have enough software programmable bits >>> available. >>> >>> So my main concern here is the overhead for non-xenaccess >>> users. I think >>> it amounts to a few extra fields in the p2m_domain struct >>> which I can >>> see here and presumably some NULL vs. non-NULL type checks >>> which I guess >>> we will get to later. The important thing is that the fast >>> paths for the >>> common case don't get a lot of extra overhead. >>> >>> WRT the xenaccess performance did you consider any options >>> other than a >>> radix tree (which seems quite expensive to me)? e.g. perhaps >>> allocating >>> (only when needed) as second page for each real T page as a >>> shadow/extended region? Perhaps pointed to by a filed in the >>> real PT >>> struct page_info. I'm sure there are other possible ideas too. >>> >>> Ian, >>> >>> >>> Those would all be possible solutions. I used the Radix tree >>> implementation already in Xen as a matter of convenience and because >>> it has an acceptable overhead. I certainly don't oppose further >>> optimizing this code, I'm just not sure if it needs to happen now, >>> provided feature freeze is rapidly approaching. The main concern >>> should be impact on non-xenaccess code-paths, which I agree are a >>> showstopper of any feature like this. If you say it can't be merged >>> unless the xenaccess code-path is also optimized I'm afraid this >>> series will be postponed till 4.6 as I won't have the time to test out >>> which approach puts the least overhead on the system under what >>> usage-scenarios etc in this timeframe. >>> >> >> WRT merging I'm only concerned about the impact on non-xenaccess uses, >> the stuff about the xenaccess-on case was just idle wondering, sorry for >> not making that clear. >> > > I didn't write this code, but read it multiple time and ask the similar > question to Tamas few version ago. > > So, the radix tree is only used when the access type of the page is > different than access_rwx. This is the default access when xenaccess is not > used. > > AFAIU, the only overhead we have is the new fields in the arch_domain > structure. > > Regards, > > -- > Julien Grall > Yeap, that's the basic outline of things (except that one thing I mentioned in the other thread, which is already fixed in v7 of the series). I looked into switching to an extended struct page_info and that looks like a good alternative to consider. It seems to be like a faster lookup to get that struct with get_page_from_gva then the radix lookup if the radix tree is large. Tamas --001a113a53d2373273050335c6e4 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable


On Tue, Sep 16, 2014 at 10:38 PM, Julien Grall <julien.grall@lin= aro.org> wrote:
Hi Ian,


On 16/09/14 06:27, Ian Campbell wrote:
On Tue, 2014-09-16 at 10:49 +0200, Tamas K Lengyel wrote:


On Tue, Sep 16, 2014 at 12:35 AM, Ian Campbell
<ian.campbe= ll@citrix.com> wrote:
=A0 =A0 =A0 =A0 =A0On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrote= :
=A0 =A0 =A0 =A0 =A0> Define p2m_access_t in ARM and add necessary change= s for
=A0 =A0 =A0 =A0 =A0page table
=A0 =A0 =A0 =A0 =A0> construction routines to pass the default access =A0 =A0 =A0 =A0 =A0information. Also,
=A0 =A0 =A0 =A0 =A0> define the Radix tree that will hold access permiss= ion
=A0 =A0 =A0 =A0 =A0settings as
=A0 =A0 =A0 =A0 =A0> the PTE's don't have enough software progra= mmable bits
=A0 =A0 =A0 =A0 =A0available.

=A0 =A0 =A0 =A0 =A0So my main concern here is the overhead for non-xenacces= s
=A0 =A0 =A0 =A0 =A0users. I think
=A0 =A0 =A0 =A0 =A0it amounts to a few extra fields in the p2m_domain struc= t
=A0 =A0 =A0 =A0 =A0which I can
=A0 =A0 =A0 =A0 =A0see here and presumably some NULL vs. non-NULL type chec= ks
=A0 =A0 =A0 =A0 =A0which I guess
=A0 =A0 =A0 =A0 =A0we will get to later. The important thing is that the fa= st
=A0 =A0 =A0 =A0 =A0paths for the
=A0 =A0 =A0 =A0 =A0common case don't get a lot of extra overhead.

=A0 =A0 =A0 =A0 =A0WRT the xenaccess performance did you consider any optio= ns
=A0 =A0 =A0 =A0 =A0other than a
=A0 =A0 =A0 =A0 =A0radix tree (which seems quite expensive to me)? e.g. per= haps
=A0 =A0 =A0 =A0 =A0allocating
=A0 =A0 =A0 =A0 =A0(only when needed) as second page for each real T page a= s a
=A0 =A0 =A0 =A0 =A0shadow/extended region? Perhaps pointed to by a filed in= the
=A0 =A0 =A0 =A0 =A0real PT
=A0 =A0 =A0 =A0 =A0struct page_info. I'm sure there are other possible = ideas too.

=A0 =A0 =A0 =A0 =A0Ian,


Those would all be possible solutions. I used the Radix tree
implementation already in Xen as a matter of convenience and because
it has an acceptable overhead. I certainly don't oppose further
optimizing this code, I'm just not sure if it needs to happen now,
provided feature freeze is rapidly approaching. The main concern
should be impact on non-xenaccess code-paths, which I agree are a
showstopper of any feature like this. If you say it can't be merged
unless the xenaccess code-path is also optimized I'm afraid this
series will be postponed till 4.6 as I won't have the time to test out<= br> which approach puts the least overhead on the system under what
usage-scenarios etc in this timeframe.

WRT merging I'm only concerned about the impact on non-xenaccess uses,<= br> the stuff about the xenaccess-on case was just idle wondering, sorry for not making that clear.

I didn't write this code, but read it multiple time and ask the similar= question to Tamas few version ago.

So, the radix tree is only used when the access type of the page is differe= nt than access_rwx. This is the default access when xenaccess is not used. =

AFAIU, the only overhead we have is the new fields in the arch_domain struc= ture.

Regards,

--
Julien Grall


Yeap, that's the basic ou= tline of things (except that one thing I=20 mentioned in the other thread, which is already fixed in v7 of the=20 series). I looked into switching to an extended struct page_info and=20 that looks like a good alternative to consider. It seems to be like a=20 faster lookup to get that struct with get_page_from_gva then the radix=20 lookup if the radix tree is large.

Tamas
--001a113a53d2373273050335c6e4-- --===============8738085515524607666== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8738085515524607666==--