Re: [PATCH] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

> At 04:36 -0700 on 17 May (1337229382), Andres Lagar-Cavilla wrote:
>> I believe the fix we'll converge to is keep paging_in in the "hole" set
>> of
>> types, and remove the check for valid mfn. Something along the lines of
>> what I've pasted below.
>
> OK.  Please send it as a diff against what's already applied - I think
> what we have now is more correct (if less useful) than what we had
> before.

Done. Tested and pasted below. Hopefully you can apply it before
end-of-day. Works for all known users of sharing+paging ;)

>
> Do you have to worry about freeing the page as well?  Will it otherwise
> be leaked into a state where it's allocated but not in the p2m?  I see
> that guest_physmap_add_entry() doesn't free paging_in pages but maybe
> that's wrong too?

Exactly. Leaked out of the p2m. Will still be cleaned up properly on
domain destruction.

The patch below takes care of the leak for sharing_add_to_physmap. However
I am not touching guest_physmap_add_entry -- it's been that way for pretty
much forever. My observation is that guest_physmap_add_entry is mostly
called from hypercalls (grant, XENMEM), so the domain is leaking its own
memory and risking running against the max_pages limit sooner, if being
sloppy. The only exception is populate physmap, which maybe should be
looked into.

Thanks,
Andres

# HG changeset patch
# Parent 485cd11f131da88b286b3b64e8f095508b67ab0b
x86/mem_sharing: Re-rectify sharing add to physmap hole test.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 485cd11f131d xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1103,7 +1103,17 @@ int mem_sharing_add_to_physmap(struct do
         ret = 0;
         /* There is a chance we're plugging a hole where a paged out page
was */
         if ( p2m_is_paging(cmfn_type) && (cmfn_type != p2m_ram_paging_out) )
+        {
             atomic_dec(&cd->paged_pages);
+            /* Further, there is a chance this was a valid page. Don't
leak it. */
+            if ( mfn_valid(cmfn) )
+            {
+                struct page_info *cpage = mfn_to_page(cmfn);
+                ASSERT(cpage != NULL);
+                if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
+                    put_page(cpage);
+            }
+        }
     }

     atomic_inc(&nr_saved_mfns);
diff -r 485cd11f131d xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -137,6 +137,7 @@ typedef unsigned int p2m_query_t;
  * entry */
 #define P2M_HOLE_TYPES (p2m_to_mask(p2m_mmio_dm)        \
                        | p2m_to_mask(p2m_invalid)       \
+                       | p2m_to_mask(p2m_ram_paging_in) \
                        | p2m_to_mask(p2m_ram_paged))

 /* Grant mapping types, which map to a real machine frame in another

guest_p
>
> Cheers,
>
> Tim.
>
>> # HG changeset patch
>> # Parent 9fc0252536f0a4ddf48b7ec9cd7a7243271545f8
>> x86/mem_sharing: Rectify test for "empty" physmap entry in
>> sharing_add_to_physmap.
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r 9fc0252536f0 xen/arch/x86/mm/mem_sharing.c
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1073,9 +1073,10 @@ int mem_sharing_add_to_physmap(struct do
>>      if ( spage->sharing->handle != sh )
>>          goto err_unlock;
>>
>> -    /* Make sure the target page is a hole in the physmap */
>> -    if ( mfn_valid(cmfn) ||
>> -         (!(p2m_is_ram(cmfn_type))) )
>> +    /* Make sure the target page is a hole in the physmap. These are
>> typically
>> +     * p2m_mmio_dm, but also accept p2m_invalid and paged out pages.
>> See the
>> +     * definition of p2m_is_hole in p2m.h. */
>> +    if ( !p2m_is_hole(cmfn_type) )
>>      {
>>          ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
>>          goto err_unlock;
>> diff -r 9fc0252536f0 xen/include/asm-x86/p2m.h
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -133,6 +133,13 @@ typedef unsigned int p2m_query_t;
>>                         | p2m_to_mask(p2m_ram_paging_in)       \
>>                         | p2m_to_mask(p2m_ram_shared))
>>
>> +/* Types that represent a physmap hole that is ok to replace with a
>> shared
>> + * entry */
>> +#define P2M_HOLE_TYPES (p2m_to_mask(p2m_mmio_dm)        \
>> +                       | p2m_to_mask(p2m_invalid)       \
>> +                       | p2m_to_mask(p2m_ram_paging_in) \
>> +                       | p2m_to_mask(p2m_ram_paged))
>> +
>>  /* Grant mapping types, which map to a real machine frame in another
>>   * VM */
>>  #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw)  \
>> @@ -173,6 +180,7 @@ typedef unsigned int p2m_query_t;
>>
>>  /* Useful predicates */
>>  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
>> +#define p2m_is_hole(_t) (p2m_to_mask(_t) & P2M_HOLE_TYPES)
>>  #define p2m_is_mmio(_t) (p2m_to_mask(_t) & P2M_MMIO_TYPES)
>>  #define p2m_is_readonly(_t) (p2m_to_mask(_t) & P2M_RO_TYPES)
>>  #define p2m_is_magic(_t) (p2m_to_mask(_t) & P2M_MAGIC_TYPES)
>>
>> >
>> > Cheers,
>> >
>> > Tim.
>> >
>>
>>
>