Re: [Xen-devel] [PATCH 1/2] tmem: add full support for x86 up to 16Tb

[Bob Liu <bob.liu@xxxxxxxxxx>]

Hi Jan,

Thanks for you review.

On 09/23/2013 06:46 PM, Jan Beulich wrote:
>>>> On 23.09.13 at 04:23, Bob Liu <lliubbo@xxxxxxxxx> wrote:
>> tmem used to have code assuming to be able to directly access all memory.
>> This patch try to fix this problem fully so that tmem can work on x86 up to
>> 16Tb.
>>
>> tmem allocates pages mainly for two purposes.
>> 1. store pages passed from guests through the frontswap/cleancache frontend.
>> In this case tmem code has already using map_domain_page() before
>> accessing the memory, no need to change for 16Tb supporting.
> 
> Did you also verify that none of these mappings remain in place
> for undue periods of time?
>

Yes, I think so.

>> 2. store tmem meta data.
>> In this case, there is a problem if we use map_domain_page(). That's the 
>> mapping
>> entrys are limited, in the 2 VCPU guest we only have 32 entries and tmem 
>> can't
>> call unmap in a short time.
>> The fixing is allocate xen heap pages instead of domain heap for tmem meta
>> data.
> 
> Please quantify the amount of Xen heap memory potentially
> getting buried here.
> 

Okay, I'll try to count xen heap pages used here and provide some
statistic result.

> Please also clarify whether reclaiming this memory in fact works
> (i.e. that upon memory pressure the rest of the system can
> be kept alive by tmem giving up this memory in due course).
> 

This memory here can't be reclaimed directly because it's used for meta
data i.e. radix tree node.
But if the tmem data page gets reclaimed during memory pressure, the
corresponding radix tree node will also be freed.
After enough radix tree nodes get freed meta page used here will be
reclaimed.

>> Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx>
> 
> The patch is full of coding style violations; in fact there are too
> many of them to list them individually.
> 

Sorry for that, it's my first xen patch.
Is there any document or checkpatch script so that I can get rid of the
coding style problem?

>> @@ -166,38 +167,48 @@ static inline struct page_info 
>> *_tmh_alloc_page_thispool(struct domain *d)
>>      if ( d->tot_pages >= d->max_pages )
>>          return NULL;
>>  
>> -    if ( tmh_page_list_pages )
>> -    {
>> -        if ( (pi = tmh_page_list_get()) != NULL )
>> -        {
>> -            if ( donate_page(d,pi,0) == 0 )
>> -                goto out;
>> -            else
>> -                tmh_page_list_put(pi);
>> -        }
>> +    if (xen_heap)
>> +        pi = alloc_xenheap_pages(0,MEMF_tmem);
>> +    else {
>> +        if ( tmh_page_list_pages )
>> +        {
>> +                if ( (pi = tmh_page_list_get()) != NULL )
>> +                {
>> +                        if ( donate_page(d,pi,0) == 0 )
>> +                                goto out;
>> +                        else
>> +                                tmh_page_list_put(pi);
>> +                }
>> +        }
>> +
>> +        pi = alloc_domheap_pages(d,0,MEMF_tmem);
>>      }
>>  [...]
>>  static inline void _tmh_free_page_thispool(struct page_info *pi)
>>  {
>>      struct domain *d = page_get_owner(pi);
>> +    int xen_heap = is_xen_heap_page(pi);
>>  
>>      ASSERT(IS_VALID_PAGE(pi));
>> -    if ( (d == NULL) || steal_page(d,pi,0) == 0 )
>> -        tmh_page_list_put(pi);
>> +    if ( (d == NULL) || steal_page(d,pi,0) == 0 ) {
>> +        if (!xen_heap)
>> +                tmh_page_list_put(pi);
> 
> Looking at the allocation path above the opposite ought to be
> impossible. Hence rather than if() you want to ASSERT() here.
> 

Thanks, will be fixed.

>> +    }
>>      else
>>      {
>>          scrub_one_page(pi);
>>          ASSERT((pi->count_info & ~(PGC_allocated | 1)) == 0);
>> -        free_domheap_pages(pi,0);
>> +    if (xen_heap)
>> +            free_xenheap_pages(pi,0);
> 
> free_xenheap_page().
> 

Same with those place.

>> +    else
>> +            free_domheap_pages(pi,0);
> 
> free_domheap_page()
> 
>>  static inline void tmh_free_page(struct page_info *pi)
>>  {
>> +    int xen_heap = is_xen_heap_page(pi);
>>      ASSERT(IS_VALID_PAGE(pi));
>> -    tmh_page_list_put(pi);
>> -    atomic_dec(&freeable_page_count);
>> +    if (xen_heap){
>> +        scrub_one_page(pi);
>> +        ASSERT((pi->count_info & ~(PGC_allocated | 1)) == 0);
> 
> Did you perhaps copy this from somewhere else without
> understanding why it is needed there? It certainly doesn't look
> necessary here: Such checks are meaningful only when you
> allow a guest access to a Xen heap page.
> 

Sorry, I just copyed this code from _tmh_free_page_thispool().
Sounds like these checks are also unneeded in _tmh_free_page_thispool()?

>> +        free_xenheap_pages(pi,0);
> 
> free_xenheap_page().
> 
>> +    } else {
>> +        tmh_page_list_put(pi);
>> +        atomic_dec(&freeable_page_count);
>> +    }
>>  }
> 
> All that said - I'm not sure we want to accept enhancements to
> tmem without the security audit happening first, which has been
> due for over a year now.
> 

Okay, I'll take a look at it.
Anyway I'll try to make a V2 of this patchset based on your feedbacks.

-- 
Regards,
-Bob

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel