Hi, On 04/01/2015 03:48 PM, 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?
Yes, indeed that would be a significant change for the applications, which is probably not what we want, except if we can keep backward compatibility. Maybe it's possible. That's something to keep in mind if I send a patch series that introduce a new rte_mbuf_pool_create() function. Regards, Olivier