From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 13/16] tmem: cleanup: __tmem_alloc_page: drop unneed parameters Date: Tue, 26 Nov 2013 12:38:44 -0500 Message-ID: <20131126173844.GK2959@phenom.dumpdata.com> References: <1384937185-24749-1-git-send-email-bob.liu@oracle.com> <1384937185-24749-13-git-send-email-bob.liu@oracle.com> <20131122181753.GH8120@phenom.dumpdata.com> <52945EC6.1080906@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VlMb8-0004UN-KG for xen-devel@lists.xenproject.org; Tue, 26 Nov 2013 17:38:54 +0000 Content-Disposition: inline In-Reply-To: <52945EC6.1080906@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Bob Liu Cc: xen-devel@lists.xenproject.org, Bob Liu , keir@xen.org, ian.campbell@citrix.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On Tue, Nov 26, 2013 at 04:41:42PM +0800, Bob Liu wrote: > > On 11/23/2013 02:17 AM, Konrad Rzeszutek Wilk wrote: > > 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. And I think that is not an issue anymore. > > > >> tmem_called_from_tmem() was also dropped by this patch. > >> > >> Signed-off-by: Bob Liu > >> --- > >> 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. > > > > I don't think so. > If pass '1' to __tmem_alloc_page(), __tmem_alloc_page() only call > tmem_page_list_get() and return. > It's no difference, so an extra parameter is unneeded. Right! Thanks for spotting that. I read that new change as: + while ( (pfp = __tmem_alloc_page()) == NULL ) ! > > > > >> { > >> 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; > >> } > >> > > See __tmem_alloc_page() implementation here. > > -- > Regards, > -Bob