From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7E6917EA8 for ; Sat, 25 Oct 2014 04:00:32 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 24 Oct 2014 19:08:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="405673871" Received: from pgsmsx101.gar.corp.intel.com ([10.221.44.78]) by FMSMGA003.fm.intel.com with ESMTP; 24 Oct 2014 19:00:55 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by PGSMSX101.gar.corp.intel.com (10.221.44.78) with Microsoft SMTP Server (TLS) id 14.3.195.1; Sat, 25 Oct 2014 10:08:43 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.156]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.44]) with mapi id 14.03.0195.001; Sat, 25 Oct 2014 10:08:42 +0800 From: "Ouyang, Changchun" To: "Ananyev, Konstantin" , "Richardson, Bruce" Thread-Topic: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag Thread-Index: AQHP72H1efKFInNDukWQrVCBFLLu5pw+iowAgAAeawCAADSrAIAABCaAgAEv9lA= Date: Sat, 25 Oct 2014 02:08:42 +0000 Message-ID: References: <1414138209-24431-1-git-send-email-changchun.ouyang@intel.com> <1414138209-24431-3-git-send-email-changchun.ouyang@intel.com> <2601191342CEEE43887BDE71AB9772582139EBAF@IRSMSX105.ger.corp.intel.com> <20141024123458.GA7648@BRICHA3-MOBL> <20141024154328.GC7648@BRICHA3-MOBL> <2601191342CEEE43887BDE71AB9772582139ECF1@IRSMSX105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772582139ECF1@IRSMSX105.ger.corp.intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag 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: Sat, 25 Oct 2014 02:00:33 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, > Konstantin > Sent: Friday, October 24, 2014 11:58 PM > To: Richardson, Bruce > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > EXTERNAL_MBUF flag >=20 >=20 >=20 > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Friday, October 24, 2014 4:43 PM > > To: Ananyev, Konstantin > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > > EXTERNAL_MBUF flag > > > > On Fri, Oct 24, 2014 at 01:34:58PM +0100, Bruce Richardson wrote: > > > On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote: > > > > Hi Changchun, > > > > > > > > > -----Original Message----- > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang > > > > > Changchun > > > > > Sent: Friday, October 24, 2014 9:10 AM > > > > > To: dev@dpdk.org > > > > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > > > > > EXTERNAL_MBUF flag > > > > > > > > > > Every pmd RX function need keep the EXTERNAL_MBUF flag in > > > > > mbuf.ol_flags, and can't overwrite it when filling ol_flags from > > > > > descriptor to mbuf, otherwise, it probably cause to crash when > > > > > freeing a mbuf and trying to freeing its attached external buffer= , say, > from guest space. > > > > > > > > > > > > > Don't really like the idea to put: > > > > mb->ol_flags =3D pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); > > > > in each and every PMD from now on... > > > > > > > > From other side, it is probably not very good that RX functions upd= ate > whole ol_flags, not only RX related part. > > > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bit= s for > TX and generic stuff. > > > > So our ol_flags will look something like that: > > > > > > > > union { > > > > uint64_t ol_raw_flags; > > > > struct { > > > > uint32_t rx; > > > > uint32_t gen_tx; > > > > } ol_flags > > > > }; > > > > > > > > And make all PMD RX functions to operate on rx part of the flags on= ly: > > > > mb->ol_flags.rx =3D pkt_flags; > > > > ? > > > > > > > > Konstantin > > > > > > > I would tend to agree with this. Changchun, did you get to assess > > > the performance impact of making this change to the PMDs? I suspect > > > that making the changes to each PMD would impact performance, while > > > Konstantin's suggestion should eliminate that impact. > > > The downside there is that we are limiting the flexibility we have > > > in expanding beyond 32 RX flags and 24 TX flags. :-( > > > > > > /Bruce > > > > > > > How about switching things about in terms of the flag. Instead of > > having to manage a flag across the baord to indicate if an mbuf is > > pointing to external memory, I think we should use the flag to > > indicate that an mbuf is attached to the memory space of another mbuf. > > > > My reasons for suggesting this are: > > 1. Mbufs pointing to externally managed memory are not really the > > problem to be dealt with on free, since they can be handled the same > > as mbufs with the data pointer pointing internally, it's mbufs > > attached to other mbufs which are - so that's what we need to track usi= ng a > flag. > > 2. Setting the flag to indicate an indirect mbuf should have no impact > > on the driver, as an mbuf that has just been allocated from mempool > > cannot be an indirect one. > > 3. The only place we would need to worry about such a flag is in the > > attach, detach and free mbuf functions - and on free we would simply > > need to replace the existing check for "md !=3D m" with a new check for > > the new flag. It would be a contained change. > > >=20 > Sounds good to me. > That's' definitely much better than my proposal. > Plus, if we'll stop to rely on: >=20 > md =3D RTE_MBUF_FROM_BADDR(m->buf_addr); > if (unlikely (md !=3D m)) { >=20 Currently seems good to me, too. But need more practice on it. > That will allow us to set buf_addr to some other valid offset inside mbu= f and > that fix an old problem with mbufs extra metadata (userdata) stored in th= e > packet's headroom. >=20 Not fully understand this. Konstantin, would you explain more? Thanks Changchun