Re: [Xen-devel] [PATCH v2 04/20] x86/mem_sharing: cleanup code and comments in various locations

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Dec 19, 2019 at 4:19 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 18/12/2019 19:40, Tamas K Lengyel wrote:
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 5a3a962fbb..1e888b403b 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1902,12 +1902,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> > long gla,
> >      if ( npfec.write_access && (p2mt == p2m_ram_shared) )
> >      {
> >          ASSERT(p2m_is_hostp2m(p2m));
> > -        sharing_enomem =
> > -            (mem_sharing_unshare_page(currd, gfn, 0) < 0);
> > +        sharing_enomem = mem_sharing_unshare_page(currd, gfn, 0);
>
> This is a logical change.  Is it intended to be in a later patch?
>
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index efb8821768..319aaf3074 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -198,24 +200,26 @@ static inline shr_handle_t get_next_handle(void)
> >  #define mem_sharing_enabled(d) \
> >      (is_hvm_domain(d) && (d)->arch.hvm.mem_sharing_enabled)
> >
> > -static atomic_t nr_saved_mfns   = ATOMIC_INIT(0);
> > +static atomic_t nr_saved_mfns   = ATOMIC_INIT(0);
> >  static atomic_t nr_shared_mfns  = ATOMIC_INIT(0);
> >
> > -/** Reverse map **/
> > -/* Every shared frame keeps a reverse map (rmap) of <domain, gfn> tuples 
> > that
> > +/*
> > + * Reverse map
> > + *
> > + * Every shared frame keeps a reverse map (rmap) of <domain, gfn> tuples 
> > that
> >   * this shared frame backs. For pages with a low degree of sharing, a O(n)
> >   * search linked list is good enough. For pages with higher degree of 
> > sharing,
> > - * we use a hash table instead. */
> > + * we use a hash table instead.
> > + */
> >
> >  typedef struct gfn_info
> >  {
> >      unsigned long gfn;
> > -    domid_t domain;
> > +    domid_t domain;
> >      struct list_head list;
> >  } gfn_info_t;
> >
> > -static inline void
> > -rmap_init(struct page_info *page)
> > +static inline void rmap_init(struct page_info *page)
>
> Seeing as you're folding this, the inline can be dropped.  In .c files,
> functions should just be static.
>
> > @@ -437,25 +441,29 @@ static inline void mem_sharing_gfn_destroy(struct 
> > page_info *page,
> >      xfree(gfn_info);
> >  }
> >
> > -static struct page_info* mem_sharing_lookup(unsigned long mfn)
> > +static inline struct page_info* mem_sharing_lookup(unsigned long mfn)
>
> Seeing as this is cleanup, the position of the * can move.  Similarly,
> it shouldn't gain an inline.
>
> I can fix all of this up on commit (and a few other brace position
> issues) if you want, to save reworking a trivial v2.
>

Provided nothing else is outstanding with the patch and it can be
committed I would certainly appreciate that.

Thanks,
Tamas

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