From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id A1EB07E62 for ; Fri, 24 Oct 2014 17:49:52 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP; 24 Oct 2014 08:56:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,780,1406617200"; d="scan'208";a="595338633" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga001.jf.intel.com with ESMTP; 24 Oct 2014 08:58:20 -0700 Received: from irsmsx152.ger.corp.intel.com (163.33.192.66) by IRSMSX101.ger.corp.intel.com (163.33.3.153) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 24 Oct 2014 16:58:20 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.56]) by IRSMSX152.ger.corp.intel.com ([169.254.6.125]) with mapi id 14.03.0195.001; Fri, 24 Oct 2014 16:58:20 +0100 From: "Ananyev, Konstantin" To: "Richardson, Bruce" Thread-Topic: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep EXTERNAL_MBUF flag Thread-Index: AQHP72II3cEQXzazwEuf9kL3IfWzwJw/Dd9AgAAQcQCAADSqAIAAEg+Q Date: Fri, 24 Oct 2014 15:58:19 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772582139ECF1@IRSMSX105.ger.corp.intel.com> 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> In-Reply-To: <20141024154328.GC7648@BRICHA3-MOBL> 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 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: Fri, 24 Oct 2014 15:49:53 -0000 > -----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_M= BUF flag >=20 > 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 Changch= un > > > > 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 free= ing 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 updat= e whole ol_flags, not only RX related part. > > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits = 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 only= : > > > 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 ma= king > > 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 > > >=20 > 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. >=20 > 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 t= he > data pointer pointing internally, it's mbufs attached to other mbufs whic= h > are - so that's what we need to track using 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 b= e > an indirect one. > 3. The only place we would need to worry about such a flag is in the atta= ch, > detach and free mbuf functions - and on free we would simply need to repl= ace > 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: md =3D RTE_MBUF_FROM_BADDR(m->buf_addr); if (unlikely (md !=3D m)) { That will allow us to set buf_addr to some other valid offset inside mbuf and that fix an old problem with mbufs extra metadata (userdata) stored in = the packet's headroom.=20 Konstantin > Thoughts? > /Bruce