From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Bob Liu <lliubbo@gmail.com>
Cc: xen-devel@lists.xenproject.org, keir@xen.org,
ian.campbell@citrix.com, JBeulich@suse.com
Subject: Re: [PATCH 04/16] tmem: cleanup: rm unneeded parameters from put path
Date: Fri, 22 Nov 2013 12:54:45 -0500 [thread overview]
Message-ID: <20131122175445.GB8120@phenom.dumpdata.com> (raw)
In-Reply-To: <1384937185-24749-4-git-send-email-bob.liu@oracle.com>
On Wed, Nov 20, 2013 at 04:46:13PM +0800, Bob Liu wrote:
> tmem only takes a page as unit, so parameters tmem_offset, pfn_offset and len
> are meanless, this patch remove those parameters from do_tmem_put() related path
^^^^^^^^ ^-s
Not exactly right. They were used by TMEM_WRITE and TMEM_READ, but your
other patch "tmem: cleanup: rm unused tmem_op" removed them.
Looking a bit on Google I see this thread:
http://marc.info/?l=xen-devel&m=136975816209598
which implies that the Windows GPL driver is using it? Did you
check its code? (See http://xenbits.xen.org/ext/win-pvdrivers/)
If it is not in the code, can you also CC James Harper on it so
he knows not use those ops?
>
> Signed-off-by: Bob Liu <bob.liu@oracle.com>
> ---
> xen/common/tmem.c | 32 +++++++++++++++-----------------
> xen/common/tmem_xen.c | 27 +++++----------------------
> xen/include/xen/tmem_xen.h | 3 +--
> 3 files changed, 21 insertions(+), 41 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index c272930..e2e69bf 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -1421,7 +1421,6 @@ out:
> }
>
> static int do_tmem_dup_put(struct tmem_page_descriptor *pgp, xen_pfn_t cmfn,
> - pagesize_t tmem_offset, pagesize_t pfn_offset, pagesize_t len,
> tmem_cli_va_param_t clibuf)
> {
> struct tmem_pool *pool;
> @@ -1442,7 +1441,7 @@ static int do_tmem_dup_put(struct tmem_page_descriptor *pgp, xen_pfn_t cmfn,
> if ( client->live_migrating )
> goto failed_dup; /* no dups allowed when migrating */
> /* can we successfully manipulate pgp to change out the data? */
> - if ( len != 0 && client->compress && pgp->size != 0 )
> + if ( client->compress && pgp->size != 0 )
> {
> ret = do_tmem_put_compress(pgp, cmfn, clibuf);
> if ( ret == 1 )
> @@ -1461,9 +1460,7 @@ copy_uncompressed:
> if ( ( pgp->pfp = tmem_page_alloc(pool) ) == NULL )
> goto failed_dup;
> pgp->size = 0;
> - /* tmem_copy_from_client properly handles len==0 and offsets != 0 */
> - ret = tmem_copy_from_client(pgp->pfp, cmfn, tmem_offset, pfn_offset, len,
> - tmem_cli_buf_null);
> + ret = tmem_copy_from_client(pgp->pfp, cmfn, tmem_cli_buf_null);
> if ( ret < 0 )
> goto bad_copy;
> if ( tmem_dedup_enabled() && !is_persistent(pool) )
> @@ -1509,11 +1506,9 @@ cleanup:
> return ret;
> }
>
> -
> static int do_tmem_put(struct tmem_pool *pool,
> struct oid *oidp, uint32_t index,
> - xen_pfn_t cmfn, pagesize_t tmem_offset,
> - pagesize_t pfn_offset, pagesize_t len, tmem_cli_va_param_t clibuf)
> + xen_pfn_t cmfn, tmem_cli_va_param_t clibuf)
> {
> struct tmem_object_root *obj = NULL, *objfound = NULL, *objnew = NULL;
> struct tmem_page_descriptor *pgp = NULL, *pgpdel = NULL;
> @@ -1527,8 +1522,7 @@ static int do_tmem_put(struct tmem_pool *pool,
> {
> ASSERT_SPINLOCK(&objfound->obj_spinlock);
> if ((pgp = pgp_lookup_in_obj(objfound, index)) != NULL)
> - return do_tmem_dup_put(pgp, cmfn, tmem_offset, pfn_offset, len,
> - clibuf);
> + return do_tmem_dup_put(pgp, cmfn, clibuf);
> }
>
> /* no puts allowed into a frozen pool (except dup puts) */
> @@ -1560,7 +1554,7 @@ static int do_tmem_put(struct tmem_pool *pool,
> pgp->index = index;
> pgp->size = 0;
>
> - if ( len != 0 && client->compress )
> + if ( client->compress )
> {
> ASSERT(pgp->pfp == NULL);
> ret = do_tmem_put_compress(pgp, cmfn, clibuf);
> @@ -1586,9 +1580,7 @@ copy_uncompressed:
> ret = -ENOMEM;
> goto delete_and_free;
> }
> - /* tmem_copy_from_client properly handles len==0 (TMEM_NEW_PAGE) */
> - ret = tmem_copy_from_client(pgp->pfp, cmfn, tmem_offset, pfn_offset, len,
> - clibuf);
> + ret = tmem_copy_from_client(pgp->pfp, cmfn, clibuf);
> if ( ret < 0 )
> goto bad_copy;
> if ( tmem_dedup_enabled() && !is_persistent(pool) )
> @@ -2440,10 +2432,17 @@ static int tmemc_restore_put_page(int cli_id, uint32_t pool_id, struct oid *oidp
> struct client *client = tmem_client_from_cli_id(cli_id);
> struct tmem_pool *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
> ? NULL : client->pools[pool_id];
> + unsigned int pagesize;
>
> if ( pool == NULL )
> return -1;
> - return do_tmem_put(pool, oidp, index, 0, 0, 0, bufsize, buf);
> + pagesize = 1 << (pool->pageshift + 12);
Ahem! 12? #define please.
> + if (bufsize != pagesize) {
> + tmem_client_err("tmem: %s bufsize %d not page aligend(%d)\n", __func__,
> + bufsize, pagesize);
> + return -1;
> + }
> + return do_tmem_put(pool, oidp, index, 0, buf);
> }
>
> static int tmemc_restore_flush_page(int cli_id, uint32_t pool_id, struct oid *oidp,
> @@ -2648,8 +2647,7 @@ EXPORT long do_tmem_op(tmem_cli_op_t uops)
> break;
> case TMEM_PUT_PAGE:
> tmem_ensure_avail_pages();
> - rc = do_tmem_put(pool, oidp, op.u.gen.index, op.u.gen.cmfn, 0, 0,
> - PAGE_SIZE, tmem_cli_buf_null);
> + 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;
> break;
> diff --git a/xen/common/tmem_xen.c b/xen/common/tmem_xen.c
> index af67703..165c7cf 100644
> --- a/xen/common/tmem_xen.c
> +++ b/xen/common/tmem_xen.c
> @@ -100,25 +100,16 @@ static inline void cli_put_page(void *cli_va, struct page_info *cli_pfp,
> #endif
>
> EXPORT int tmem_copy_from_client(struct page_info *pfp,
> - xen_pfn_t cmfn, pagesize_t tmem_offset,
> - pagesize_t pfn_offset, pagesize_t len, tmem_cli_va_param_t clibuf)
> + xen_pfn_t cmfn, tmem_cli_va_param_t clibuf)
> {
> unsigned long tmem_mfn, cli_mfn = 0;
> char *tmem_va, *cli_va = NULL;
> struct page_info *cli_pfp = NULL;
> int rc = 1;
>
> - if ( tmem_offset > PAGE_SIZE || pfn_offset > PAGE_SIZE || len > PAGE_SIZE )
> - return -EINVAL;
> ASSERT(pfp != NULL);
> tmem_mfn = page_to_mfn(pfp);
> tmem_va = map_domain_page(tmem_mfn);
> - if ( tmem_offset == 0 && pfn_offset == 0 && len == 0 )
> - {
> - memset(tmem_va, 0, PAGE_SIZE);
> - unmap_domain_page(tmem_va);
> - return 1;
> - }
> if ( guest_handle_is_null(clibuf) )
> {
> cli_va = cli_get_page(cmfn, &cli_mfn, &cli_pfp, 0);
> @@ -129,21 +120,13 @@ EXPORT int tmem_copy_from_client(struct page_info *pfp,
> }
> }
> smp_mb();
> - if ( len == PAGE_SIZE && !tmem_offset && !pfn_offset && cli_va )
> - memcpy(tmem_va, cli_va, PAGE_SIZE);
> - else if ( (tmem_offset+len <= PAGE_SIZE) &&
> - (pfn_offset+len <= PAGE_SIZE) )
> + if ( cli_va )
> {
> - if ( cli_va )
> - memcpy(tmem_va + tmem_offset, cli_va + pfn_offset, len);
> - else if ( copy_from_guest_offset(tmem_va + tmem_offset, clibuf,
> - pfn_offset, len) )
> - rc = -EFAULT;
> + memcpy(tmem_va, cli_va, PAGE_SIZE);
> + cli_put_page(cli_va, cli_pfp, cli_mfn, 0);
> }
> - else if ( len )
> + else
> rc = -EINVAL;
> - if ( cli_va )
> - cli_put_page(cli_va, cli_pfp, cli_mfn, 0);
> unmap_domain_page(tmem_va);
> return rc;
> }
> diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h
> index 4e4c227..25ce268 100644
> --- a/xen/include/xen/tmem_xen.h
> +++ b/xen/include/xen/tmem_xen.h
> @@ -385,8 +385,7 @@ int tmem_decompress_to_client(xen_pfn_t, void *, size_t,
> int tmem_compress_from_client(xen_pfn_t, void **, size_t *,
> tmem_cli_va_param_t);
>
> -int tmem_copy_from_client(struct page_info *, xen_pfn_t, pagesize_t tmem_offset,
> - pagesize_t pfn_offset, pagesize_t len, tmem_cli_va_param_t);
> +int tmem_copy_from_client(struct page_info *, xen_pfn_t, tmem_cli_va_param_t);
>
> int tmem_copy_to_client(xen_pfn_t, struct page_info *, pagesize_t tmem_offset,
> pagesize_t pfn_offset, pagesize_t len, tmem_cli_va_param_t);
> --
> 1.7.10.4
>
next prev parent reply other threads:[~2013-11-22 17:54 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-20 8:46 [PATCH 01/16] tmem: cleanup: drop some debug code Bob Liu
2013-11-20 8:46 ` [PATCH 02/16] tmem: cleanup: drop useless function 'tmem_copy_page' Bob Liu
2013-11-20 8:46 ` [PATCH 03/16] tmem: cleanup: rm unused tmem_op Bob Liu
2013-11-22 17:38 ` Konrad Rzeszutek Wilk
2013-11-25 9:43 ` Jan Beulich
2013-11-25 9:52 ` Ian Campbell
2013-11-25 9:58 ` Jan Beulich
2013-11-25 16:37 ` Konrad Rzeszutek Wilk
2013-11-25 16:40 ` Ian Campbell
2013-11-25 17:09 ` Konrad Rzeszutek Wilk
2013-11-25 17:12 ` Ian Campbell
2013-11-25 19:56 ` Konrad Rzeszutek Wilk
2013-11-26 8:56 ` Bob Liu
2013-11-20 8:46 ` [PATCH 04/16] tmem: cleanup: rm unneeded parameters from put path Bob Liu
2013-11-22 17:54 ` Konrad Rzeszutek Wilk [this message]
2013-11-26 8:22 ` Bob Liu
2013-11-20 8:46 ` [PATCH 05/16] tmem: cleanup: rm unneeded parameters from get path Bob Liu
2013-11-22 17:55 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 06/16] tmem: cleanup: reorg do_tmem_put() Bob Liu
2013-11-22 18:04 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 07/16] tmem: drop unneeded is_ephemeral() and is_private() Bob Liu
2013-11-20 8:46 ` [PATCH 08/16] tmem: cleanup: rm useless EXPORT/FORWARD define Bob Liu
2013-11-22 18:05 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 09/16] tmem: cleanup: drop tmemc_list() temporary Bob Liu
2013-11-22 18:07 ` Konrad Rzeszutek Wilk
2013-11-26 8:28 ` Bob Liu
2013-11-22 21:00 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 10/16] tmem: cleanup: drop runtime statistics Bob Liu
2013-11-22 18:08 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 11/16] tmem: cleanup: drop tmem_lock_all Bob Liu
2013-11-20 8:46 ` [PATCH 12/16] tmem: cleanup: refactor the alloc/free path Bob Liu
2013-11-20 8:46 ` [PATCH 13/16] tmem: cleanup: __tmem_alloc_page: drop unneed parameters Bob Liu
2013-11-22 18:17 ` Konrad Rzeszutek Wilk
2013-11-26 8:41 ` Bob Liu
2013-11-26 17:38 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 14/16] tmem: cleanup: drop useless functions from head file Bob Liu
2013-11-27 14:38 ` Andrew Cooper
2013-11-27 14:52 ` Konrad Rzeszutek Wilk
2013-11-27 14:59 ` Andrew Cooper
2013-11-27 15:55 ` Jan Beulich
2013-11-20 8:46 ` [PATCH 15/16] tmem: refator function tmem_ensure_avail_pages() Bob Liu
2013-11-22 18:22 ` Konrad Rzeszutek Wilk
2013-11-20 8:46 ` [PATCH 16/16] tmem: cleanup: rename tmem_relinquish_npages() Bob Liu
2013-11-20 9:08 ` [PATCH 01/16] tmem: cleanup: drop some debug code Jan Beulich
2013-11-20 9:19 ` Bob Liu
2013-11-20 9:25 ` Jan Beulich
2013-11-20 13:51 ` Konrad Rzeszutek Wilk
2013-11-20 14:21 ` Jan Beulich
2013-11-20 18:46 ` Konrad Rzeszutek Wilk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131122175445.GB8120@phenom.dumpdata.com \
--to=konrad.wilk@oracle.com \
--cc=JBeulich@suse.com \
--cc=ian.campbell@citrix.com \
--cc=keir@xen.org \
--cc=lliubbo@gmail.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.