From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH v3 1/5] mbuf: fix clone support when application uses private mbuf data Date: Wed, 08 Apr 2015 11:44:22 +0200 Message-ID: <5524F876.8090900@6wind.com> References: <1427385595-15011-1-git-send-email-olivier.matz@6wind.com> <1427829784-12323-1-git-send-email-zer0@droids-corp.org> <1427829784-12323-2-git-send-email-zer0@droids-corp.org> <2601191342CEEE43887BDE71AB97725821413A2D@irsmsx105.ger.corp.intel.com> <5522FF6B.1030503@6wind.com> <2601191342CEEE43887BDE71AB97725821414310@irsmsx105.ger.corp.intel.com> <5523FB9B.2060508@6wind.com> <2601191342CEEE43887BDE71AB9772582141451F@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit To: "Ananyev, Konstantin" , "dev-VfR2kkLFssw@public.gmane.org" Return-path: In-Reply-To: <2601191342CEEE43887BDE71AB9772582141451F-pww93C2UFcwu0RiL9chJVbfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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. 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. 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? > > Konstantin