From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 40C062C19 for ; Fri, 3 Jun 2016 14:28:46 +0200 (CEST) Received: by mail-wm0-f46.google.com with SMTP id n184so123693769wmn.1 for ; Fri, 03 Jun 2016 05:28:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=v1Yc0aBpj4o0C46f59uY9luYgtnLCMEo5Gpz4djayPU=; b=REPLwk62zf3xetwEFv1TbNX0zOQUvRhIQsu6h4rn700y62lmof6gA/mUYuMQYF+TGa 7IRAWjqdxi4edgRbYVg0yxmovvXNVVCbgWwmD9H/bsQikUpFfYeofZi/RVClJE2zIQMA +6REgCB5w9suiClkWR9I9og7oZY41fYHWNt2kIiXsukhG+nEWlRv35wmzH8wKF8DcUIa X3+FMhpoKiqYdoCRDnqaN2/LzpNpqNmKJ2qtKi+uLg4DKfcyTWqcZQXFHvnocyFPA1rA tH81tNgvrN/mtRbQJF1cbRJADA6BCh0OXl8QCHEA0YgRqwEbLNNNTXrCZFkgBDPy5qQx 2X2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=v1Yc0aBpj4o0C46f59uY9luYgtnLCMEo5Gpz4djayPU=; b=fzlXmssz+esk1TsasdmVwy6lmrMxzRnbApfwTkx/Ni4oovyoZz4qdnTQnIWelhJgzD 22O+S/aBTb/HAD8mmgIe9A4a0PJDu4I6Al+EhO07beYM9UPo9zqTBvU+K91LOSgek8CL 6DFveDpXo5vG0P90hWqOqLYJDMlagiCOSLBF7iGKyp1aFtnty/ImZhvqk25lYbKA7qA5 QnUXZdc0K1DdRp5bcrNQfjOBkYffcVFsPiZ+4YPG/2SeAe/VOw2+mSoo0ZONIS5Mu/AM Sh1LPZyDJppkHgKZZGPVmq7/wwu5Gnr6wER1i9eK7NA6OIUF/M0csSTg2vi4sEZXT38G R4rw== X-Gm-Message-State: ALyK8tKop4hzvNlHThfSuJWbj9WrIhKj+kIlHpKgAZU9AIXpLP1TuIUEAA7ilG5bLR5ISSZ5 X-Received: by 10.194.235.193 with SMTP id uo1mr3251250wjc.1.1464956925816; Fri, 03 Jun 2016 05:28:45 -0700 (PDT) Received: from [192.168.0.11] (was59-1-82-226-113-214.fbx.proxad.net. [82.226.113.214]) by smtp.gmail.com with ESMTPSA id e1sm5533430wjv.9.2016.06.03.05.28.44 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 03 Jun 2016 05:28:45 -0700 (PDT) To: David Hunt , dev@dpdk.org References: <1464797998-76690-1-git-send-email-david.hunt@intel.com> <1464874043-67467-1-git-send-email-david.hunt@intel.com> <1464874043-67467-2-git-send-email-david.hunt@intel.com> Cc: viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com From: Olivier MATZ Message-ID: <575177FB.7080301@6wind.com> Date: Fri, 3 Jun 2016 14:28:43 +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: <1464874043-67467-2-git-send-email-david.hunt@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v7 1/5] 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Jun 2016 12:28:46 -0000 Hi, Some comments below. On 06/02/2016 03:27 PM, David Hunt wrote: > --- a/lib/librte_mempool/rte_mempool.h > +++ b/lib/librte_mempool/rte_mempool.h > +/** > + * 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 mostlikely point to a different type of data structure, and > + * will be transparent to the application programmer. > + * The function should also not touch the given *mp instance. > + */ > +typedef void *(*rte_mempool_alloc_t)(const struct rte_mempool *mp); nit: about doxygen comments, it's better to have a one-line description, then a blank line, then the full description. While there, please also check the uppercase at the beginning and the dot when relevant. > +/** > + * Structure storing the table of registered ops structs, each of which contain > + * the function pointers for the mempool ops functions. > + * Each process has it's own storage for this ops struct aray so that it's -> its aray -> array > + * the mempools can be shared across primary and secondary processes. > + * The indices used to access the array are valid across processes, whereas > + * any function pointers stored directly in the mempool struct would not be. > + * This results in us simply having "ops_index" in the mempool struct. > + */ > +struct rte_mempool_ops_table { > + rte_spinlock_t sl; /**< Spinlock for add/delete. */ > + uint32_t num_ops; /**< Number of used ops structs in the table. */ > + /** > + * Storage for all possible ops structs. > + */ > + struct rte_mempool_ops ops[RTE_MEMPOOL_MAX_OPS_IDX]; > +} __rte_cache_aligned; > + > +/** Array of registered ops structs */ > +extern struct rte_mempool_ops_table rte_mempool_ops_table; > + > +/** > + * @internal Get the mempool ops struct from its index. > + * > + * @param ops_index > + * The index of the ops struct in the ops struct table. It must be a valid > + * index: (0 <= idx < num_ops). > + * @return > + * The pointer to the ops struct in the table. > + */ > +static inline struct rte_mempool_ops * > +rte_mempool_ops_get(int ops_index) > +{ > + return &rte_mempool_ops_table.ops[ops_index]; > +} > + > +/** > + * @internal wrapper for external mempool manager alloc callback. wrapper for mempool_ops alloc callback ? (same for other functions) > @@ -922,7 +1124,7 @@ rte_mempool_put(struct rte_mempool *mp, void *obj) > */ > static inline int __attribute__((always_inline)) > __mempool_get_bulk(struct rte_mempool *mp, void **obj_table, > - unsigned n, int is_mc) > + unsigned int n, int is_mc) > { > int ret; > struct rte_mempool_cache *cache; Despite "unsigned" is not conform to current checkpatch policy, I don't think it should be modified in this patch. > --- /dev/null > +++ b/lib/librte_mempool/rte_mempool_ops.c > + > +#include > + > +/* indirect jump table to support external memory pools */ > +struct rte_mempool_ops_table rte_mempool_ops_table = { > + .sl = RTE_SPINLOCK_INITIALIZER , > + .num_ops = 0 > +}; > + > +/* add a new ops struct in rte_mempool_ops_table, return its index */ > +int > +rte_mempool_ops_register(const struct rte_mempool_ops *h) nit: "h" should be "ops" :) > +{ > + struct rte_mempool_ops *ops; > + int16_t ops_index; > + > + rte_spinlock_lock(&rte_mempool_ops_table.sl); > + > + if (rte_mempool_ops_table.num_ops >= > + RTE_MEMPOOL_MAX_OPS_IDX) { > + rte_spinlock_unlock(&rte_mempool_ops_table.sl); > + RTE_LOG(ERR, MEMPOOL, > + "Maximum number of mempool ops structs exceeded\n"); > + return -ENOSPC; > + } > + > + if (h->put == NULL || h->get == NULL || h->get_count == NULL) { > + rte_spinlock_unlock(&rte_mempool_ops_table.sl); > + RTE_LOG(ERR, MEMPOOL, > + "Missing callback while registering mempool ops\n"); > + return -EINVAL; > + } > + > + ops_index = rte_mempool_ops_table.num_ops++; > + ops = &rte_mempool_ops_table.ops[ops_index]; > + snprintf(ops->name, sizeof(ops->name), "%s", h->name); I think we should check for truncation here, as it was done in: 85cf00791cca ("mem: avoid memzone/mempool/ring name truncation") (this should be done before the num_ops++) Regards, Olivier