Hi Jan, On 10/6/2016 9:49 AM, Jan Viktorin wrote: > On Fri, 10 Jun 2016 09:29:44 +0200 > Olivier Matz <olivier.matz at 6wind.com> wrote: > >> Hi, >> >> On 06/09/2016 03:09 PM, Jan Viktorin wrote: >>>>> My suggestion is to have an additional flag, >>>>> 'MEMPOOL_F_PKT_ALLOC', which, if specified, would: >>>>> >>>>> ... #define MEMPOOL_F_SC_GET 0x0008 #define >>>>> MEMPOOL_F_PKT_ALLOC 0x0010 ... >>>>> >>>>> in rte_mempool_create_empty: ... after checking the other >>>>> MEMPOOL_F_* flags... >>>>> >>>>> if (flags & MEMPOOL_F_PKT_ALLOC) rte_mempool_set_ops_byname(mp, >>>>> RTE_MBUF_DEFAULT_MEMPOOL_OPS) >>>>> >>>>> And removing the redundant call to rte_mempool_set_ops_byname() >>>>> in rte_pktmbuf_create_pool(). >>>>> >>>>> Thereafter, rte_pktmbuf_pool_create can be changed to: >>>>> >>>>> ... mp = rte_mempool_create_empty(name, n, elt_size, cache_size, >>>>> - sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); >>>>> + sizeof(struct rte_pktmbuf_pool_private), socket_id, + >>>>> MEMPOOL_F_PKT_ALLOC); if (mp == NULL) return NULL; >>>> Yes, this would simplify somewhat the creation of a pktmbuf pool, >>>> in that it replaces the rte_mempool_set_ops_byname with a flag bit. >>>> However, I'm not sure we want to introduce a third method of >>>> creating a mempool to the developers. If we introduced this, we >>>> would then have: 1. rte_pktmbuf_pool_create() 2. >>>> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC set (which >>>> would use the configured custom handler) 3. >>>> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC __not__ set >>>> followed by a call to rte_mempool_set_ops_byname() (would allow >>>> several different custom handlers to be used in one application >>>> >>>> Does anyone else have an opinion on this? Oliver, Jerin, Jan? >>> I am quite careful about this topic as I don't feel to be very >>> involved in all the use cases. My opinion is that the _new API_ >>> should be able to cover all cases and the _old API_ should be >>> backwards compatible, however, built on top of the _new API_. >>> >>> I.e. I think, the flags MEMPOOL_F_SP_PUT, MEMPOOL_F_SC_GET (relicts >>> of the old API) should be accepted by the old API ONLY. The >>> rte_mempool_create_empty should not process them. >> The rte_mempool_create_empty() function already processes these flags >> (SC_GET, SP_PUT) as of today. > Yes, I consider it quite strange. When thinking more about the mempool API, > I'd move the flags processing to the rte_mempool_create. Semantically, it > makes more sense as the "empty" clearly describes that it is empty. But with > the flags, it is not... What is the reason to have those flags there?
Yes, they should be in rte_mempool_create. There were in an earlier patch, but regressed. I'll have them in rte_mempool_create in the next patch. [...] Rgds, Dave.
