| Message ID | 1387387873-3269-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 18 Dec 2013, Julien Grall wrote: > When Xen is removing/relinquishing mapping, it will create second/third tables > if they don't exist. > > Non-existent table means the address range was never mapped, so Xen can safely > skip them. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Release: This is an improvement for Xen 4.4. It will save time during > relinquish phase and avoid dummy allocation. > The downside is the patch is modifying p2m loop which is used everywhere. > --- > xen/arch/arm/p2m.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d24a6fc..9ef8819 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -246,10 +246,12 @@ static int create_p2m_entries(struct domain *d, > cur_first_offset = ~0, > cur_second_offset = ~0; > unsigned long count = 0; > + bool_t populate = (op == INSERT || op == ALLOCATE); why don't you just check for op == REMOVE? > spin_lock(&p2m->lock); > > - for(addr = start_gpaddr; addr < end_gpaddr; addr += PAGE_SIZE) > + addr = start_gpaddr; > + while ( addr < end_gpaddr ) > { > if ( cur_first_page != p2m_first_level_index(addr) ) > { > @@ -265,8 +267,15 @@ static int create_p2m_entries(struct domain *d, > > if ( !first[first_table_offset(addr)].p2m.valid ) > { > + if ( !populate ) > + { > + addr += FIRST_SIZE; > + continue; > + } > + > rc = p2m_create_table(d, &first[first_table_offset(addr)]); > - if ( rc < 0 ) { > + if ( rc < 0 ) > + { > printk("p2m_populate_ram: L1 failed\n"); > goto out; > } > @@ -284,6 +293,12 @@ static int create_p2m_entries(struct domain *d, > > if ( !second[second_table_offset(addr)].p2m.valid ) > { > + if ( !populate ) > + { > + addr += SECOND_SIZE; > + continue; > + } > + > rc = p2m_create_table(d, &second[second_table_offset(addr)]); > if ( rc < 0 ) { > printk("p2m_populate_ram: L2 failed\n"); > @@ -372,6 +387,9 @@ static int create_p2m_entries(struct domain *d, > } > count = 0; > } > + > + /* Got the next page */ > + addr += PAGE_SIZE; > } > > if ( op == ALLOCATE || op == INSERT ) > -- > 1.7.10.4 >
On 12/18/2013 07:28 PM, Stefano Stabellini wrote: > On Wed, 18 Dec 2013, Julien Grall wrote: >> When Xen is removing/relinquishing mapping, it will create second/third tables >> if they don't exist. >> >> Non-existent table means the address range was never mapped, so Xen can safely >> skip them. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Release: This is an improvement for Xen 4.4. It will save time during >> relinquish phase and avoid dummy allocation. >> The downside is the patch is modifying p2m loop which is used everywhere. >> --- >> xen/arch/arm/p2m.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d24a6fc..9ef8819 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -246,10 +246,12 @@ static int create_p2m_entries(struct domain *d, >> cur_first_offset = ~0, >> cur_second_offset = ~0; >> unsigned long count = 0; >> + bool_t populate = (op == INSERT || op == ALLOCATE); > > why don't you just check for op == REMOVE? Because you also need to check op == RELINQUISH (added by my patch series on foreign mapping).
On Wed, 18 Dec 2013, Julien Grall wrote: > On 12/18/2013 07:28 PM, Stefano Stabellini wrote: > > On Wed, 18 Dec 2013, Julien Grall wrote: > > > When Xen is removing/relinquishing mapping, it will create second/third > > > tables > > > if they don't exist. > > > > > > Non-existent table means the address range was never mapped, so Xen can > > > safely > > > skip them. > > > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > > > --- > > > Release: This is an improvement for Xen 4.4. It will save time during > > > relinquish phase and avoid dummy allocation. > > > The downside is the patch is modifying p2m loop which is used > > > everywhere. > > > --- > > > xen/arch/arm/p2m.c | 22 ++++++++++++++++++++-- > > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > > index d24a6fc..9ef8819 100644 > > > --- a/xen/arch/arm/p2m.c > > > +++ b/xen/arch/arm/p2m.c > > > @@ -246,10 +246,12 @@ static int create_p2m_entries(struct domain *d, > > > cur_first_offset = ~0, > > > cur_second_offset = ~0; > > > unsigned long count = 0; > > > + bool_t populate = (op == INSERT || op == ALLOCATE); > > > > why don't you just check for op == REMOVE? > > Because you also need to check op == RELINQUISH (added by my patch series on > foreign mapping). I see. In that case Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
On Wed, 2013-12-18 at 17:31 +0000, Julien Grall wrote: > When Xen is removing/relinquishing mapping, it will create second/third tables > if they don't exist. > > Non-existent table means the address range was never mapped, so Xen can safely > skip them. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Release: This is an improvement for Xen 4.4. It will save time during > relinquish phase and avoid dummy allocation. > The downside is the patch is modifying p2m loop which is used everywhere. > --- > xen/arch/arm/p2m.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d24a6fc..9ef8819 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -246,10 +246,12 @@ static int create_p2m_entries(struct domain *d, > cur_first_offset = ~0, > cur_second_offset = ~0; > unsigned long count = 0; > + bool_t populate = (op == INSERT || op == ALLOCATE); > > spin_lock(&p2m->lock); > > - for(addr = start_gpaddr; addr < end_gpaddr; addr += PAGE_SIZE) > + addr = start_gpaddr; > + while ( addr < end_gpaddr ) > { > if ( cur_first_page != p2m_first_level_index(addr) ) > { > @@ -265,8 +267,15 @@ static int create_p2m_entries(struct domain *d, > > if ( !first[first_table_offset(addr)].p2m.valid ) > { > + if ( !populate ) > + { > + addr += FIRST_SIZE; I think this subtly does the wrong thing if addr is not FIRST_SIZE aligned, which it might be on the first iteration. That will skip the start of the next 1st level block. I think addr needs to be rounded up to the next first boundary. > + continue; > + } > + > rc = p2m_create_table(d, &first[first_table_offset(addr)]); > - if ( rc < 0 ) { > + if ( rc < 0 ) > + { > printk("p2m_populate_ram: L1 failed\n"); > goto out; > } > @@ -284,6 +293,12 @@ static int create_p2m_entries(struct domain *d, > > if ( !second[second_table_offset(addr)].p2m.valid ) > { > + if ( !populate ) > + { > + addr += SECOND_SIZE; Likewise here. > + continue; > + } > + > rc = p2m_create_table(d, &second[second_table_offset(addr)]); > if ( rc < 0 ) { > printk("p2m_populate_ram: L2 failed\n"); > @@ -372,6 +387,9 @@ static int create_p2m_entries(struct domain *d, > } > count = 0; > } > + > + /* Got the next page */ > + addr += PAGE_SIZE; > } > > if ( op == ALLOCATE || op == INSERT )
On 12/19/2013 09:38 AM, Ian Campbell wrote: > On Wed, 2013-12-18 at 17:31 +0000, Julien Grall wrote: >> When Xen is removing/relinquishing mapping, it will create second/third tables >> if they don't exist. >> >> Non-existent table means the address range was never mapped, so Xen can safely >> skip them. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Release: This is an improvement for Xen 4.4. It will save time during >> relinquish phase and avoid dummy allocation. >> The downside is the patch is modifying p2m loop which is used everywhere. >> --- >> xen/arch/arm/p2m.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d24a6fc..9ef8819 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -246,10 +246,12 @@ static int create_p2m_entries(struct domain *d, >> cur_first_offset = ~0, >> cur_second_offset = ~0; >> unsigned long count = 0; >> + bool_t populate = (op == INSERT || op == ALLOCATE); >> >> spin_lock(&p2m->lock); >> >> - for(addr = start_gpaddr; addr < end_gpaddr; addr += PAGE_SIZE) >> + addr = start_gpaddr; >> + while ( addr < end_gpaddr ) >> { >> if ( cur_first_page != p2m_first_level_index(addr) ) >> { >> @@ -265,8 +267,15 @@ static int create_p2m_entries(struct domain *d, >> >> if ( !first[first_table_offset(addr)].p2m.valid ) >> { >> + if ( !populate ) >> + { >> + addr += FIRST_SIZE; > > I think this subtly does the wrong thing if addr is not FIRST_SIZE > aligned, which it might be on the first iteration. That will skip the > start of the next 1st level block. > > I think addr needs to be rounded up to the next first boundary. Right, I will send a new version of this patch.
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d24a6fc..9ef8819 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -246,10 +246,12 @@ static int create_p2m_entries(struct domain *d, cur_first_offset = ~0, cur_second_offset = ~0; unsigned long count = 0; + bool_t populate = (op == INSERT || op == ALLOCATE); spin_lock(&p2m->lock); - for(addr = start_gpaddr; addr < end_gpaddr; addr += PAGE_SIZE) + addr = start_gpaddr; + while ( addr < end_gpaddr ) { if ( cur_first_page != p2m_first_level_index(addr) ) { @@ -265,8 +267,15 @@ static int create_p2m_entries(struct domain *d, if ( !first[first_table_offset(addr)].p2m.valid ) { + if ( !populate ) + { + addr += FIRST_SIZE; + continue; + } + rc = p2m_create_table(d, &first[first_table_offset(addr)]); - if ( rc < 0 ) { + if ( rc < 0 ) + { printk("p2m_populate_ram: L1 failed\n"); goto out; } @@ -284,6 +293,12 @@ static int create_p2m_entries(struct domain *d, if ( !second[second_table_offset(addr)].p2m.valid ) { + if ( !populate ) + { + addr += SECOND_SIZE; + continue; + } + rc = p2m_create_table(d, &second[second_table_offset(addr)]); if ( rc < 0 ) { printk("p2m_populate_ram: L2 failed\n"); @@ -372,6 +387,9 @@ static int create_p2m_entries(struct domain *d, } count = 0; } + + /* Got the next page */ + addr += PAGE_SIZE; } if ( op == ALLOCATE || op == INSERT )
When Xen is removing/relinquishing mapping, it will create second/third tables if they don't exist. Non-existent table means the address range was never mapped, so Xen can safely skip them. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Release: This is an improvement for Xen 4.4. It will save time during relinquish phase and avoid dummy allocation. The downside is the patch is modifying p2m loop which is used everywhere. --- xen/arch/arm/p2m.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)