From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 15/16] tmem: refator function tmem_ensure_avail_pages() Date: Fri, 22 Nov 2013 13:22:10 -0500 Message-ID: <20131122182210.GI8120@phenom.dumpdata.com> References: <1384937185-24749-1-git-send-email-bob.liu@oracle.com> <1384937185-24749-15-git-send-email-bob.liu@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VjvMx-0000Un-Pm for xen-devel@lists.xenproject.org; Fri, 22 Nov 2013 18:22:19 +0000 Content-Disposition: inline In-Reply-To: <1384937185-24749-15-git-send-email-bob.liu@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, keir@xen.org, ian.campbell@citrix.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On Wed, Nov 20, 2013 at 04:46:24PM +0800, Bob Liu wrote: > tmem_ensure_avail_pages() doesn't return a value which is incorrect because > the caller need to confirm there is enough memory. Which it was not doing until now. Ouch. > > Signed-off-by: Bob Liu > --- > xen/common/tmem.c | 43 +++++++++++++++++++------------------------ > xen/include/xen/tmem_xen.h | 6 ------ > 2 files changed, 19 insertions(+), 30 deletions(-) > > diff --git a/xen/common/tmem.c b/xen/common/tmem.c > index 2e807d4..0bc33ee 100644 > --- a/xen/common/tmem.c > +++ b/xen/common/tmem.c > @@ -1296,22 +1296,28 @@ static unsigned long tmem_relinquish_npages(unsigned long n) > return avail_pages; > } > > -/* Under certain conditions (e.g. if each client is putting pages for exactly > +/* > + * Under certain conditions (e.g. if each client is putting pages for exactly > * one object), once locks are held, freeing up memory may > * result in livelocks and very long "put" times, so we try to ensure there > * is a minimum amount of memory (1MB) available BEFORE any data structure > - * locks are held */ > -static inline void tmem_ensure_avail_pages(void) > + * locks are held. > + */ > +static inline bool_t tmem_ensure_avail_pages(void) > { > int failed_evict = 10; > + unsigned long free_mem; > > - while ( !tmem_free_mb() ) > - { > - if ( tmem_evict() ) > - continue; > - else if ( failed_evict-- <= 0 ) > - break; > - } > + do { > + free_mem = (tmem_page_list_pages + total_free_pages()) > + >> (20 - PAGE_SHIFT); > + if ( free_mem ) > + return 1; > + if ( !tmem_evict() ) > + failed_evict--; > + } while ( failed_evict > 0 ); > + > + return 0; > } > > /************ TMEM CORE OPERATIONS ************************************/ > @@ -2282,9 +2288,6 @@ long do_tmem_op(tmem_cli_op_t uops) > struct tmem_pool *pool = NULL; > struct oid *oidp; > int rc = 0; > - bool_t succ_get = 0, succ_put = 0; > - bool_t non_succ_get = 0, non_succ_put = 0; > - bool_t flush = 0, flush_obj = 0; Um, OK, they seem to belong to a different patch? > bool_t write_lock_set = 0, read_lock_set = 0; > > if ( !tmem_initialized ) > @@ -2299,8 +2302,7 @@ long do_tmem_op(tmem_cli_op_t uops) > if ( unlikely(tmem_get_tmemop_from_client(&op, uops) != 0) ) > { > tmem_client_err("tmem: can't get tmem struct from %s\n", tmem_client_str); > - rc = -EFAULT; > - return rc; > + return -EFAULT; This too. > } > > if ( op.cmd == TMEM_CONTROL ) > @@ -2369,28 +2371,21 @@ long do_tmem_op(tmem_cli_op_t uops) > op.u.creat.uuid[0], op.u.creat.uuid[1]); > break; > case TMEM_PUT_PAGE: > - tmem_ensure_avail_pages(); > - rc = do_tmem_put(pool, oidp, op.u.gen.index, op.u.gen.cmfn, > + if ( tmem_ensure_avail_pages() ) > + rc = do_tmem_put(pool, oidp, op.u.gen.index, op.u.gen.cmfn, > tmem_cli_buf_null); > - if (rc == 1) succ_put = 1; > - else non_succ_put = 1; which we ignore.. > break; > case TMEM_GET_PAGE: > rc = do_tmem_get(pool, oidp, op.u.gen.index, op.u.gen.cmfn, > tmem_cli_buf_null); > - if (rc == 1) succ_get = 1; > - else non_succ_get = 1; ugh. > break; > case TMEM_FLUSH_PAGE: > - flush = 1; > rc = do_tmem_flush_page(pool, oidp, op.u.gen.index); > break; > case TMEM_FLUSH_OBJECT: > rc = do_tmem_flush_object(pool, oidp); > - flush_obj = 1; > break; > case TMEM_DESTROY_POOL: > - flush = 1; Those really belong to a seperate cleanup patch. > rc = do_tmem_destroy_pool(op.pool_id); > break; > default: > diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h > index bf3be62..258e8e7 100644 > --- a/xen/include/xen/tmem_xen.h > +++ b/xen/include/xen/tmem_xen.h > @@ -164,13 +164,7 @@ static inline void __tmem_free_page(struct page_info *pi) > atomic_dec(&freeable_page_count); > } > > -static inline unsigned long tmem_free_mb(void) > -{ > - return (tmem_page_list_pages + total_free_pages()) >> (20 - PAGE_SHIFT); > -} > - > /* "Client" (==domain) abstraction */ > - > struct client; > static inline struct client *tmem_client_from_cli_id(domid_t cli_id) > { > -- > 1.7.10.4 >