> -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz at 6wind.com] > Sent: Tuesday, March 31, 2015 8:01 PM > To: Ananyev, Konstantin > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when > application uses private mbuf data > > Hi Konstantin, > > On 03/31/2015 01:17 AM, Ananyev, Konstantin wrote: > >>>> With this solution, there are 2 options: > >>>> - no mempool modification, so each application/driver has to add > >>>> priv_size bytes to the object to get the mbuf pointer. This does not > >>>> look feasible. > >>>> - change the mempool to support private area before each object. I > >>>> think mempool API is already quite complex, and I think it's not > >>>> the role of the mempool library to provide such features. > >>> > >>> > >>> In fact, I think the changes would be minimal. > >>> All we have to do, is to make changes in rte_pktmbuf_init(): > >>> > >>> void > >>> rte_pktmbuf_init(struct rte_mempool *mp, > >>> __attribute__((unused)) void *opaque_arg, > >>> void *_m, > >>> __attribute__((unused)) unsigned i) > >>> { > >>> > >>> //extract priv_size from mempool (discussed above). > >>> uint16_t priv_size = rte_mbufpool_get_priv_size(mp); > >>> > >>> struct rte_mbuf *m = _m + priv_size; > >>> uint32_t buf_len = mp->elt_size - sizeof(struct rte_mbuf) - > >>> priv_size; > >>> > >>> .... > >>> > >>> > >>> With that way priv_size inside mbuf would always be the size its own > >>> private space, > >>> for both direct and indirect mbus., so we don't require to set priv_size > >>> at attach/detach. > >>> Again RTE_MBUF_TO_BADDR() and RTE_MBUF_FROM_BADDR() would just work > >>> without any modifications. > >> > >> I'm not sure I'm getting it. The argument '_m' of your > >> rte_pktmbuf_init() is the pointer to the priv data, right? > >> So it means that the mbuf is located priv_size bytes after. > >> > >> The rte_pktmbuf_init() function is called by mempool_create(), > >> and the _m parameter is a pointer to a mempool object. So > >> in my understanding, mempool_get() would not return a rte_mbuf > >> but a pointer to the application private data. > > > > Ah my bad, forgot that mempool's obj_init() returns void now :( > > To make this approach work also need to change it, so it return a pointer > > to the new object. > > So, rte_pktmbuf_init() would return m and then in mempool_add_elem(): > > > > if (obj_init) > > obj = obj_init(mp, obj_init_arg, obj, obj_idx); > > > > rte_ring_sp_enqueue(mp->ring, obj); > > Yes, but modififying mempool is what I wanted to avoid for several > reasons. First, I think that changing the mempool_create() API here > (actually the obj_init prototype) would impact existing applications. > > Also, I'm afraid that storing a different address than the start > address of the object would have additional impacts. For instance, > some data is supposed to be stored before the object, see > __mempool_from_obj() or mempool_obj_audit().
mempool_obj_audit() should be ok, I think, but yes - rte_mempool_from_obj() would change the behaviour and can't be used by mempool_obj_audit() anymore. Ok, I am convinced - let's stick with private space between rte_mbuf and buf_adddr, as you suggested. > > Finally, I believe that mempool is not the proper place to do > modifications that are only needed for mbufs. If we really want > to implement mbuf private data in that way, maybe it would be > better to add a new layer above mempool (a mbuf pool layer). Not that I am against it, but seems like even more massive change - every application would need to be changed to use rte_mbufpool_create(), no? Konstantin > > > Regards > Olivier