| Message ID | 88408b9bf706a28d8879edef61606f39a9df68b2.1576697796.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
Hi Tamas, On 18/12/2019 19:40, Tamas K Lengyel wrote: > MEM_SHARING_DESTROY_GFN is used on the 'flags' bitfield during unsharing. > However, the bitfield is not used for anything else, so just convert it to a > bool instead. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > --- > xen/arch/x86/mm/mem_sharing.c | 7 +++---- > xen/arch/x86/mm/p2m.c | 1 + > xen/common/memory.c | 2 +- > xen/include/asm-x86/mem_sharing.h | 5 ++--- > 4 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c > index fc1d8be1eb..6e81e1a895 100644 > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -1175,7 +1175,7 @@ err_out: > */ > int __mem_sharing_unshare_page(struct domain *d, > unsigned long gfn, > - uint16_t flags) > + bool destroy) > { > p2m_type_t p2mt; > mfn_t mfn; > @@ -1231,7 +1231,7 @@ int __mem_sharing_unshare_page(struct domain *d, > * If the GFN is getting destroyed drop the references to MFN > * (possibly freeing the page), and exit early. > */ > - if ( flags & MEM_SHARING_DESTROY_GFN ) > + if ( destroy ) > { > if ( !last_gfn ) > mem_sharing_gfn_destroy(page, d, gfn_info); > @@ -1321,8 +1321,7 @@ int relinquish_shared_pages(struct domain *d) > if ( mfn_valid(mfn) && p2m_is_shared(t) ) > { > /* Does not fail with ENOMEM given the DESTROY flag */ > - BUG_ON(__mem_sharing_unshare_page(d, gfn, > - MEM_SHARING_DESTROY_GFN)); > + BUG_ON(__mem_sharing_unshare_page(d, gfn, true)); > /* > * Clear out the p2m entry so no one else may try to > * unshare. Must succeed: we just read the old entry and > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index baea632acc..53ea44fe3c 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -517,6 +517,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l, > */ > if ( mem_sharing_unshare_page(p2m->domain, gfn_l) < 0 ) > mem_sharing_notify_enomem(p2m->domain, gfn_l, false); > + This line looks spurious. > mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL); > } > > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 309e872edf..c7d2bac452 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -352,7 +352,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) > * might be the only one using this shared page, and we need to > * trigger proper cleanup. Once done, this is like any other page. > */ > - rc = mem_sharing_unshare_page(d, gmfn, 0); > + rc = mem_sharing_unshare_page(d, gmfn); AFAICT, this patch does not reduce the number of parameters for mem_sharing_unshare_page(). Did you intend to make this change in another patch? > if ( rc ) > { > mem_sharing_notify_enomem(d, gmfn, false); > diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h > index 89cdaccea0..4b982a4803 100644 > --- a/xen/include/asm-x86/mem_sharing.h > +++ b/xen/include/asm-x86/mem_sharing.h > @@ -76,17 +76,16 @@ struct page_sharing_info > unsigned int mem_sharing_get_nr_saved_mfns(void); > unsigned int mem_sharing_get_nr_shared_mfns(void); > > -#define MEM_SHARING_DESTROY_GFN (1<<1) > /* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */ > int __mem_sharing_unshare_page(struct domain *d, > unsigned long gfn, > - uint16_t flags); > + bool destroy); > > static inline > int mem_sharing_unshare_page(struct domain *d, > unsigned long gfn) > { > - int rc = __mem_sharing_unshare_page(d, gfn, 0); > + int rc = __mem_sharing_unshare_page(d, gfn, false); > BUG_ON(rc && (rc != -ENOMEM)); > return rc; > } > Cheers,
On Wed, Dec 18, 2019 at 2:29 PM Julien Grall <julien@xen.org> wrote: > > Hi Tamas, > > On 18/12/2019 19:40, Tamas K Lengyel wrote: > > MEM_SHARING_DESTROY_GFN is used on the 'flags' bitfield during unsharing. > > However, the bitfield is not used for anything else, so just convert it to a > > bool instead. > > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > --- > > xen/arch/x86/mm/mem_sharing.c | 7 +++---- > > xen/arch/x86/mm/p2m.c | 1 + > > xen/common/memory.c | 2 +- > > xen/include/asm-x86/mem_sharing.h | 5 ++--- > > 4 files changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c > > index fc1d8be1eb..6e81e1a895 100644 > > --- a/xen/arch/x86/mm/mem_sharing.c > > +++ b/xen/arch/x86/mm/mem_sharing.c > > @@ -1175,7 +1175,7 @@ err_out: > > */ > > int __mem_sharing_unshare_page(struct domain *d, > > unsigned long gfn, > > - uint16_t flags) > > + bool destroy) > > { > > p2m_type_t p2mt; > > mfn_t mfn; > > @@ -1231,7 +1231,7 @@ int __mem_sharing_unshare_page(struct domain *d, > > * If the GFN is getting destroyed drop the references to MFN > > * (possibly freeing the page), and exit early. > > */ > > - if ( flags & MEM_SHARING_DESTROY_GFN ) > > + if ( destroy ) > > { > > if ( !last_gfn ) > > mem_sharing_gfn_destroy(page, d, gfn_info); > > @@ -1321,8 +1321,7 @@ int relinquish_shared_pages(struct domain *d) > > if ( mfn_valid(mfn) && p2m_is_shared(t) ) > > { > > /* Does not fail with ENOMEM given the DESTROY flag */ > > - BUG_ON(__mem_sharing_unshare_page(d, gfn, > > - MEM_SHARING_DESTROY_GFN)); > > + BUG_ON(__mem_sharing_unshare_page(d, gfn, true)); > > /* > > * Clear out the p2m entry so no one else may try to > > * unshare. Must succeed: we just read the old entry and > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > > index baea632acc..53ea44fe3c 100644 > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -517,6 +517,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l, > > */ > > if ( mem_sharing_unshare_page(p2m->domain, gfn_l) < 0 ) > > mem_sharing_notify_enomem(p2m->domain, gfn_l, false); > > + > > This line looks spurious. Yeap. > > > mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL); > > } > > > > diff --git a/xen/common/memory.c b/xen/common/memory.c > > index 309e872edf..c7d2bac452 100644 > > --- a/xen/common/memory.c > > +++ b/xen/common/memory.c > > @@ -352,7 +352,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) > > * might be the only one using this shared page, and we need to > > * trigger proper cleanup. Once done, this is like any other page. > > */ > > - rc = mem_sharing_unshare_page(d, gmfn, 0); > > + rc = mem_sharing_unshare_page(d, gmfn); > > AFAICT, this patch does not reduce the number of parameters for > mem_sharing_unshare_page(). Did you intend to make this change in > another patch? Ah yea, it should have been dropped in patch 6 of the series. > > > if ( rc ) > > { > > mem_sharing_notify_enomem(d, gmfn, false); > > diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h > > index 89cdaccea0..4b982a4803 100644 > > --- a/xen/include/asm-x86/mem_sharing.h > > +++ b/xen/include/asm-x86/mem_sharing.h > > @@ -76,17 +76,16 @@ struct page_sharing_info > > unsigned int mem_sharing_get_nr_saved_mfns(void); > > unsigned int mem_sharing_get_nr_shared_mfns(void); > > > > -#define MEM_SHARING_DESTROY_GFN (1<<1) > > /* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */ > > int __mem_sharing_unshare_page(struct domain *d, > > unsigned long gfn, > > - uint16_t flags); > > + bool destroy); > > > > static inline > > int mem_sharing_unshare_page(struct domain *d, > > unsigned long gfn) > > { > > - int rc = __mem_sharing_unshare_page(d, gfn, 0); > > + int rc = __mem_sharing_unshare_page(d, gfn, false); > > BUG_ON(rc && (rc != -ENOMEM)); > > return rc; > > } > > > > Cheers, Thanks, Tamas
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index fc1d8be1eb..6e81e1a895 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1175,7 +1175,7 @@ err_out: */ int __mem_sharing_unshare_page(struct domain *d, unsigned long gfn, - uint16_t flags) + bool destroy) { p2m_type_t p2mt; mfn_t mfn; @@ -1231,7 +1231,7 @@ int __mem_sharing_unshare_page(struct domain *d, * If the GFN is getting destroyed drop the references to MFN * (possibly freeing the page), and exit early. */ - if ( flags & MEM_SHARING_DESTROY_GFN ) + if ( destroy ) { if ( !last_gfn ) mem_sharing_gfn_destroy(page, d, gfn_info); @@ -1321,8 +1321,7 @@ int relinquish_shared_pages(struct domain *d) if ( mfn_valid(mfn) && p2m_is_shared(t) ) { /* Does not fail with ENOMEM given the DESTROY flag */ - BUG_ON(__mem_sharing_unshare_page(d, gfn, - MEM_SHARING_DESTROY_GFN)); + BUG_ON(__mem_sharing_unshare_page(d, gfn, true)); /* * Clear out the p2m entry so no one else may try to * unshare. Must succeed: we just read the old entry and diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index baea632acc..53ea44fe3c 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -517,6 +517,7 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l, */ if ( mem_sharing_unshare_page(p2m->domain, gfn_l) < 0 ) mem_sharing_notify_enomem(p2m->domain, gfn_l, false); + mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL); } diff --git a/xen/common/memory.c b/xen/common/memory.c index 309e872edf..c7d2bac452 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -352,7 +352,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) * might be the only one using this shared page, and we need to * trigger proper cleanup. Once done, this is like any other page. */ - rc = mem_sharing_unshare_page(d, gmfn, 0); + rc = mem_sharing_unshare_page(d, gmfn); if ( rc ) { mem_sharing_notify_enomem(d, gmfn, false); diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h index 89cdaccea0..4b982a4803 100644 --- a/xen/include/asm-x86/mem_sharing.h +++ b/xen/include/asm-x86/mem_sharing.h @@ -76,17 +76,16 @@ struct page_sharing_info unsigned int mem_sharing_get_nr_saved_mfns(void); unsigned int mem_sharing_get_nr_shared_mfns(void); -#define MEM_SHARING_DESTROY_GFN (1<<1) /* Only fails with -ENOMEM. Enforce it with a BUG_ON wrapper. */ int __mem_sharing_unshare_page(struct domain *d, unsigned long gfn, - uint16_t flags); + bool destroy); static inline int mem_sharing_unshare_page(struct domain *d, unsigned long gfn) { - int rc = __mem_sharing_unshare_page(d, gfn, 0); + int rc = __mem_sharing_unshare_page(d, gfn, false); BUG_ON(rc && (rc != -ENOMEM)); return rc; }
MEM_SHARING_DESTROY_GFN is used on the 'flags' bitfield during unsharing. However, the bitfield is not used for anything else, so just convert it to a bool instead. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/mm/mem_sharing.c | 7 +++---- xen/arch/x86/mm/p2m.c | 1 + xen/common/memory.c | 2 +- xen/include/asm-x86/mem_sharing.h | 5 ++--- 4 files changed, 7 insertions(+), 8 deletions(-)