| Message ID | 20190715091756.39065-1-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated | expand |
On 15.07.2019 11:17, Paul Durrant wrote: > The _PGC_allocated flag is set on a page when it is assigned to a domain > along with an initial reference count of at least 1. To clear this > 'allocation' reference it is necessary to test-and-clear _PGC_allocated and > then only drop the reference if the test-and-clear succeeds. This is open- > coded in many places. It is also unsafe to test-and-clear _PGC_allocated > unless the caller holds an additional reference. > > This patch adds a helper function, put_page_alloc_ref(), to replace all the > open-coded test-and-clear/put_page occurrences and incorporates in that a > BUG_ON() an additional page reference not being held. This last sentence reads somewhat strange to me - are there words missing and/or mis-ordered? > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> With the commit message aspect clarified Acked-by: Jan Beulich <jbeulich@suse.com> Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 15 July 2019 10:24 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; VolodymyrBabchuk > <Volodymyr_Babchuk@epam.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Tamas K Lengyel <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu > <wl@xen.org> > Subject: Re: [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated > > On 15.07.2019 11:17, Paul Durrant wrote: > > The _PGC_allocated flag is set on a page when it is assigned to a domain > > along with an initial reference count of at least 1. To clear this > > 'allocation' reference it is necessary to test-and-clear _PGC_allocated and > > then only drop the reference if the test-and-clear succeeds. This is open- > > coded in many places. It is also unsafe to test-and-clear _PGC_allocated > > unless the caller holds an additional reference. > > > > This patch adds a helper function, put_page_alloc_ref(), to replace all the > > open-coded test-and-clear/put_page occurrences and incorporates in that a > > BUG_ON() an additional page reference not being held. > > This last sentence reads somewhat strange to me - are there words > missing and/or mis-ordered? Perhaps it reads better if 'BUG_ON()' is substituted with 'BUG() on'? I just wanted to express that there was a new check in the helper function that the necessary additional reference is held. > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > With the commit message aspect clarified I am happy for you to re-word it if you feel it is not clear. With the extra comment in the helper function in v2 then perhaps it is not really necessary to have any additional explanation in the commit comment anyway? > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks, Paul > > Jan
On 15.07.2019 11:50, Paul Durrant wrote: >> From: Jan Beulich <JBeulich@suse.com> >> Sent: 15 July 2019 10:24 >> >> On 15.07.2019 11:17, Paul Durrant wrote: >>> The _PGC_allocated flag is set on a page when it is assigned to a domain >>> along with an initial reference count of at least 1. To clear this >>> 'allocation' reference it is necessary to test-and-clear _PGC_allocated and >>> then only drop the reference if the test-and-clear succeeds. This is open- >>> coded in many places. It is also unsafe to test-and-clear _PGC_allocated >>> unless the caller holds an additional reference. >>> >>> This patch adds a helper function, put_page_alloc_ref(), to replace all the >>> open-coded test-and-clear/put_page occurrences and incorporates in that a >>> BUG_ON() an additional page reference not being held. >> >> This last sentence reads somewhat strange to me - are there words >> missing and/or mis-ordered? > > Perhaps it reads better if 'BUG_ON()' is substituted with 'BUG() on'? > I just wanted to express that there was a new check in the helper > function that the necessary additional reference is held. But then still more like "... incorporates in a BUG() on that an additional ..."? At which point it imo could as well be "... incorporates in a BUG_ON() that an additional ..." (i.e. just a word order change from your original sentence). (There's then perhaps also an "is" missing later in the sentence.) >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >> >> With the commit message aspect clarified > > I am happy for you to re-word it if you feel it is not clear. Well, the problem is that I don't feel well adjusting what a native English speaking person has written. Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 15 July 2019 11:44 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: JulienGrall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne > <roger.pau@citrix.com>; VolodymyrBabchuk <Volodymyr_Babchuk@epam.com>; Stefano Stabellini > <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Tamas K Lengyel <tamas@tklengyel.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu > <wl@xen.org> > Subject: Re: [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated > > On 15.07.2019 11:50, Paul Durrant wrote: > >> From: Jan Beulich <JBeulich@suse.com> > >> Sent: 15 July 2019 10:24 > >> > >> On 15.07.2019 11:17, Paul Durrant wrote: > >>> The _PGC_allocated flag is set on a page when it is assigned to a domain > >>> along with an initial reference count of at least 1. To clear this > >>> 'allocation' reference it is necessary to test-and-clear _PGC_allocated and > >>> then only drop the reference if the test-and-clear succeeds. This is open- > >>> coded in many places. It is also unsafe to test-and-clear _PGC_allocated > >>> unless the caller holds an additional reference. > >>> > >>> This patch adds a helper function, put_page_alloc_ref(), to replace all the > >>> open-coded test-and-clear/put_page occurrences and incorporates in that a > >>> BUG_ON() an additional page reference not being held. > >> > >> This last sentence reads somewhat strange to me - are there words > >> missing and/or mis-ordered? > > > > Perhaps it reads better if 'BUG_ON()' is substituted with 'BUG() on'? > > I just wanted to express that there was a new check in the helper > > function that the necessary additional reference is held. > > But then still more like "... incorporates in a BUG() on that an > additional ..."? At which point it imo could as well be "... > incorporates in a BUG_ON() that an additional ..." (i.e. just a > word order change from your original sentence). (There's then > perhaps also an "is" missing later in the sentence.) > > >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > >> > >> With the commit message aspect clarified > > > > I am happy for you to re-word it if you feel it is not clear. > > Well, the problem is that I don't feel well adjusting what a native > English speaking person has written. Ok. How about... "This patch adds a helper function, put_page_alloc_ref(), to replace all the open-coded test-and-clear/put_page occurrences. That helper function incorporates a check that an additional page reference is held and will BUG() if it is not." ? Paul > > Jan
On 15.07.2019 13:09, Paul Durrant wrote: >> From: Jan Beulich <JBeulich@suse.com> >> Sent: 15 July 2019 11:44 >> >> Well, the problem is that I don't feel well adjusting what a native >> English speaking person has written. > > Ok. How about... > > "This patch adds a helper function, put_page_alloc_ref(), to replace > all the open-coded test-and-clear/put_page occurrences. That helper > function incorporates a check that an additional page reference is > held and will BUG() if it is not." Sounds good. I'll record this for merging in if no other need for a v3 arises. Jan
Hi, On 15/07/2019 10:17, Paul Durrant wrote: > The _PGC_allocated flag is set on a page when it is assigned to a domain > along with an initial reference count of at least 1. To clear this > 'allocation' reference it is necessary to test-and-clear _PGC_allocated and > then only drop the reference if the test-and-clear succeeds. This is open- > coded in many places. It is also unsafe to test-and-clear _PGC_allocated > unless the caller holds an additional reference. > > This patch adds a helper function, put_page_alloc_ref(), to replace all the > open-coded test-and-clear/put_page occurrences and incorporates in that a > BUG_ON() an additional page reference not being held. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> For the Arm bits: Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 4f44d5c742..941bbff4fe 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -926,9 +926,7 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list) */ continue; - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); - + put_page_alloc_ref(page); put_page(page); if ( hypercall_preempt_check() ) diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 147f96a09e..e791d86892 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1939,8 +1939,7 @@ static int relinquish_memory( BUG(); } - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + put_page_alloc_ref(page); /* * Forcibly invalidate top-most, still valid page tables at this point diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 7a80cfb28b..a79cabb680 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -398,8 +398,7 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) return 0; fail: - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + put_page_alloc_ref(page); put_page_and_type(page); return -ENOMEM; @@ -418,13 +417,7 @@ static void hvm_free_ioreq_mfn(struct hvm_ioreq_server *s, bool buf) unmap_domain_page_global(iorp->va); iorp->va = NULL; - /* - * Check whether we need to clear the allocation reference before - * dropping the explicit references taken by get_page_and_type(). - */ - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); - + put_page_alloc_ref(page); put_page_and_type(page); } diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index df2c0130f1..138662e777 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -498,8 +498,7 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d, void free_shared_domheap_page(struct page_info *page) { - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + put_page_alloc_ref(page); if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) ) ASSERT_UNREACHABLE(); page->u.inuse.type_info = 0; diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index f16a3f5324..58d9157fa8 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1000,8 +1000,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh, mem_sharing_page_unlock(firstpg); /* Free the client page */ - if(test_and_clear_bit(_PGC_allocated, &cpage->count_info)) - put_page(cpage); + put_page_alloc_ref(cpage); put_page(cpage); /* We managed to free a domain page. */ @@ -1082,8 +1081,7 @@ int mem_sharing_add_to_physmap(struct domain *sd, unsigned long sgfn, shr_handle ret = -EOVERFLOW; goto err_unlock; } - if ( test_and_clear_bit(_PGC_allocated, &cpage->count_info) ) - put_page(cpage); + put_page_alloc_ref(cpage); put_page(cpage); } } @@ -1177,8 +1175,7 @@ int __mem_sharing_unshare_page(struct domain *d, domain_crash(d); return -EOVERFLOW; } - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + put_page_alloc_ref(page); put_page(page); } put_gfn(d, gfn); diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c index 4313863066..096e2773fb 100644 --- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -274,9 +274,7 @@ p2m_pod_set_cache_target(struct p2m_domain *p2m, unsigned long pod_target, int p if ( test_and_clear_bit(_PGT_pinned, &(page+i)->u.inuse.type_info) ) put_page_and_type(page + i); - if ( test_and_clear_bit(_PGC_allocated, &(page+i)->count_info) ) - put_page(page + i); - + put_page_alloc_ref(page + i); put_page(page + i); if ( preemptible && pod_target != p2m->pod.count && diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 4c9954867c..883352da7d 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1609,8 +1609,7 @@ int p2m_mem_paging_evict(struct domain *d, unsigned long gfn_l) goto out_put; /* Decrement guest domain's ref count of the page */ - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + put_page_alloc_ref(page); /* Remove mapping from p2m table */ ret = p2m_set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_4K, diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index e6a0f30a4b..f0ca10a7fa 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1707,8 +1707,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) } BUG_ON(page_get_owner(pg) != d); - if ( test_and_clear_bit(_PGC_allocated, &pg->count_info) ) - put_page(pg); + put_page_alloc_ref(pg); if ( pg->count_info & ~PGC_xen_heap ) { diff --git a/xen/common/memory.c b/xen/common/memory.c index 03db7bfa9e..30d210fc08 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -388,9 +388,8 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) * For this purpose (and to match populate_physmap() behavior), the page * is kept allocated. */ - if ( !rc && !is_domain_direct_mapped(d) && - test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + if ( !rc && !is_domain_direct_mapped(d) ) + put_page_alloc_ref(page); put_page(page); diff --git a/xen/common/xenoprof.c b/xen/common/xenoprof.c index 8a72e382e6..4f3e799ebb 100644 --- a/xen/common/xenoprof.c +++ b/xen/common/xenoprof.c @@ -173,8 +173,7 @@ unshare_xenoprof_page_with_guest(struct xenoprof *x) struct page_info *page = mfn_to_page(mfn_add(mfn, i)); BUG_ON(page_get_owner(page) != current->domain); - if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) - put_page(page); + put_page_alloc_ref(page); } } diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index a57974ae51..5d0c19f011 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -658,4 +658,18 @@ static inline void share_xen_page_with_privileged_guests( share_xen_page_with_guest(page, dom_xen, flags); } +static inline void put_page_alloc_ref(struct page_info *page) +{ + /* + * Whenever a page is assigned to a domain then the _PGC_allocated bit + * is set and the reference count is set to at least 1. This function + * clears that 'allocation reference' but it is unsafe to do so without + * the caller holding an additional reference. I.e. the allocation + * reference must never be the last reference held. + */ + BUG_ON((page->count_info & PGC_count_mask) <= 1); + if ( test_and_clear_bit(_PGC_allocated, &page->count_info) ) + put_page(page); +} + #endif /* __XEN_MM_H__ */
The _PGC_allocated flag is set on a page when it is assigned to a domain along with an initial reference count of at least 1. To clear this 'allocation' reference it is necessary to test-and-clear _PGC_allocated and then only drop the reference if the test-and-clear succeeds. This is open- coded in many places. It is also unsafe to test-and-clear _PGC_allocated unless the caller holds an additional reference. This patch adds a helper function, put_page_alloc_ref(), to replace all the open-coded test-and-clear/put_page occurrences and incorporates in that a BUG_ON() an additional page reference not being held. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wl@xen.org> Cc: "Roger Pau Monné" <roger.pau@citrix.com> Cc: Tamas K Lengyel <tamas@tklengyel.com> Cc: George Dunlap <george.dunlap@eu.citrix.com> v2: - Re-name clear_assignment_reference() to put_page_alloc_ref() - Swap ASSERT() for BUG_ON() - Add an extra comment explaining what put_page_alloc_ref() is doing --- xen/arch/arm/domain.c | 4 +--- xen/arch/x86/domain.c | 3 +-- xen/arch/x86/hvm/ioreq.c | 11 ++--------- xen/arch/x86/mm.c | 3 +-- xen/arch/x86/mm/mem_sharing.c | 9 +++------ xen/arch/x86/mm/p2m-pod.c | 4 +--- xen/arch/x86/mm/p2m.c | 3 +-- xen/common/grant_table.c | 3 +-- xen/common/memory.c | 5 ++--- xen/common/xenoprof.c | 3 +-- xen/include/xen/mm.h | 14 ++++++++++++++ 11 files changed, 28 insertions(+), 34 deletions(-)