| Message ID | 20210715051819.3073628-8-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On 15.07.2021 07:18, Penny Zheng wrote: > In order to deal with the trouble of count-to-order conversion when page number > is not in a power-of-two, this commit re-define assign_pages for nr pages and > assign_page for original page with a single order. > > Backporting confusion could be helped by altering the order of assign_pages > parameters, such that the compiler would point out that adjustments at call > sites are needed. Back on the initial form of this patch Julien said: "However, I think I would prefer if we introduce a new interface (maybe assign_pages_nr()) rather than change the meaning of the field. This is for two reasons: 1) We limit the risk to make mistake when backporting a patch touch assign_pages(). 2) Adding (1UL << order) for pretty much all the caller is not nice." 1) is taken care of here anyway (albeit see the remark below), and of the callers only some would really need "1UL <<" added (others could simply convert their literal 0 to literal 1). Julien - do you still think 2) is pretty important to avoid at the, overall, 2 places that would be left? > --- a/xen/arch/x86/pv/dom0_build.c > +++ b/xen/arch/x86/pv/dom0_build.c > @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d, > else > { > while ( count-- ) > - if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) > + if ( assign_page(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) I think in all cases where order-0 pages get passed, you'd rather want to call assign_pages() directly (if, as per above, we'll keep both functions in the first place). > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2283,8 +2283,8 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) > > int assign_pages( > struct domain *d, > + unsigned long nr, > struct page_info *pg, > - unsigned int order, > unsigned int memflags) > { I'm afraid I consider putting "nr" ahead of "pg" unusual (considering the rest of our code base). How about int assign_pages( struct page_info *pg, unsigned long nr, struct domain *d, unsigned int memflags) { ? > @@ -2354,6 +2354,11 @@ int assign_pages( > return rc; > } > > +int assign_page(struct domain *d, struct page_info *pg, unsigned int order, > + unsigned int memflags) > +{ > + return assign_pages(d, (1UL << order), pg, memflags); May I ask that you omit the unnecessary parentheses? Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, July 19, 2021 4:41 PM > To: Penny Zheng <Penny.Zheng@arm.com>; julien@xen.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Subject: Re: [PATCH V3 07/10] xen: re-define assign_pages and introduce > assign_page > > On 15.07.2021 07:18, Penny Zheng wrote: > > In order to deal with the trouble of count-to-order conversion when > > page number is not in a power-of-two, this commit re-define > > assign_pages for nr pages and assign_page for original page with a single > order. > > > > Backporting confusion could be helped by altering the order of > > assign_pages parameters, such that the compiler would point out that > > adjustments at call sites are needed. > > Back on the initial form of this patch Julien said: > > "However, I think I would prefer if we introduce a new interface (maybe > assign_pages_nr()) rather than change the meaning of the field. This is for two > reasons: > 1) We limit the risk to make mistake when backporting a patch touch > assign_pages(). > 2) Adding (1UL << order) for pretty much all the caller is not nice." > > 1) is taken care of here anyway (albeit see the remark below), and of the > callers only some would really need "1UL <<" added (others could simply > convert their literal 0 to literal 1). Julien - do you still think 2) is pretty > important to avoid at the, overall, 2 places that would be left? > > > --- a/xen/arch/x86/pv/dom0_build.c > > +++ b/xen/arch/x86/pv/dom0_build.c > > @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d, > > else > > { > > while ( count-- ) > > - if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) > > + if ( assign_page(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) > > I think in all cases where order-0 pages get passed, you'd rather want to call > assign_pages() directly (if, as per above, we'll keep both functions in the first > place). > Sure. I'll use literal 1, it's more reasonable to me also. > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2283,8 +2283,8 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) > > > > int assign_pages( > > struct domain *d, > > + unsigned long nr, > > struct page_info *pg, > > - unsigned int order, > > unsigned int memflags) > > { > > I'm afraid I consider putting "nr" ahead of "pg" unusual (considering the rest of > our code base). How about > > int assign_pages( > struct page_info *pg, > unsigned long nr, > struct domain *d, > unsigned int memflags) > { > > ? > Sure. Thx for reconstructing. > > @@ -2354,6 +2354,11 @@ int assign_pages( > > return rc; > > } > > > > +int assign_page(struct domain *d, struct page_info *pg, unsigned int order, > > + unsigned int memflags) { > > + return assign_pages(d, (1UL << order), pg, memflags); > > May I ask that you omit the unnecessary parentheses? > Oh, sorry. I'll remove it, loopy head sometimes... > Jan Cheers Penny
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index af47615b22..476f8a2012 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d, else { while ( count-- ) - if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) + if ( assign_page(d, mfn_to_page(_mfn(mfn++)), 0, 0) ) BUG(); } initrd->mod_end = 0; diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index fab77ab9cc..10b23f7e09 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -2342,7 +2342,7 @@ gnttab_transfer( * is respected and speculative execution is blocked accordingly */ if ( unlikely(!evaluate_nospec(okay)) || - unlikely(assign_pages(e, page, 0, MEMF_no_refcount)) ) + unlikely(assign_page(e, page, 0, MEMF_no_refcount)) ) { bool drop_dom_ref; diff --git a/xen/common/memory.c b/xen/common/memory.c index e07bd9a5ea..8c7c9c8fe4 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -728,7 +728,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) /* Assign each output page to the domain. */ for ( j = 0; (page = page_list_remove_head(&out_chunk_list)); ++j ) { - if ( assign_pages(d, page, exch.out.extent_order, + if ( assign_page(d, page, exch.out.extent_order, MEMF_no_refcount) ) { unsigned long dec_count; @@ -797,7 +797,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) * cleared PGC_allocated. */ while ( (page = page_list_remove_head(&in_chunk_list)) ) - if ( assign_pages(d, page, 0, MEMF_no_refcount) ) + if ( assign_page(d, page, 0, MEMF_no_refcount) ) { BUG_ON(!d->is_dying); free_domheap_page(page); diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 15edaca227..3414873679 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2283,8 +2283,8 @@ void init_domheap_pages(paddr_t ps, paddr_t pe) int assign_pages( struct domain *d, + unsigned long nr, struct page_info *pg, - unsigned int order, unsigned int memflags) { int rc = 0; @@ -2304,7 +2304,7 @@ int assign_pages( { unsigned int extra_pages = 0; - for ( i = 0; i < (1ul << order); i++ ) + for ( i = 0; i < nr; i++ ) { ASSERT(!(pg[i].count_info & ~PGC_extra)); if ( pg[i].count_info & PGC_extra ) @@ -2313,18 +2313,18 @@ int assign_pages( ASSERT(!extra_pages || ((memflags & MEMF_no_refcount) && - extra_pages == 1u << order)); + extra_pages == nr)); } #endif if ( pg[0].count_info & PGC_extra ) { - d->extra_pages += 1u << order; + d->extra_pages += nr; memflags &= ~MEMF_no_refcount; } else if ( !(memflags & MEMF_no_refcount) ) { - unsigned int tot_pages = domain_tot_pages(d) + (1 << order); + unsigned int tot_pages = domain_tot_pages(d) + nr; if ( unlikely(tot_pages > d->max_pages) ) { @@ -2336,10 +2336,10 @@ int assign_pages( } if ( !(memflags & MEMF_no_refcount) && - unlikely(domain_adjust_tot_pages(d, 1 << order) == (1 << order)) ) + unlikely(domain_adjust_tot_pages(d, nr) == nr) ) get_knownalive_domain(d); - for ( i = 0; i < (1 << order); i++ ) + for ( i = 0; i < nr; i++ ) { ASSERT(page_get_owner(&pg[i]) == NULL); page_set_owner(&pg[i], d); @@ -2354,6 +2354,11 @@ int assign_pages( return rc; } +int assign_page(struct domain *d, struct page_info *pg, unsigned int order, + unsigned int memflags) +{ + return assign_pages(d, (1UL << order), pg, memflags); +} struct page_info *alloc_domheap_pages( struct domain *d, unsigned int order, unsigned int memflags) @@ -2396,7 +2401,7 @@ struct page_info *alloc_domheap_pages( pg[i].count_info = PGC_extra; } } - if ( assign_pages(d, pg, order, memflags) ) + if ( assign_page(d, pg, order, memflags) ) { free_heap_pages(pg, order, memflags & MEMF_no_scrub); return NULL; diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 8e8fb5a615..65ba1587ad 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -132,6 +132,12 @@ int query_page_offline(mfn_t mfn, uint32_t *status); void heap_init_late(void); int assign_pages( + struct domain *d, + unsigned long nr, + struct page_info *pg, + unsigned int memflags); + +int assign_page( struct domain *d, struct page_info *pg, unsigned int order,
In order to deal with the trouble of count-to-order conversion when page number is not in a power-of-two, this commit re-define assign_pages for nr pages and assign_page for original page with a single order. Backporting confusion could be helped by altering the order of assign_pages parameters, such that the compiler would point out that adjustments at call sites are needed. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v3 change: - rename assign_pages_nr to assign_pages - alter the order of assign_pages parameters --- xen/arch/x86/pv/dom0_build.c | 2 +- xen/common/grant_table.c | 2 +- xen/common/memory.c | 4 ++-- xen/common/page_alloc.c | 21 +++++++++++++-------- xen/include/xen/mm.h | 6 ++++++ 5 files changed, 23 insertions(+), 12 deletions(-)