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 5A96A1BB3E for ; Wed, 11 Apr 2018 02:25:36 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Apr 2018 17:25:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,434,1517904000"; d="scan'208";a="41056439" Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by FMSMGA003.fm.intel.com with ESMTP; 10 Apr 2018 17:25:33 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.164]) by IRSMSX109.ger.corp.intel.com ([169.254.13.170]) with mapi id 14.03.0319.002; Wed, 11 Apr 2018 01:25:31 +0100 From: "Ananyev, Konstantin" To: Yongseok Koh , Olivier Matz CC: "Lu, Wenzhuo" , "Wu, Jingjing" , "adrien.mazarguil@6wind.com" , "nelio.laranjeiro@6wind.com" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 1/6] mbuf: add buffer offset field for flexible indirection Thread-Index: AQHTyyWFk8vygk5Jwk2G86W8WINYuKPvrBIAgAjlxgCAAKYZgIABhjNg Date: Wed, 11 Apr 2018 00:25:31 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258AE91344A@IRSMSX102.ger.corp.intel.com> References: <20180310012532.15809-1-yskoh@mellanox.com> <20180402185008.13073-1-yskoh@mellanox.com> <20180402185008.13073-2-yskoh@mellanox.com> <20180403082615.etnr33cuyey7i3u3@platinum> <20180404001205.GB1867@yongseok-MBP.local> <20180409160434.kmw4iyztemrkzmtc@platinum> <20180410015902.GA20627@yongseok-MBP.local> In-Reply-To: <20180410015902.GA20627@yongseok-MBP.local> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjZmYjE3NjItNGEzZC00ZGYyLTk4ZTYtZjVjZDI5Zjk2YmM3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IkpadjR6OHhlNlI3V2p5ZHJrZzJQVG1YNTFCaWZvU0pjQUdVXC9acVFHYnNvPSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 1/6] mbuf: add buffer offset field for flexible indirection X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Apr 2018 00:25:37 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yongseok Koh > Sent: Tuesday, April 10, 2018 2:59 AM > To: Olivier Matz > Cc: Lu, Wenzhuo ; Wu, Jingjing ; adrien.mazarguil@6wind.com; > nelio.laranjeiro@6wind.com; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/6] mbuf: add buffer offset field for = flexible indirection >=20 > On Mon, Apr 09, 2018 at 06:04:34PM +0200, Olivier Matz wrote: > > Hi Yongseok, > > > > On Tue, Apr 03, 2018 at 05:12:06PM -0700, Yongseok Koh wrote: > > > On Tue, Apr 03, 2018 at 10:26:15AM +0200, Olivier Matz wrote: > > > > Hi, > > > > > > > > On Mon, Apr 02, 2018 at 11:50:03AM -0700, Yongseok Koh wrote: > > > > > When attaching a mbuf, indirect mbuf has to point to start of buf= fer of > > > > > direct mbuf. By adding buf_off field to rte_mbuf, this becomes mo= re > > > > > flexible. Indirect mbuf can point to any part of direct mbuf by c= alling > > > > > rte_pktmbuf_attach_at(). > > > > > > > > > > Possible use-cases could be: > > > > > - If a packet has multiple layers of encapsulation, multiple indi= rect > > > > > buffers can reference different layers of the encapsulated pack= et. > > > > > - A large direct mbuf can even contain multiple packets in series= and > > > > > each packet can be referenced by multiple mbuf indirections. > > > > > > > > > > Signed-off-by: Yongseok Koh > > > > > > > > I think the current API is already able to do what you want. > > > > > > > > 1/ Here is a mbuf m with its data > > > > > > > > off > > > > <--> > > > > len > > > > +----+ <----------> > > > > | | > > > > +-|----v----------------------+ > > > > | | -----------------------| > > > > m | buf | XXXXXXXXXXX || > > > > | -----------------------| > > > > +-----------------------------+ > > > > > > > > > > > > 2/ clone m: > > > > > > > > c =3D rte_pktmbuf_alloc(pool); > > > > rte_pktmbuf_attach(c, m); > > > > > > > > Note that c has its own offset and length fields. > > > > > > > > > > > > off > > > > <--> > > > > len > > > > +----+ <----------> > > > > | | > > > > +-|----v----------------------+ > > > > | | -----------------------| > > > > m | buf | XXXXXXXXXXX || > > > > | -----------------------| > > > > +------^----------------------+ > > > > | > > > > +----+ > > > > indirect | > > > > +-|---------------------------+ > > > > | | -----------------------| > > > > c | buf | || > > > > | -----------------------| > > > > +-----------------------------+ > > > > > > > > off len > > > > <--><----------> > > > > > > > > > > > > 3/ remove some data from c without changing m > > > > > > > > rte_pktmbuf_adj(c, 10) // at head > > > > rte_pktmbuf_trim(c, 10) // at tail > > > > > > > > > > > > Please let me know if it fits your needs. > > > > > > No, it doesn't. > > > > > > Trimming head and tail with the current APIs removes data and make th= e space > > > available. Adjusting packet head means giving more headroom, not shif= ting the > > > buffer itself. If m has two indirect mbufs (c1 and c2) and those are = pointing to > > > difference offsets in m, > > > > > > rte_pktmbuf_adj(c1, 10); > > > rte_pktmbuf_adj(c2, 20); > > > > > > then the owner of c2 regard the first (off+20)B as available headroom= . If it > > > wants to attach outer header, it will overwrite the headroom even tho= ugh the > > > owner of c1 is still accessing it. Instead, another mbuf (h1) for the= outer > > > header should be linked by h1->next =3D c2. > > > > Yes, after these operations c1, c2 and m should become read-only. So, t= o > > prepend headers, another mbuf has to be inserted before as you suggest.= It > > is possible to wrap this in a function rte_pktmbuf_clone_area(m, offset= , > > length) that will: > > - alloc and attach indirect mbuf for each segment of m that is > > in the range [offset : length+offset]. > > - prepend an empty and writable mbuf for the headers > > > > > If c1 and c2 are attached with shifting buffer address by adjusting b= uf_off, > > > which actually shrink the headroom, this case can be properly handled= . > > > > What do you mean by properly handled? > > > > Yes, prepending data or adding data in the indirect mbuf won't override > > the direct mbuf. But prepending data or adding data in the direct mbuf = m > > won't be protected. > > > > From an application point of view, indirect mbufs, or direct mbufs that > > have refcnt !=3D 1, should be both considered as read-only because they > > may share their data. How an application can know if the data is shared > > or not? > > > > Maybe we need a flag to differentiate mbufs that are read-only > > (something like SHARED_DATA, or simply READONLY). In your case, if my > > understanding is correct, you want to have indirect mbufs with RW data. >=20 > Agree that indirect mbuf must be treated as read-only, Then the current c= ode is > enough to handle that use-case. >=20 > > > And another use-case (this is my actual use-case) is to make a large = mbuf have > > > multiple packets in series. AFAIK, this will also be helpful for some= FPGA NICs > > > because it transfers multiple packets to a single large buffer to red= uce PCIe > > > overhead for small packet traffic like the Multi-Packet Rx of mlx5 do= es. > > > Otherwise, packets should be memcpy'd to regular mbufs one by one ins= tead of > > > indirect referencing. But just to make HW to RX multiple packets into one mbuf, data_off inside indirect mbuf should be enough, correct? As I understand, what you'd like to achieve with this new field - ability to manipulate packet boundaries after RX, probably at upper layer. As Olivier pointed above, that doesn't sound as safe approach - as you have= multiple indirect mbufs trying to modify same direct buffer. Though if you really need to do that, why it can be achieved by updating bu= f_len and priv_size Fields for indirect mbufs, straight after attach()? Konstantin > > > > > > Does this make sense? > > > > I understand the need. > > > > Another option would be to make the mbuf->buffer point to an external > > buffer (not inside the direct mbuf). This would require to add a > > mbuf->free_cb. See "Mbuf with external data buffer" (page 19) in [1] fo= r > > a quick overview. > > > > [1] > https://emea01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fdpdk= summit.com%2FArchive%2Fpdf%2F2016Userspace%2FDay01 > -Session05-OlivierMatz- > Userspace2016.pdf&data=3D02%7C01%7Cyskoh%40mellanox.com%7Ca5405edb36e445e= 6540808d59e339a38%7Ca652971c7d2e4d9ba6a4d > 149256f461b%7C0%7C0%7C636588866861082855&sdata=3Dllw%2BwiY5cC56naOUhBbIg8= TKtfFN6VZcIRY5PV7VqZs%3D&reserved=3D0 > > > > The advantage is that it does not require the large data to be inside a > > mbuf (requiring a mbuf structure before the buffer, and requiring to be > > allocated from a mempool). On the other hand, it is maybe more complex > > to implement compared to your solution. >=20 > I knew that you presented the slides and frankly, I had considered that o= ption > at first. But even with that option, metadata to store refcnt should also= be > allocated and managed anyway. Kernel also maintains the skb_shared_info a= t the > end of the data segment. Even though it could have smaller metadata struc= ture, > I just wanted to make full use of the existing framework because it is le= ss > complex as you mentioned. Given that you presented the idea of external d= ata > buffer in 2016 and there hasn't been many follow-up discussions/activitie= s so > far, I thought the demand isn't so big yet thus I wanted to make this pat= ch > simpler. I personally think that we can take the idea of external data s= eg when > more demands come from users in the future as it would be a huge change a= nd may > break current ABI/API. When the day comes, I'll gladly participate in the > discussions and write codes for it if I can be helpful. >=20 > Do you think this patch is okay for now? >=20 >=20 > Thanks for your comments, > Yongseok