Re: [Xen-devel] [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated
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 For the Arm bits: Acked-by: Julien Grall Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated
On 15.07.2019 13:09, Paul Durrant wrote: >> From: Jan Beulich >> 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated
> -Original Message- > From: Jan Beulich > Sent: 15 July 2019 11:44 > To: Paul Durrant > Cc: JulienGrall ; Andrew Cooper > ; George Dunlap > ; Ian Jackson ; Roger Pau > Monne > ; VolodymyrBabchuk ; > Stefano Stabellini > ; xen-devel@lists.xenproject.org; Konrad Rzeszutek > Wilk > ; Tamas K Lengyel ; Tim > (Xen.org) ; Wei Liu > > 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 > >> 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 > >> > >> 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [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 >> 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 >> >> 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated
> -Original Message- > From: Jan Beulich > Sent: 15 July 2019 10:24 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org; Julien Grall ; > Andrew Cooper > ; Roger Pau Monne ; > VolodymyrBabchuk > ; George Dunlap ; Ian > Jackson > ; Stefano Stabellini ; Konrad > Rzeszutek Wilk > ; Tamas K Lengyel ; Tim > (Xen.org) ; Wei Liu > > 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 > > 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 Thanks, Paul > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [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? > Signed-off-by: Paul Durrant With the commit message aspect clarified Acked-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated
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
---
Cc: Stefano Stabellini
Cc: Julien Grall
Cc: Volodymyr Babchuk
Cc: Andrew Cooper
Cc: George Dunlap
Cc: Ian Jackson
Cc: Jan Beulich
Cc: Konrad Rzeszutek Wilk
Cc: Tim Deegan
Cc: Wei Liu
Cc: "Roger Pau Monné"
Cc: Tamas K Lengyel
Cc: George Dunlap
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(-)
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);
}
}
@@ -1
