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

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

> 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.
I was wary that accessing the pointer directly might be unsafe if the
domain is gone, and it was better to search the domain on a list when we
needed to.

Other stuff will fix
Andres

>
>
> 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