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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

> +    }
>      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().

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

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

Jan


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