From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
by dpdk.org (Postfix) with ESMTP id 109895586
for <dev@dpdk.org>; Mon, 13 Jun 2016 14:17:00 +0200 (CEST)
Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214]
helo=[192.168.0.10])
by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128)
(Exim 4.84_2) (envelope-from <olivier.matz@6wind.com>)
id 1bCQpj-0007tF-Ie; Mon, 13 Jun 2016 14:19:12 +0200
To: David Hunt <david.hunt@intel.com>, dev@dpdk.org
References: <1464965906-108927-1-git-send-email-david.hunt@intel.com>
<1465571806-22008-1-git-send-email-david.hunt@intel.com>
<1465571806-22008-2-git-send-email-david.hunt@intel.com>
Cc: viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com,
shreyansh.jain@nxp.com
From: Olivier Matz <olivier.matz@6wind.com>
Message-ID: <575EA42D.2030003@6wind.com>
Date: Mon, 13 Jun 2016 14:16:45 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
Icedove/38.6.0
MIME-Version: 1.0
In-Reply-To: <1465571806-22008-2-git-send-email-david.hunt@intel.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH v9 1/3] mempool: support external mempool
operations
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 13 Jun 2016 12:17:00 -0000
Hi David,
Some comments below.
On 06/10/2016 05:16 PM, David Hunt wrote:
> Until now, the objects stored in a mempool were internally stored in a
> ring. This patch introduces the possibility to register external handlers
> replacing the ring.
>
> The default behavior remains unchanged, but calling the new function
> rte_mempool_set_handler() right after rte_mempool_create_empty() allows
> the user to change the handler that will be used when populating
> the mempool.
>
> This patch also adds a set of default ops (function callbacks) based
> on rte_ring.
>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
>
> ...
> @@ -386,10 +352,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr,
> int ret;
>
> /* create the internal ring if not already done */
> - if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> - ret = rte_mempool_ring_create(mp);
> - if (ret < 0)
> - return ret;
> + if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) {
> + rte_errno = 0;
> + ret = rte_mempool_ops_alloc(mp);
> + if (ret != 0) {
> + if (rte_errno == 0)
> + return -EINVAL;
> + return -rte_errno;
> + }
> }
The rte_errno should be removed. Just return the error code from
rte_mempool_ops_alloc() on failure.
> +/** Structure defining mempool operations structure */
> +struct rte_mempool_ops {
> + char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct */
> + rte_mempool_alloc_t alloc; /**< Allocate private data */
> + rte_mempool_free_t free; /**< Free the external pool. */
> + rte_mempool_put_t put; /**< Put an object. */
> + rte_mempool_get_t get; /**< Get an object. */
> + rte_mempool_get_count get_count; /**< Get qty of available objs. */
> +} __rte_cache_aligned;
> +
Sorry, I missed that in the previous reviews, but since the get/put
functions have been renamed in dequeue/enqueue, I think the same change
should also be done here.
> +/**
> + * Prototype for implementation specific data provisioning function.
> + *
> + * The function should provide the implementation specific memory for
> + * for use by the other mempool ops functions in a given mempool ops struct.
> + * E.g. the default ops provides an instance of the rte_ring for this purpose.
> + * it will most likely point to a different type of data structure, and
> + * will be transparent to the application programmer.
> + */
> +typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp);
A comment saying that this function should set mp->pool_data
would be nice here, I think.
> +/* wrapper to allocate an external mempool's private (pool) data */
> +int
> +rte_mempool_ops_alloc(struct rte_mempool *mp)
> +{
> + struct rte_mempool_ops *ops;
> +
> + ops = rte_mempool_ops_get(mp->ops_index);
> + if (ops->alloc == NULL)
> + return -ENOMEM;
> + return ops->alloc(mp);
> +}
Now that we check that ops->alloc != NULL in the register function,
I wonder if we should keep this test or not. Yes, it doesn't hurt,
but for consistency with the other functions (get/put/get_count),
we may remove it.
This would be a good thing because it would prevent any confusion
with rte_mempool_ops_get(), which returns a pointer to the ops struct
(and has nothing to do with ops->get()).
Regards,
Olivier