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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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