From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 8AE142B84 for ; Fri, 3 Jun 2016 14:11:02 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 03 Jun 2016 05:11:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,411,1459839600"; d="scan'208";a="990349588" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.24]) ([10.237.221.24]) by orsmga002.jf.intel.com with ESMTP; 03 Jun 2016 05:11:00 -0700 To: Olivier MATZ , Jerin Jacob 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> Cc: dev@dpdk.org, viktorin@rehivetech.com From: "Hunt, David" Message-ID: <575173D3.1070605@intel.com> Date: Fri, 3 Jun 2016 13:10:59 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <575164FF.1080509@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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:11:03 -0000 On 6/3/2016 12:07 PM, Olivier MATZ wrote: > > 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 = 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. >> 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. I would tend to agree. The uint64 didn't sit well with me :) I would prefer the rte_mempool* > By the way, I found a strange thing: > >> typedef void (*rte_mempool_free_t)(void *p); Yes, I spotted that earler, will be fixed in next version > [...] > >> void >> rte_mempool_ops_free(struct rte_mempool *mp) >> { >> struct rte_mempool_ops *ops; >> >> ops = rte_mempool_ops_get(mp->ops_index); >> if (ops->free == NULL) >> return; >> return ops->free(mp); >> } >> > Seems that the free cb expects mp->pool_data, but mp is passed. Working in it. > > > 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. I had assumed that the requirement was for 64 bits even on 32 bit OS's. I've implemented the double cast of the u64 to uintptr_t to struct pointer done to avoid compiler warnings on 32-bit but I really prefer the solution of passing the rte_mempool pointer instead. I'll change to *mp. Regards, Dave.