Hi Olivier, > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz at 6wind.com] > Sent: Wednesday, April 08, 2015 10:44 AM > To: Ananyev, Konstantin; dev at dpdk.org > Cc: zoltan.kiss at linaro.org; Richardson, Bruce > Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses > private mbuf data > > Hi Konstantin, > > On 04/07/2015 07:17 PM, Ananyev, Konstantin wrote: > >> Just to be sure we're on the same line: > >> > >> - before the patch series > >> > >> - private area was working before that patch series if clones were not > >> used. To use a private are, the user had to provide another > >> function derived from pktmbuf_init() to change m->buf_addr and > >> m->buf_len. > >> - using both private area + clones was broken > >> > >> - after the patch series > >> > >> - private area is working with or without clone. But yo use it, > >> the user still has to provide another function to change > >> m->buf_addr, m->buf_len *and m->priv_size*. > >> > >> The series just fixes the fact that "clones + priv" was not working. > >> It does not address the problem that providing a new pktmbuf_init() > >> function is required to use privata area. To fix this, I think it > >> could require a API evolution that should be part of another series. > > > > I don't think we need new pktmbuf_init(). > > We just need to update it, so both pktmbuf_init() and detach() setup > > buf_addr, buf_len (and priv_size) to exactly the same values. > > If they don't do that, it means that you can't use attach/detach with > > mempools created with pktmbuf_init() any more. > > > > BTW, another thing that I just realised: > > examples/ipv4_multicast and examples/ip_fragmentation/ - > > both create a pool of mbufs with elem_size < 2K and don't populate > > mempool's private area - > > so mbp_priv->mbuf_data_room_size == 0, for them. > > > > So that code in detach(): > > > > + mbp_priv = rte_mempool_get_priv(mp); > > + m->priv_size = mp->elt_size - RTE_PKTMBUF_HEADROOM - > > + mbp_priv->mbuf_data_room_size - > > + sizeof(struct rte_mbuf); > > > > > > Would break both these samples. > > I suppose we need to handle situation when mp->elt_size < > > RTE_PKTMBUF_HEADROOM + sizeof(struct rte_mbuf), > > (and probably also when mbuf_data_room_size == 0) correctly. > > Indeed. I think a mbuf pool (even with buf_len == 0 like in > ip_fragmentation example) should have a pool with a private area and > should call rte_pktmbuf_pool_init() to populate it. So > rte_pktmbuf_pool_init() has to be fixed first to use elt_size > and support the buf_len < RTE_PKTMBUF_HEADROOM, then we can > update frag/multicast examples. > > Unfortunately, we don't know the size of the mbuf private area > in rte_pktmbuf_pool_init() if the opaque arg (data_room_size) > is 0, which is the default. I think it should be replaced by a structure > containing data_room_size and mbuf_priv_size, but it would break > applications that are setting data_room_size.
Yes, same thoughts here. > I don't see any good > solution to do that while keeping a backward compatibility for > rte_pktmbuf_pool_init(), but as the current API is not ideal, > I think it's worth changing it and add something in the release > note. If no one else has a better alternative than that, then I suppose it is good enough. > > We may also want to introduce a new helper as discussed previously: > > struct rte_mempool * > rte_pktmbuf_pool_create(const char *name, unsigned n, unsigned elt_size, > unsigned cache_size, size_t mbuf_priv_size, > rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg, > int socket_id, unsigned flags) > > Any comment? Looks good to me. Should we also introduce rte_pktmbuf_pool_xmem_create()? Konstantin > > > > > > Konstantin
