From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so2.wedos.net (wes1-so2.wedos.net [46.28.106.16]) by dpdk.org (Postfix) with ESMTP id 283CC58D9 for ; Fri, 3 Jun 2016 13:42:25 +0200 (CEST) Received: from [127.0.0.1] (cst-prg-105-0.cust.vodafone.cz [46.135.105.0]) by wes1-so2.wedos.net (Postfix) with ESMTPSA id 3rLhz01rC9zBhm; Fri, 3 Jun 2016 13:42:19 +0200 (CEST) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: BlackBerry Email (10.3.2.2876) Message-ID: <20160603114219.5861459.63711.4484@rehivetech.com> Date: Fri, 03 Jun 2016 13:42:19 +0200 From: Jan Viktorin In-Reply-To: <575164FF.1080509@6wind.com> 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> <20160603063755.GA5277@localhost.localdomain> <57515BBE.4070205@intel.com> <575164FF.1080509@6wind.com> To: Olivier MATZ , "Hunt, David" , Jerin Jacob Cc: dev@dpdk.org 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 11:42:25 -0000 Hello, sorry for top-posting. I vote for passing rte_mempool instead of void *. Th= is is what I've already pointed at, a kind of type-safety...=E2=80=8E Passi= ng uint64_t just hides the problem. Another way is to name the union and pa= ss it as eg. union rte_mempool_priv * to the callbacks. Jan=C2=A0Viktorin RehiveTech Sent=C2=A0from=C2=A0a=C2=A0mobile=C2=A0device =C2=A0 P=C5=AFvodn=C3=AD zpr=C3=A1va =C2=A0 Od: Olivier MATZ Odesl=C3=A1no: p=C3=A1tek, 3. =C4=8Dervna 2016 13:08 Komu: Hunt, David; Jerin Jacob Kopie: dev@dpdk.org; viktorin@rehivetech.com P=C5=99edm=C4=9Bt: Re: [PATCH v7 1/5] mempool: support external mempool ope= rations On 06/03/2016 12:28 PM, Hunt, David wrote: > On 6/3/2016 7:38 AM, Jerin Jacob wrote: >> On Thu, Jun 02, 2016 at 02:27:19PM +0100, David Hunt wrote: >>> +/** >>> + * @internal wrapper for external mempool manager put callback. >>> + * >>> + * @param mp >>> + * Pointer to the memory pool. >>> + * @param obj_table >>> + * Pointer to a table of void * pointers (objects). >>> + * @param n >>> + * Number of objects to put. >>> + * @return >>> + * - 0: Success; n objects supplied. >>> + * - <0: Error; code of put function. >>> + */ >>> +static inline int >>> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const >>> *obj_table, >>> + unsigned n) >>> +{ >>> + struct rte_mempool_ops *ops; >>> + >>> + ops =3D rte_mempool_ops_get(mp->ops_index); >>> + return ops->put(mp->pool_data, obj_table, n); >> Pass by value of "pool_data", On 32 bit systems, casting back to >> pool_id will >> be an issue as void* on 32 bit is 4B. IMO, May be can use uint64_t to >> pass by value and typecast to void* to fix it. >=20 > OK. I see the problem. I'll see 4 callbacks that need to change, free, > get, put and get_count. > So the callbacks will be: > typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp); > typedef void (*rte_mempool_free_t)(uint64_t p); > typedef int (*rte_mempool_put_t)(uint64_t p, void * const *obj_table, > unsigned int n); > typedef int (*rte_mempool_get_t)(uint64_t p, void **obj_table, unsigned > int n); > typedef unsigned (*rte_mempool_get_count)(uint64_t p); I don't quite like the uint64_t argument (I exepect that most handlers will use a pointer, and they will have to do a cast). What about giving a (struct rte_mempool *) instead? The handler function would then select between void * or uint64_t without a cast. In that case, maybe the prototype of alloc should be: typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp); It would directly set mp->pool_data and return 0 or -errno. By the way, I found a strange thing: > typedef void (*rte_mempool_free_t)(void *p); [...] > void > rte_mempool_ops_free(struct rte_mempool *mp) > { > struct rte_mempool_ops *ops; >=20 > ops =3D rte_mempool_ops_get(mp->ops_index); > if (ops->free =3D=3D NULL) > return; > return ops->free(mp); > } >=20 Seems that the free cb expects mp->pool_data, but mp is passed. Another alternative to the "uint64_t or ptr" question would be to use a uintptr_t instead of a uint64_t. This is won't be possible if it need to be a 64 bits value even on 32 bits architectures. We could then keep the argument as pointer, and cast it to uintptr_t if needed. Regards, Olivier