|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 13/14] tmem: reorg the shared pool allocate path
On Tue, Jan 28, 2014 at 12:28:32PM +0800, Bob Liu wrote:
> Reorg the code to make it more readable.
> Check the return value of shared_pool_join() and drop a unneeded call to
> it. Disable creating a shared&persistant pool in an advance place.
>
> Signed-off-by: Bob Liu <bob.liu@xxxxxxxxxx>
> ---
> xen/common/tmem.c | 104
> +++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 70 insertions(+), 34 deletions(-)
>
> diff --git a/xen/common/tmem.c b/xen/common/tmem.c
> index 37f4e8f..ab5bba7 100644
> --- a/xen/common/tmem.c
> +++ b/xen/common/tmem.c
> @@ -1021,21 +1021,24 @@ static void pool_free(struct tmem_pool *pool)
> xfree(pool);
> }
>
> -/* register new_client as a user of this shared pool and return new
> - total number of registered users */
> +/*
> + * Register new_client as a user of this shared pool and return 0 on succ.
> + */
> static int shared_pool_join(struct tmem_pool *pool, struct client
> *new_client)
> {
> struct share_list *sl;
> -
> ASSERT(is_shared(pool));
> +
Stray?
> if ( (sl = tmem_malloc(sizeof(struct share_list), NULL)) == NULL )
> return -1;
> sl->client = new_client;
> list_add_tail(&sl->share_list, &pool->share_list);
> if ( new_client->cli_id != pool->client->cli_id )
> tmem_client_info("adding new %s %d to shared pool owned by %s %d\n",
> - tmem_client_str, new_client->cli_id, tmem_client_str,
> pool->client->cli_id);
> - return ++pool->shared_count;
> + tmem_client_str, new_client->cli_id, tmem_client_str,
> + pool->client->cli_id);
> + ++pool->shared_count;
> + return 0;
> }
>
> /* reassign "ownership" of the pool to another client that shares this pool
> */
> @@ -1841,8 +1844,7 @@ static int do_tmem_new_pool(domid_t this_cli_id,
> int specversion = (flags >> TMEM_POOL_VERSION_SHIFT)
> & TMEM_POOL_VERSION_MASK;
> struct tmem_pool *pool, *shpool;
> - int s_poolid, first_unused_s_poolid;
> - int i;
> + int i, first_unused_s_poolid;
>
> if ( this_cli_id == TMEM_CLI_ID_NULL )
> cli_id = current->domain->domain_id;
> @@ -1856,6 +1858,11 @@ static int do_tmem_new_pool(domid_t this_cli_id,
> tmem_client_err("failed... unsupported spec version\n");
> return -EPERM;
> }
> + if ( shared && persistent )
> + {
> + tmem_client_err("failed... unable to create a shared-persistant
> pool\n");
> + return -EPERM;
> + }
> if ( pagebits != (PAGE_SHIFT - 12) )
> {
> tmem_client_err("failed... unsupported pagesize %d\n",
> @@ -1872,17 +1879,12 @@ static int do_tmem_new_pool(domid_t this_cli_id,
> tmem_client_err("failed... reserved bits must be zero\n");
> return -EPERM;
> }
> - if ( (pool = pool_alloc()) == NULL )
> - {
> - tmem_client_err("failed... out of memory\n");
> - return -ENOMEM;
> - }
> if ( this_cli_id != TMEM_CLI_ID_NULL )
> {
> if ( (client = tmem_client_from_cli_id(this_cli_id)) == NULL
> || d_poolid >= MAX_POOLS_PER_DOMAIN
> || client->pools[d_poolid] != NULL )
> - goto fail;
> + return -EPERM;
> }
> else
> {
> @@ -1895,13 +1897,35 @@ static int do_tmem_new_pool(domid_t this_cli_id,
> {
> tmem_client_err("failed... no more pool slots available for this
> %s\n",
> tmem_client_str);
> - goto fail;
> + return -EPERM;
> }
> }
> +
> + if ( (pool = pool_alloc()) == NULL )
> + {
> + tmem_client_err("failed... out of memory\n");
> + return -ENOMEM;
> + }
> + client->pools[d_poolid] = pool;
> + pool->client = client;
> + pool->pool_id = d_poolid;
> + pool->shared = shared;
> + pool->persistent = persistent;
> + pool->uuid[0] = uuid_lo;
> + pool->uuid[1] = uuid_hi;
> +
> + /*
> + * Already created a pool when arrived here, but need some special
> process
> + * for shared pool.
> + */
> if ( shared )
> {
> if ( uuid_lo == -1L && uuid_hi == -1L )
> - shared = 0;
> + {
> + tmem_client_info("Invalid uuid, create non shared pool instead!\n");
> + pool->shared = 0;
spaces.
> + goto out;
We don't set any errors? Should we do that and return?
(And potentially do the pool_alloc even further down)
> + }
> if ( client->shared_auth_required && !global_shared_auth )
> {
> for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++)
> @@ -1909,48 +1933,60 @@ static int do_tmem_new_pool(domid_t this_cli_id,
> (client->shared_auth_uuid[i][1] == uuid_hi) )
> break;
> if ( i == MAX_GLOBAL_SHARED_POOLS )
> - shared = 0;
> + {
> + tmem_client_info("Shared auth failed, create non shared pool
> instead!\n");
> + pool->shared = 0;
> + goto out;
Ditto. Should we set errors and return?
(And naturally do the pool_allocation even further down?)
> + }
> }
> - }
> - pool->shared = shared;
> - pool->client = client;
> - if ( shared )
> - {
> +
> + /*
> + * Authorize okay, match a global shared pool or use the newly allocated
> + * one
> + */
> first_unused_s_poolid = MAX_GLOBAL_SHARED_POOLS;
> - for ( s_poolid = 0; s_poolid < MAX_GLOBAL_SHARED_POOLS; s_poolid++ )
> + for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++ )
> {
> - if ( (shpool = global_shared_pools[s_poolid]) != NULL )
> + if ( (shpool = global_shared_pools[i]) != NULL )
> {
> if ( shpool->uuid[0] == uuid_lo && shpool->uuid[1] ==
> uuid_hi )
> {
> + /* Succ to match a global shared pool */
> tmem_client_info("(matches shared pool
> uuid=%"PRIx64".%"PRIx64") pool_id=%d\n",
> uuid_hi, uuid_lo, d_poolid);
> - client->pools[d_poolid] = global_shared_pools[s_poolid];
> - shared_pool_join(global_shared_pools[s_poolid], client);
> - pool_free(pool);
> - return d_poolid;
> + client->pools[d_poolid] = shpool;
> + if ( !shared_pool_join(shpool, client) )
Yeey! Error checking.
> + {
> + pool_free(pool);
> + goto out;
> + }
> + else
> + goto fail;
At this point we would return d_poolid. Is that OK? Should we instead
return an error?
> }
> }
> - else if ( first_unused_s_poolid == MAX_GLOBAL_SHARED_POOLS )
> - first_unused_s_poolid = s_poolid;
> + else
> + {
> + if ( first_unused_s_poolid == MAX_GLOBAL_SHARED_POOLS )
> + first_unused_s_poolid = i;
> + }
> }
> +
> + /* Failed to find a global shard pool slot */
Spaces.
> if ( first_unused_s_poolid == MAX_GLOBAL_SHARED_POOLS )
> {
> tmem_client_warn("tmem: failed... no global shared pool slots
> available\n");
> goto fail;
> }
> + /* Add pool to global shard pool */
Spaces.
> else
> {
> INIT_LIST_HEAD(&pool->share_list);
> pool->shared_count = 0;
> global_shared_pools[first_unused_s_poolid] = pool;
> - (void)shared_pool_join(pool,client);
> }
> }
> - client->pools[d_poolid] = pool;
> - pool->pool_id = d_poolid;
> - pool->persistent = persistent;
> - pool->uuid[0] = uuid_lo; pool->uuid[1] = uuid_hi;
> +
> +out:
> tmem_client_info("pool_id=%d\n", d_poolid);
> return d_poolid;
>
> --
> 1.7.10.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |