Re: [PATCH 13/16] tmem: cleanup: __tmem_alloc_page: drop unneed parameters

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Nov 20, 2013 at 04:46:22PM +0800, Bob Liu wrote:
> The two parameters of __tmem_alloc_page() can be reduced.

That is dangerous. The last one (no_heap) was added to guard
against recursion. Check how tmem_alloc_page_thispool calls
alloc_domheap_pages, whcih calls alloc_heap_pages which
can call tmem_relinquish_pages, which can .. well, you will
get the idea.

> tmem_called_from_tmem() was also dropped by this patch.
> 
> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
>  xen/common/tmem.c          |   13 +++++--------
>  xen/include/xen/tmem_xen.h |   11 +++++------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index ee61899..bf0fa1b 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -245,7 +245,7 @@ static struct page_info *tmem_alloc_page(struct tmem_pool *pool)
>      if ( pool != NULL && is_persistent(pool) )
>          pfp = __tmem_alloc_page_thispool(pool->client->domain);
>      else
> -        pfp = __tmem_alloc_page(pool,0);
> +        pfp = __tmem_alloc_page();
>      return pfp;
>  }
>  
> @@ -263,9 +263,8 @@ static noinline void *tmem_mempool_page_get(unsigned long size)
>      struct page_info *pi;
>  
>      ASSERT(size == PAGE_SIZE);
> -    if ( (pi = __tmem_alloc_page(NULL,0)) == NULL )
> +    if ( (pi = __tmem_alloc_page()) == NULL )
>          return NULL;
> -    ASSERT(IS_VALID_PAGE(pi));
>      return page_to_virt(pi);
>  }
>  
> @@ -2450,7 +2449,6 @@ void tmem_freeze_all(unsigned char key)
>  void *tmem_relinquish_pages(unsigned int order, unsigned int memflags)
>  {
>      struct page_info *pfp;
> -    unsigned long evicts_per_relinq = 0;
>      int max_evictions = 10;
>  
>      if (!tmem_enabled() || !tmem_freeable_pages())
> @@ -2464,14 +2462,13 @@ void *tmem_relinquish_pages(unsigned int order, unsigned int memflags)
>          return NULL;
>      }
>  
> -    if ( tmem_called_from_tmem(memflags) )
> +    if ( memflags & MEMF_tmem )
>          read_lock(&tmem_rwlock);
>  
> -    while ( (pfp = __tmem_alloc_page(NULL,1)) == NULL )
> +    while ( (pfp = tmem_page_list_get()) == NULL )

No no. That could lead to bad recursion. You need to pass '1' somehow.


>      {
>          if ( (max_evictions-- <= 0) || !tmem_evict())
>              break;
> -        evicts_per_relinq++;


? Should this be a seperate patch?

>      }
>      if ( pfp != NULL )
>      {
> @@ -2479,7 +2476,7 @@ void *tmem_relinquish_pages(unsigned int order, unsigned int memflags)
>              scrub_one_page(pfp);
>      }
>  
> -    if ( tmem_called_from_tmem(memflags) )
> +    if ( memflags & MEMF_tmem )

Perhaps a comment too? /* Called recursively from within tmem. */

>          read_unlock(&tmem_rwlock);
>  
>      return pfp;
> diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h
> index 7468c28..c4b9f5a 100644
> --- a/xen/include/xen/tmem_xen.h
> +++ b/xen/include/xen/tmem_xen.h
> @@ -144,15 +144,16 @@ static inline void __tmem_free_page_thispool(struct page_info *pi)
>  /*
>   * Memory allocation for ephemeral (non-persistent) data
>   */
> -static inline struct page_info *__tmem_alloc_page(void *pool, int no_heap)
> +static inline struct page_info *__tmem_alloc_page(void)
>  {
>      struct page_info *pi = tmem_page_list_get();
>  
> -    if ( pi == NULL && !no_heap )
> +    if ( pi == NULL)
>          pi = alloc_domheap_pages(0,0,MEMF_tmem);
> -    ASSERT((pi == NULL) || IS_VALID_PAGE(pi));
> -    if ( pi != NULL && !no_heap )
> +
> +    if ( pi )
>          atomic_inc(&freeable_page_count);
> +    ASSERT((pi == NULL) || IS_VALID_PAGE(pi));
>      return pi;
>  }
>  
> @@ -168,8 +169,6 @@ static inline unsigned long tmem_free_mb(void)
>      return (tmem_page_list_pages + total_free_pages()) >> (20 - PAGE_SHIFT);
>  }
>  
> -#define tmem_called_from_tmem(_memflags) (_memflags & MEMF_tmem)
> -
>  /*  "Client" (==domain) abstraction */
>  
>  struct client;
> -- 
> 1.7.10.4
>