Re: [Xen-devel] [PATCH 01 of 12] x86/mm: Eliminate hash table in sharing code as index of shared mfns

[Tim Deegan <tim@xxxxxxx>]

Hi, 

This seems basically fine; it's a logical progression from the interface
changes.  A few comments inline below.

One other thing I noticed, which is independent of this patch, really:
I'm not sure that having a domain ID in the gfn_info is the right
thing to do.  It seems like we could just carry a domain pointer.  


At 21:56 -0500 on 15 Jan (1326664581), Andres Lagar-Cavilla wrote:
> The hashtable mechanism used by the sharing code is a bit odd.  It seems
> unnecessary and adds complexity. 

I think that's a bit harsh, since you removed its main use yourself! :)

> Instead, we replace this by storing a list
> head directly in the page info for the case when the page is shared.  This 
> does
> not add any extra space to the page_info and serves to remove significant
> complexity from sharing.
> 
> Signed-off-by: Adin Scannell <adin@xxxxxxxxxxx>
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> diff -r 302a58874eb1 -r 2a4112172e44 xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -3,6 +3,7 @@
>   *
>   * Memory sharing support.
>   *
> + * Copyright (c) 2011 GridCentric, Inc. (Adin Scannell & Andres 
> Lagar-Cavilla)
>   * Copyright (c) 2009 Citrix Systems, Inc. (Grzegorz Milos)
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -34,15 +35,30 @@
>  
>  #include "mm-locks.h"
>  
> -/* Auditing of memory sharing code? */
> -#define MEM_SHARING_AUDIT 0
> -
>  #if MEM_SHARING_AUDIT
> -static void mem_sharing_audit(void);
> +static int mem_sharing_audit(void);

Hmmm.  You haven't made any of the callers use this new return value. :(
Better to leave it as it was.

>  struct page_info
>  {
>      union {
> @@ -49,8 +51,13 @@ struct page_info
>          /* For non-pinnable single-page shadows, a higher entry that points
>           * at us. */
>          paddr_t up;
> -        /* For shared/sharable pages the sharing handle */
> -        uint64_t shr_handle; 
> +        /* For shared/sharable pages, we use a doubly-linked list
> +         * of all the {pfn,domain} pairs that map this page. We also include
> +         * an opaque handle, which is effectively a version, so that clients
> +         * of sharing share the version they expect to.
> +         * This list is allocated and freed when a page is shared/unshared.
> +         */
> +        struct page_sharing_info *shared_info;

Can you call this field 'sharing' instead?  'shared-info' makes me
think of the per-domain shared_info page, and tripped me up a few times
reading this patch. :)

Cheers,

Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel