Re: [Xen-devel] [PATCH v2 11/20] x86/mem_sharing: Convert MEM_SHARING_DESTROY_GFN to a bool

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Dec 18, 2019 at 2:29 PM Julien Grall <julien@xxxxxxx> 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@xxxxxxxxx>
> > ---
> >   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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel