From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 53E30A00E6 for ; Thu, 8 Aug 2019 18:26:56 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 425701BE87; Thu, 8 Aug 2019 18:26:55 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 3AE8A1BE7D for ; Thu, 8 Aug 2019 18:26:52 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Aug 2019 09:26:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,362,1559545200"; d="scan'208";a="199102725" Received: from irsmsx108.ger.corp.intel.com ([163.33.3.3]) by fmsmga004.fm.intel.com with ESMTP; 08 Aug 2019 09:26:49 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.164]) by IRSMSX108.ger.corp.intel.com ([169.254.11.229]) with mapi id 14.03.0439.000; Thu, 8 Aug 2019 17:26:49 +0100 From: "Ananyev, Konstantin" To: Matan Azrad , "dev@dpdk.org" CC: Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko , Olivier Matz Thread-Topic: [PATCH 2/2] doc: announce new mbuf field for LRO Thread-Index: AQHVTGc16KeOU/cz2kOGn4+V+To/WqbuRh+ggAAf2gCAAQTTYIAAJLwAgAAiQMCAAUl1AIAAFYfA///7BACAAGFK8A== Date: Thu, 8 Aug 2019 16:26:48 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580168A63C61@irsmsx105.ger.corp.intel.com> References: <1565103383-23864-1-git-send-email-matan@mellanox.com> <1565103383-23864-2-git-send-email-matan@mellanox.com> <2601191342CEEE43887BDE71AB9772580168A62AC0@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580168A630E5@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580168A6325D@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772580168A63A39@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTEzNTZkMjAtMDY2OS00NGNkLTgzYWUtZjE3ZmQzN2UxODAyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWnBTN2w5dHUxK2FpTTE1REpLeVUrMDdlWjh0SVEzbXFXWEFXOWY2RTJJM1VSYmVTZGU3SnF5elMzRmJUZ1dSRiJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action 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 2/2] doc: announce new mbuf field for LRO 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > Hi > From: Ananyev, Konstantin > > Hi Matan, > > > > > > > > > > > > > > > > > > > > The API breakage is because the ``tso_segsz`` field was > > > > > > > > > documented for LRO. > > > > > > > > > > > > > > > > > > The ``tso_segsz`` field in mbuf indicates the size of eac= h > > > > > > > > > segment in the LRO packet in Rx path and should be > > > > > > > > > provided by the LRO packet port. > > > > > > > > > > > > > > > > > > While the generic LRO packet may aggregate different > > > > > > > > > segments sizes in one packet, it is impossible to expose > > > > > > > > > this information for each segment by one field and it > > > > > > > > > doesn't make sense to expose all the segments sizes in th= e > > mbuf. > > > > > > > > > > > > > > > > > > A new field may be added as union with the above field to > > > > > > > > > expose the number of segments aggregated in the LRO packe= t. > > > > > > > > > > > > > > > > > > Signed-off-by: Matan Azrad > > > > > > > > > --- > > > > > > > > > doc/guides/rel_notes/deprecation.rst | 4 ++++ > > > > > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/doc/guides/rel_notes/deprecation.rst > > > > > > > > > b/doc/guides/rel_notes/deprecation.rst > > > > > > > > > index c0cd9bc..e826b69 100644 > > > > > > > > > --- a/doc/guides/rel_notes/deprecation.rst > > > > > > > > > +++ b/doc/guides/rel_notes/deprecation.rst > > > > > > > > > @@ -45,6 +45,10 @@ Deprecation Notices > > > > > > > > > - ``eal_parse_pci_DomBDF`` replaced by ``rte_pci_addr_= parse`` > > > > > > > > > - ``rte_eal_compare_pci_addr`` replaced by > > > > > > > > > ``rte_pci_addr_cmp`` > > > > > > > > > > > > > > > > > > +* mbuf: Remove ``tso_segsz`` mbuf field providing for LR= O > > > > support. > > > > > > > > > +Use union > > > > > > > > > + block for the field memory to be shared with a new > > > > > > > > > +field ``lro_segs_n`` > > > > > > > > > + indicates the number of segments aggregated in the LRO > > packet. > > > > > > > > > + > > > > > > > > > > > > > > > > Wonder how the upper layer will use that information (excep= t > > > > > > > > for > > > > stats)? > > > > > > > > Could you guys provide any examples? > > > > > > > > > > > > > > 1. Stats, allow to calc accurate PPS. > > > > > > > 2. Supply accurate information unlike the seg size which > > > > > > > cannot be > > > > > > accurate. > > > > > > > 2. Let the user all the information (segs num allow an averag= e > > > > > > > seg size calculation) > > > > > > > > > > > > So just for stats, right? > > > > > > > > > > Stats it is one option. > > > > > > > > > > The user configured LRO, means he wants X > 1 packets to be > > > > > aggregated > > > > by the port. > > > > > > > > > > Don't you think X is interesting for the user? > > > > > > > > > > For example, maybe there is Y for the next calculation: > > > > > > > > > > If average(X) < Y: > > > > > Stop LRO - not very good for performance to aggregate small > > > > > number > > > > of packets - stop LRO. > > > > > > > > > > > > > Might be, but I think user can use other metrics (let say average > > > > aggregated packet size) for that purpose. > > > > > > Yes, but I think it is better to supply the segs number which is an a= ccurate > > number instead of average size of segment. > > > Then, user can decide any calculation he prefers. > > > > > > > > > > > > > > > > > > > > > > > If so, wouldn't it be more plausible to extend PMD itself to > > > > > > provide some extra statistics? > > > > > > Just a thought. > > > > > > > > > > Yes, may be interesting but it can be redundant work when the use= r > > > > > don't > > > > need it. > > > > > > > > > > > > > > > > > > > > > > > > > > Also what PMD should do if HW does supports LRO, but doesn'= t > > > > > > > > to information? > > > > > > > > > > > > > > If the PMD knows all the segments size he can calculate it, n= o? > > > > > > > 0 means PMD doesn't support it. > > > > > > > > > > > > I mean HW/PMD might support LRO, but doesn't provide informatio= n > > > > > > about number of coalesced segments. > > > > > > What PMD should do in that case? > > > > > > > > > > As I said, to set this field with 0 and set the PKT_RX_LRO flag i= n ol_flags. > > > > > 0 in this case means support LRO but cannot supply the segments n= um. > > > > > > > > Ok..., but then what for then to set PKT_RX_LRO at all? > > > > From PMD perspective it would be easier not to set that flag at all > > > > and not to touch tso_segsz. > > > > > > The user should know that LRO is working. LRO flag should be set in a= ny > > case. > > > > Well, then I think you trying to introduce a new requirement for PMD. >=20 > Yes, this is the patch purpose. >=20 > > Right now, as I can see it is optional, and supposed to be set only whe= n PMD > > RX path updates tso_segsz. > > > > /** > > * When packets are coalesced by a hardware or virtual driver, this fla= g > > * can be set in the RX mbuf, meaning that the m->tso_segsz field is > > * valid and is set to the segment size of original packets. > > */ > > #define PKT_RX_LRO (1ULL << 16) >=20 > Yes, need to be changed to: > /** > * When packets are coalesced by a hardware or virtual driver, this flag > * is set in the RX mbuf. > */ >=20 >=20 > > > > > > > > > > > > > Do you familiar with PMDs that supports LRO but cannot provide th= e > > > > segments num? > > > > > If so, what do these PMDs can provide instead? > > > > > > > > Yes, ixgbe PMD. > > > > It does support TCP_LRO offload, and when enabled, does coalesce th= e > > > > packets, but doesn't set PKT_RX_LRO and doesn't touch tso_segsz. > > > > > > I think it should be changed to set the flag. > > > > > > > > > > > > > > > > > > Still set DEV_RX_OFFLOAD_TCP_LRO as enabled RX offload, but > > > > > > don't set PKT_RX_LRO flag in the RX-ed mbuf, even if it does > > > > > > contain coalesced packets? > > > > > > > > > > No, read above. > > > > > > > > > > > As I understand that what happens now. > > > > > > It is probably ok by me (as means no changes in ixgbe PMD)... > > > > > > But wouldn't that mean no defined way for the user to determine > > > > > > will HW/PMD provide that information or not? > > > > > > > > > > Will compare to 0, see above. > > > > > > > > I mean how the user will determine in advance would given PMD/HW > > > > provide that info in tso_segsz or not? > > > > Wait for the first LRO packet? Something else? > > > > > > Or wait to for the first LRO packet, or we can add a new ethdev capab= ility > > for it. > > > > > > What do you think? > > > > I still in doubt is it really worth to support that feature at all... > > Though if we'll decide to add it, then I think it needs to be a new cap= ability > > DEV_RX_OFFLOAD_LRO_STAT (or so) and probably new mbuf.ol_flag value > > for it. >=20 > I don't think that new mbuf ol_flag should be introduced. Only capability= . >=20 > We discussed on 2 topics: >=20 > 1. Changing the LRO field meaning in mbuf. segs size =3D> segs num. > 2. Whether to set the PKT_RX_LRO when the PMD cannot supply the field but= can coalesce packets or not. >=20 > If we do only 1, the flag setting method can be stay as is. >=20 > Can you agree with 1? this is the main reason for this patch. Ok, NP with #1. >=20 > I think it is problematic that ixgbe PMD doesn't allow to the user to kno= w whether the packet is coalesced or not. >=20 > IMO setting the PKT_RX_LRO flag and 0 in the LRO field is better. >=20