From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 6B59537A4 for ; Fri, 27 Mar 2015 01:24:32 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 26 Mar 2015 17:24:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,476,1422950400"; d="scan'208";a="704850428" Received: from irsmsx108.ger.corp.intel.com ([163.33.3.3]) by orsmga002.jf.intel.com with ESMTP; 26 Mar 2015 17:24:22 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.47]) by IRSMSX108.ger.corp.intel.com ([163.33.3.3]) with mapi id 14.03.0224.002; Fri, 27 Mar 2015 00:24:22 +0000 From: "Ananyev, Konstantin" To: Olivier Matz , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when application uses private mbuf data Thread-Index: AQHQZ94CFsrfyU7g3Uqqq56hnHKrXZ0vakUw Date: Fri, 27 Mar 2015 00:24:21 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725821407D4D@irsmsx105.ger.corp.intel.com> References: <1427302838-8285-1-git-send-email-olivier.matz@6wind.com> <1427385595-15011-1-git-send-email-olivier.matz@6wind.com> <1427385595-15011-2-git-send-email-olivier.matz@6wind.com> In-Reply-To: <1427385595-15011-2-git-send-email-olivier.matz@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when application uses private mbuf data X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Mar 2015 00:24:33 -0000 Hi Olivier, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz > Sent: Thursday, March 26, 2015 4:00 PM > To: dev@dpdk.org > Cc: zoltan.kiss@linaro.org > Subject: [dpdk-dev] [PATCH v2 1/5] mbuf: fix clone support when applicati= on uses private mbuf data >=20 > Add a new private_size field in mbuf structure that should > be initialized at mbuf pool creation. This field contains the > size of the application private data in mbufs. >=20 > Introduce new static inline functions rte_mbuf_from_indirect() > and rte_mbuf_to_baddr() to replace the existing macros, which > take the private size in account when attaching and detaching > mbufs. >=20 > Signed-off-by: Olivier Matz > --- > app/test-pmd/testpmd.c | 1 + > examples/vhost/main.c | 2 +- > lib/librte_mbuf/rte_mbuf.c | 1 + > lib/librte_mbuf/rte_mbuf.h | 44 ++++++++++++++++++++++++++++++++++------= ---- > 4 files changed, 37 insertions(+), 11 deletions(-) >=20 > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 3057791..c5a195a 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -425,6 +425,7 @@ testpmd_mbuf_ctor(struct rte_mempool *mp, > mb->tx_offload =3D 0; > mb->vlan_tci =3D 0; > mb->hash.rss =3D 0; > + mb->priv_size =3D 0; > } >=20 > static void > diff --git a/examples/vhost/main.c b/examples/vhost/main.c > index c3fcb80..d542461 100644 > --- a/examples/vhost/main.c > +++ b/examples/vhost/main.c > @@ -139,7 +139,7 @@ > /* Number of descriptors per cacheline. */ > #define DESC_PER_CACHELINE (RTE_CACHE_LINE_SIZE / sizeof(struct vring_de= sc)) >=20 > -#define MBUF_EXT_MEM(mb) (RTE_MBUF_FROM_BADDR((mb)->buf_addr) !=3D (mb= )) > +#define MBUF_EXT_MEM(mb) (rte_mbuf_from_indirect(mb) !=3D (mb)) >=20 > /* mask of enabled ports */ > static uint32_t enabled_port_mask =3D 0; > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c > index 526b18d..e095999 100644 > --- a/lib/librte_mbuf/rte_mbuf.c > +++ b/lib/librte_mbuf/rte_mbuf.c > @@ -125,6 +125,7 @@ rte_pktmbuf_init(struct rte_mempool *mp, > m->pool =3D mp; > m->nb_segs =3D 1; > m->port =3D 0xff; > + m->priv_size =3D 0; > } >=20 > /* do some sanity checks on a mbuf: panic if it fails */ > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index 17ba791..45ac948 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -317,18 +317,42 @@ struct rte_mbuf { > /* uint64_t unused:8; */ > }; > }; > + > + uint16_t priv_size; /**< size of the application private data */ > } __rte_cache_aligned; >=20 > /** > - * Given the buf_addr returns the pointer to corresponding mbuf. > + * Return the mbuf owning the data buffer address of an indirect mbuf. > + * > + * @param mi > + * The pointer to the indirect mbuf. > + * @return > + * The address of the direct mbuf corresponding to buffer_addr. > */ > -#define RTE_MBUF_FROM_BADDR(ba) (((struct rte_mbuf *)(ba)) - 1) > +static inline struct rte_mbuf * > +rte_mbuf_from_indirect(struct rte_mbuf *mi) > +{ > + struct rte_mbuf *md; > + md =3D (struct rte_mbuf *)((char *)mi->buf_addr - sizeof(*mi) - > + mi->priv_size); > + return md; > +} >=20 > /** > - * Given the pointer to mbuf returns an address where it's buf_addr > - * should point to. > + * Return the buffer address embedded in the given mbuf. > + * > + * @param md > + * The pointer to the mbuf. > + * @return > + * The address of the data buffer owned by the mbuf. > */ > -#define RTE_MBUF_TO_BADDR(mb) (((struct rte_mbuf *)(mb)) + 1) > +static inline char * > +rte_mbuf_to_baddr(struct rte_mbuf *md) > +{ > + char *buffer_addr; > + buffer_addr =3D (char *)md + sizeof(*md) + md->priv_size; > + return buffer_addr; > +} >=20 I am a bit puzzled here, so for indirect mbuf, what value priv_size should = hold? Is that priv_size of indirect mfuf, or priv_size of direct mbuf, that mbuf = is attached to? If it is first one, then your changes in __rte_pktmbuf_prefree_seg() wouldn= 't work properly, If second one then changes in rte_pktmbuf_detach() looks wrong to me. Unless, of course priv_size for all mbufs in the system should always be th= e same value. But I suppose, that's not what your intention was, otherwise we don't need = priv_size inside mbuf at all - just a new macro in config file seems enough, plus it would be easier and f= aster. I think that to support ability to setup priv_size on a mempool basis, and reserve private space between struct rte_mbuf and rte_mbuf. buf_addr, we need to:=20 1. Store priv_size both inside the mempool and inside the mbuf. 2. rte_pktmbuf_attach() should change the value of priv_size to the priv_si= ze of the direct mbuf we are going to attach to: rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *md) {... mi->priv_= size =3D md->priv_size; ...} 3. rte_pktmbuf_detach() should restore original value of mbuf's priv_size: rte_pktmbuf_detach(struct rte_mbuf *m)=20 { ... m->priv_size =3D rte_mempool_get_privsize(m->pool); m->buf _addr=3D rte_mbuf_to_baddr(m); ... } Also I think we need to provide a way to specify priv_size for all mbufs of= the mepool at init time: - either force people to specify it at rte_mempool_create() time (probably = use init_arg for that), - or provide separate function that could be called straight after rte_memp= ool_create() , that would setup priv_size for the pool and for all its mbufs. - or some sort of combination of these 2 approaches - introduce a wrapper f= unction=20 (rte_mbuf_pool_create() or something) that would take priv_size as paramete= r,=20 create a new mempool and then setup priv_size. Though, I still think that the better approach is to reserve private space = before struct rte_mbuf, not after. In that case, user got his private space, and we keep buf_addr straight aft= er rte_mbuf, without any whole. So we don't need steps 2 and 3, above, plus we don't need rte_mbuf_to_baddr() and rte_mbuf_from_indirect() -=20 RTE_MBUF_TO_BADDR/ RTE_MBUF_FROM_BADDR would keep working correctly. In fact, with this scheme - we don't even need priv_size for mbuf managemen= t (attach/detach/free). Wonder did you try that approach? Thanks Konstantin > /** > * Returns TRUE if given mbuf is indirect, or FALSE otherwise. > @@ -744,12 +768,12 @@ static inline void rte_pktmbuf_attach(struct rte_mb= uf *mi, struct rte_mbuf *md) > static inline void rte_pktmbuf_detach(struct rte_mbuf *m) > { > const struct rte_mempool *mp =3D m->pool; > - void *buf =3D RTE_MBUF_TO_BADDR(m); > - uint32_t buf_len =3D mp->elt_size - sizeof(*m); > - m->buf_physaddr =3D rte_mempool_virt2phy(mp, m) + sizeof (*m); > + void *buf =3D rte_mbuf_to_baddr(m); > + unsigned mhdr_size =3D (char *)buf - (char *)m; >=20 > + m->buf_physaddr =3D rte_mempool_virt2phy(mp, m) + mhdr_size; > m->buf_addr =3D buf; > - m->buf_len =3D (uint16_t)buf_len; > + m->buf_len =3D (uint16_t)(mp->elt_size - mhdr_size); >=20 > m->data_off =3D (RTE_PKTMBUF_HEADROOM <=3D m->buf_len) ? > RTE_PKTMBUF_HEADROOM : m->buf_len; > @@ -774,7 +798,7 @@ __rte_pktmbuf_prefree_seg(struct rte_mbuf *m) > * - free attached mbuf segment > */ > if (RTE_MBUF_INDIRECT(m)) { > - struct rte_mbuf *md =3D RTE_MBUF_FROM_BADDR(m->buf_addr); > + struct rte_mbuf *md =3D rte_mbuf_from_indirect(m); > rte_pktmbuf_detach(m); > if (rte_mbuf_refcnt_update(md, -1) =3D=3D 0) > __rte_mbuf_raw_free(md); > -- > 2.1.4