| Message ID | 1474991845-27962-4-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At 17:56 +0200 on 27 Sep (1474999018), Roger Pau Monne wrote: > Return should be an int, and the number of pages should be an unsigned long. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Both those changes seem fine to me. Since you're changing the return types to int, can you please also change the two callers (hap_enable() and shadow_enable()) that assign it to an unsigned variable? AFAICS both should just pass the result through instead of forcing all errors to -ENOMEM. Cheers, Tim.
>>> On 27.09.16 at 17:56, <roger.pau@citrix.com> wrote: > Return should be an int, and the number of pages should be an unsigned long. I can see the former, but why the latter? Acting on 32-bit quantities is a little cheaper after all. Jan
On Thu, Sep 29, 2016 at 04:39:02AM -0600, Jan Beulich wrote: > >>> On 27.09.16 at 17:56, <roger.pau@citrix.com> wrote: > > Return should be an int, and the number of pages should be an unsigned long. > > I can see the former, but why the latter? Acting on 32-bit quantities > is a little cheaper after all. This was requested by Andrew in the previous version: https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg03126.html But yes, an unsigned int is enough to hold the maximum number of pages for a domain given than we support up to 1TB for HVM guests. Or maybe there are plans to increase the maximum supported memory per guest? Roger.
>>> On 29.09.16 at 16:33, <roger.pau@citrix.com> wrote: > On Thu, Sep 29, 2016 at 04:39:02AM -0600, Jan Beulich wrote: >> >>> On 27.09.16 at 17:56, <roger.pau@citrix.com> wrote: >> > Return should be an int, and the number of pages should be an unsigned long. >> >> I can see the former, but why the latter? Acting on 32-bit quantities >> is a little cheaper after all. > > This was requested by Andrew in the previous version: > > https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg03126.html > > But yes, an unsigned int is enough to hold the maximum number of pages for > a domain given than we support up to 1TB for HVM guests. Or maybe there are > plans to increase the maximum supported memory per guest? Larger guests or not - here we're talking about number of pages used internally for page tables, not numbers of pages assigned to the guest, aren't we? In any event I'm not sure which truncation issue Andrew had spotted back then. Jan
On 27/09/16 16:56, Roger Pau Monne wrote: > Return should be an int, and the number of pages should be an unsigned long. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Non-shadow mm bits: Acked-by: George Dunlap <george.dunlap@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Tim Deegan <tim@xen.org> > --- > xen/arch/x86/mm/hap/hap.c | 6 +++--- > xen/arch/x86/mm/shadow/common.c | 7 +++---- > xen/include/asm-x86/domain.h | 12 ++++++------ > 3 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c > index 3218fa2..b6d2c61 100644 > --- a/xen/arch/x86/mm/hap/hap.c > +++ b/xen/arch/x86/mm/hap/hap.c > @@ -325,7 +325,7 @@ static void hap_free_p2m_page(struct domain *d, struct page_info *pg) > static unsigned int > hap_get_allocation(struct domain *d) > { > - unsigned int pg = d->arch.paging.hap.total_pages > + unsigned long pg = d->arch.paging.hap.total_pages > + d->arch.paging.hap.p2m_pages; > > return ((pg >> (20 - PAGE_SHIFT)) > @@ -334,8 +334,8 @@ hap_get_allocation(struct domain *d) > > /* Set the pool of pages to the required number of pages. > * Returns 0 for success, non-zero for failure. */ > -static unsigned int > -hap_set_allocation(struct domain *d, unsigned int pages, int *preempted) > +static int > +hap_set_allocation(struct domain *d, unsigned long pages, int *preempted) > { > struct page_info *pg; > > diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c > index 21607bf..d3cc2cc 100644 > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -1613,9 +1613,8 @@ shadow_free_p2m_page(struct domain *d, struct page_info *pg) > * Input will be rounded up to at least shadow_min_acceptable_pages(), > * plus space for the p2m table. > * Returns 0 for success, non-zero for failure. */ > -static unsigned int sh_set_allocation(struct domain *d, > - unsigned int pages, > - int *preempted) > +static int sh_set_allocation(struct domain *d, unsigned long pages, > + int *preempted) > { > struct page_info *sp; > unsigned int lower_bound; > @@ -1692,7 +1691,7 @@ static unsigned int sh_set_allocation(struct domain *d, > /* Return the size of the shadow pool, rounded up to the nearest MB */ > static unsigned int shadow_get_allocation(struct domain *d) > { > - unsigned int pg = d->arch.paging.shadow.total_pages > + unsigned long pg = d->arch.paging.shadow.total_pages > + d->arch.paging.shadow.p2m_pages; > return ((pg >> (20 - PAGE_SHIFT)) > + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0)); > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h > index 5807a1f..11ac2a5 100644 > --- a/xen/include/asm-x86/domain.h > +++ b/xen/include/asm-x86/domain.h > @@ -93,9 +93,9 @@ struct shadow_domain { > > /* Memory allocation */ > struct page_list_head freelist; > - unsigned int total_pages; /* number of pages allocated */ > - unsigned int free_pages; /* number of pages on freelists */ > - unsigned int p2m_pages; /* number of pages allocates to p2m */ > + unsigned long total_pages; /* number of pages allocated */ > + unsigned long free_pages; /* number of pages on freelists */ > + unsigned long p2m_pages; /* number of pages allocates to p2m */ > > /* 1-to-1 map for use when HVM vcpus have paging disabled */ > pagetable_t unpaged_pagetable; > @@ -155,9 +155,9 @@ struct shadow_vcpu { > /************************************************/ > struct hap_domain { > struct page_list_head freelist; > - unsigned int total_pages; /* number of pages allocated */ > - unsigned int free_pages; /* number of pages on freelists */ > - unsigned int p2m_pages; /* number of pages allocates to p2m */ > + unsigned long total_pages; /* number of pages allocated */ > + unsigned long free_pages; /* number of pages on freelists */ > + unsigned long p2m_pages; /* number of pages allocates to p2m */ > }; > > /************************************************/ >
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Roger Pau Monne > Sent: 27 September 2016 16:57 > To: xen-devel@lists.xenproject.org > Cc: George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>; Jan Beulich > <jbeulich@suse.com>; boris.ostrovsky@oracle.com; Roger Pau Monne > <roger.pau@citrix.com> > Subject: [Xen-devel] [PATCH v2 03/30] xen/x86: fix parameters and return > value of *_set_allocation functions > > Return should be an int, and the number of pages should be an unsigned > long. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Tim Deegan <tim@xen.org> > --- > xen/arch/x86/mm/hap/hap.c | 6 +++--- > xen/arch/x86/mm/shadow/common.c | 7 +++---- > xen/include/asm-x86/domain.h | 12 ++++++------ > 3 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c > index 3218fa2..b6d2c61 100644 > --- a/xen/arch/x86/mm/hap/hap.c > +++ b/xen/arch/x86/mm/hap/hap.c > @@ -325,7 +325,7 @@ static void hap_free_p2m_page(struct domain *d, > struct page_info *pg) > static unsigned int > hap_get_allocation(struct domain *d) > { > - unsigned int pg = d->arch.paging.hap.total_pages > + unsigned long pg = d->arch.paging.hap.total_pages > + d->arch.paging.hap.p2m_pages; You are modifying this calculation (by including hap.p2m_pages as well as hap.total_pages) so the comment should probably mention this. > > return ((pg >> (20 - PAGE_SHIFT)) > @@ -334,8 +334,8 @@ hap_get_allocation(struct domain *d) > > /* Set the pool of pages to the required number of pages. > * Returns 0 for success, non-zero for failure. */ > -static unsigned int > -hap_set_allocation(struct domain *d, unsigned int pages, int *preempted) > +static int > +hap_set_allocation(struct domain *d, unsigned long pages, int > *preempted) > { > struct page_info *pg; > > diff --git a/xen/arch/x86/mm/shadow/common.c > b/xen/arch/x86/mm/shadow/common.c > index 21607bf..d3cc2cc 100644 > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -1613,9 +1613,8 @@ shadow_free_p2m_page(struct domain *d, struct > page_info *pg) > * Input will be rounded up to at least shadow_min_acceptable_pages(), > * plus space for the p2m table. > * Returns 0 for success, non-zero for failure. */ > -static unsigned int sh_set_allocation(struct domain *d, > - unsigned int pages, > - int *preempted) > +static int sh_set_allocation(struct domain *d, unsigned long pages, > + int *preempted) > { > struct page_info *sp; > unsigned int lower_bound; > @@ -1692,7 +1691,7 @@ static unsigned int sh_set_allocation(struct domain > *d, > /* Return the size of the shadow pool, rounded up to the nearest MB */ > static unsigned int shadow_get_allocation(struct domain *d) > { > - unsigned int pg = d->arch.paging.shadow.total_pages > + unsigned long pg = d->arch.paging.shadow.total_pages > + d->arch.paging.shadow.p2m_pages; Same here. > return ((pg >> (20 - PAGE_SHIFT)) > + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0)); OMG. Is there no rounding macro you can use for this? Paul > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h > index 5807a1f..11ac2a5 100644 > --- a/xen/include/asm-x86/domain.h > +++ b/xen/include/asm-x86/domain.h > @@ -93,9 +93,9 @@ struct shadow_domain { > > /* Memory allocation */ > struct page_list_head freelist; > - unsigned int total_pages; /* number of pages allocated */ > - unsigned int free_pages; /* number of pages on freelists */ > - unsigned int p2m_pages; /* number of pages allocates to p2m */ > + unsigned long total_pages; /* number of pages allocated */ > + unsigned long free_pages; /* number of pages on freelists */ > + unsigned long p2m_pages; /* number of pages allocates to p2m */ > > /* 1-to-1 map for use when HVM vcpus have paging disabled */ > pagetable_t unpaged_pagetable; > @@ -155,9 +155,9 @@ struct shadow_vcpu { > /************************************************/ > struct hap_domain { > struct page_list_head freelist; > - unsigned int total_pages; /* number of pages allocated */ > - unsigned int free_pages; /* number of pages on freelists */ > - unsigned int p2m_pages; /* number of pages allocates to p2m */ > + unsigned long total_pages; /* number of pages allocated */ > + unsigned long free_pages; /* number of pages on freelists */ > + unsigned long p2m_pages; /* number of pages allocates to p2m */ > }; > > /************************************************/ > -- > 2.7.4 (Apple Git-66) > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On Mon, Oct 03, 2016 at 09:05:43AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > > Roger Pau Monne > > Sent: 27 September 2016 16:57 > > To: xen-devel@lists.xenproject.org > > Cc: George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > > <Andrew.Cooper3@citrix.com>; Tim (Xen.org) <tim@xen.org>; Jan Beulich > > <jbeulich@suse.com>; boris.ostrovsky@oracle.com; Roger Pau Monne > > <roger.pau@citrix.com> > > Subject: [Xen-devel] [PATCH v2 03/30] xen/x86: fix parameters and return > > value of *_set_allocation functions > > > > Return should be an int, and the number of pages should be an unsigned > > long. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Tim Deegan <tim@xen.org> > > --- > > xen/arch/x86/mm/hap/hap.c | 6 +++--- > > xen/arch/x86/mm/shadow/common.c | 7 +++---- > > xen/include/asm-x86/domain.h | 12 ++++++------ > > 3 files changed, 12 insertions(+), 13 deletions(-) > > > > diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c > > index 3218fa2..b6d2c61 100644 > > --- a/xen/arch/x86/mm/hap/hap.c > > +++ b/xen/arch/x86/mm/hap/hap.c > > @@ -325,7 +325,7 @@ static void hap_free_p2m_page(struct domain *d, > > struct page_info *pg) > > static unsigned int > > hap_get_allocation(struct domain *d) > > { > > - unsigned int pg = d->arch.paging.hap.total_pages > > + unsigned long pg = d->arch.paging.hap.total_pages > > + d->arch.paging.hap.p2m_pages; > > You are modifying this calculation (by including hap.p2m_pages as well as hap.total_pages) so the comment should probably mention this. I think your mail reader is fooling you, the '+' was already there before my change (and in fact I haven't even touched that line). > > > > return ((pg >> (20 - PAGE_SHIFT)) > > @@ -334,8 +334,8 @@ hap_get_allocation(struct domain *d) > > > > /* Set the pool of pages to the required number of pages. > > * Returns 0 for success, non-zero for failure. */ > > -static unsigned int > > -hap_set_allocation(struct domain *d, unsigned int pages, int *preempted) > > +static int > > +hap_set_allocation(struct domain *d, unsigned long pages, int > > *preempted) > > { > > struct page_info *pg; > > > > diff --git a/xen/arch/x86/mm/shadow/common.c > > b/xen/arch/x86/mm/shadow/common.c > > index 21607bf..d3cc2cc 100644 > > --- a/xen/arch/x86/mm/shadow/common.c > > +++ b/xen/arch/x86/mm/shadow/common.c > > @@ -1613,9 +1613,8 @@ shadow_free_p2m_page(struct domain *d, struct > > page_info *pg) > > * Input will be rounded up to at least shadow_min_acceptable_pages(), > > * plus space for the p2m table. > > * Returns 0 for success, non-zero for failure. */ > > -static unsigned int sh_set_allocation(struct domain *d, > > - unsigned int pages, > > - int *preempted) > > +static int sh_set_allocation(struct domain *d, unsigned long pages, > > + int *preempted) > > { > > struct page_info *sp; > > unsigned int lower_bound; > > @@ -1692,7 +1691,7 @@ static unsigned int sh_set_allocation(struct domain > > *d, > > /* Return the size of the shadow pool, rounded up to the nearest MB */ > > static unsigned int shadow_get_allocation(struct domain *d) > > { > > - unsigned int pg = d->arch.paging.shadow.total_pages > > + unsigned long pg = d->arch.paging.shadow.total_pages > > + d->arch.paging.shadow.p2m_pages; > > Same here. > > > return ((pg >> (20 - PAGE_SHIFT)) > > + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0)); > > OMG. Is there no rounding macro you can use for this? Hm, I don't think there's any, and the code was already there (I haven't added this), so I will just leave it as-is. Thanks, Roger.
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c index 3218fa2..b6d2c61 100644 --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -325,7 +325,7 @@ static void hap_free_p2m_page(struct domain *d, struct page_info *pg) static unsigned int hap_get_allocation(struct domain *d) { - unsigned int pg = d->arch.paging.hap.total_pages + unsigned long pg = d->arch.paging.hap.total_pages + d->arch.paging.hap.p2m_pages; return ((pg >> (20 - PAGE_SHIFT)) @@ -334,8 +334,8 @@ hap_get_allocation(struct domain *d) /* Set the pool of pages to the required number of pages. * Returns 0 for success, non-zero for failure. */ -static unsigned int -hap_set_allocation(struct domain *d, unsigned int pages, int *preempted) +static int +hap_set_allocation(struct domain *d, unsigned long pages, int *preempted) { struct page_info *pg; diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index 21607bf..d3cc2cc 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -1613,9 +1613,8 @@ shadow_free_p2m_page(struct domain *d, struct page_info *pg) * Input will be rounded up to at least shadow_min_acceptable_pages(), * plus space for the p2m table. * Returns 0 for success, non-zero for failure. */ -static unsigned int sh_set_allocation(struct domain *d, - unsigned int pages, - int *preempted) +static int sh_set_allocation(struct domain *d, unsigned long pages, + int *preempted) { struct page_info *sp; unsigned int lower_bound; @@ -1692,7 +1691,7 @@ static unsigned int sh_set_allocation(struct domain *d, /* Return the size of the shadow pool, rounded up to the nearest MB */ static unsigned int shadow_get_allocation(struct domain *d) { - unsigned int pg = d->arch.paging.shadow.total_pages + unsigned long pg = d->arch.paging.shadow.total_pages + d->arch.paging.shadow.p2m_pages; return ((pg >> (20 - PAGE_SHIFT)) + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0)); diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 5807a1f..11ac2a5 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -93,9 +93,9 @@ struct shadow_domain { /* Memory allocation */ struct page_list_head freelist; - unsigned int total_pages; /* number of pages allocated */ - unsigned int free_pages; /* number of pages on freelists */ - unsigned int p2m_pages; /* number of pages allocates to p2m */ + unsigned long total_pages; /* number of pages allocated */ + unsigned long free_pages; /* number of pages on freelists */ + unsigned long p2m_pages; /* number of pages allocates to p2m */ /* 1-to-1 map for use when HVM vcpus have paging disabled */ pagetable_t unpaged_pagetable; @@ -155,9 +155,9 @@ struct shadow_vcpu { /************************************************/ struct hap_domain { struct page_list_head freelist; - unsigned int total_pages; /* number of pages allocated */ - unsigned int free_pages; /* number of pages on freelists */ - unsigned int p2m_pages; /* number of pages allocates to p2m */ + unsigned long total_pages; /* number of pages allocated */ + unsigned long free_pages; /* number of pages on freelists */ + unsigned long p2m_pages; /* number of pages allocates to p2m */ }; /************************************************/
Return should be an int, and the number of pages should be an unsigned long. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Tim Deegan <tim@xen.org> --- xen/arch/x86/mm/hap/hap.c | 6 +++--- xen/arch/x86/mm/shadow/common.c | 7 +++---- xen/include/asm-x86/domain.h | 12 ++++++------ 3 files changed, 12 insertions(+), 13 deletions(-)