From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kavanagh, Mark B" Subject: Re: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support Date: Thu, 14 Sep 2017 08:51:56 +0000 Message-ID: References: <1504598270-60080-1-git-send-email-jiayu.hu@intel.com> <1505184211-36728-1-git-send-email-jiayu.hu@intel.com> <1505184211-36728-3-git-send-email-jiayu.hu@intel.com> <2601191342CEEE43887BDE71AB9772584F249FE8@irsmsx105.ger.corp.intel.com> <20170913104407.GA57844@dpdk15.sh.intel.com> <2601191342CEEE43887BDE71AB9772584F24AACB@irsmsx105.ger.corp.intel.com> <20170914060705.GA60858@dpdk15.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Tan, Jianfeng" To: "Hu, Jiayu" , "Ananyev, Konstantin" Return-path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id D99822C58 for ; Thu, 14 Sep 2017 10:51:59 +0200 (CEST) In-Reply-To: <20170914060705.GA60858@dpdk15.sh.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >From: Hu, Jiayu >Sent: Thursday, September 14, 2017 7:07 AM >To: Ananyev, Konstantin >Cc: dev@dpdk.org; Kavanagh, Mark B ; Tan, Jianf= eng > >Subject: Re: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support > >Hi Konstantin, > >On Thu, Sep 14, 2017 at 06:10:37AM +0800, Ananyev, Konstantin wrote: >> >> Hi Jiayu, >> >> > > >> > > >> > > > -----Original Message----- >> > > > From: Ananyev, Konstantin >> > > > Sent: Tuesday, September 12, 2017 12:18 PM >> > > > To: Hu, Jiayu ; dev@dpdk.org >> > > > Cc: Kavanagh, Mark B ; Tan, Jianfeng > >> > > > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support >> > > > >> > > > > result, when all of its GSOed segments are freed, the packet is >freed >> > > > > automatically. >> > > > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c >> > > > > index dda50ee..95f6ea6 100644 >> > > > > --- a/lib/librte_gso/rte_gso.c >> > > > > +++ b/lib/librte_gso/rte_gso.c >> > > > > @@ -33,18 +33,53 @@ >> > > > > >> > > > > #include >> > > > > >> > > > > +#include >> > > > > + >> > > > > #include "rte_gso.h" >> > > > > +#include "gso_common.h" >> > > > > +#include "gso_tcp4.h" >> > > > > >> > > > > int >> > > > > rte_gso_segment(struct rte_mbuf *pkt, >> > > > > - struct rte_gso_ctx gso_ctx __rte_unused, >> > > > > + struct rte_gso_ctx gso_ctx, >> > > > > struct rte_mbuf **pkts_out, >> > > > > uint16_t nb_pkts_out) >> > > > > { >> > > > > + struct rte_mempool *direct_pool, *indirect_pool; >> > > > > + struct rte_mbuf *pkt_seg; >> > > > > + uint16_t gso_size; >> > > > > + uint8_t ipid_delta; >> > > > > + int ret =3D 1; >> > > > > + >> > > > > if (pkt =3D=3D NULL || pkts_out =3D=3D NULL || nb_pkts_out < 1= ) >> > > > > return -EINVAL; >> > > > > >> > > > > - pkts_out[0] =3D pkt; >> > > > > + if (gso_ctx.gso_size >=3D pkt->pkt_len || >> > > > > + (pkt->packet_type & gso_ctx.gso_types) !=3D >> > > > > + pkt->packet_type) { >> > > > > + pkts_out[0] =3D pkt; >> > > > > + return ret; >> > > > > + } >> > > > > + >> > > > > + direct_pool =3D gso_ctx.direct_pool; >> > > > > + indirect_pool =3D gso_ctx.indirect_pool; >> > > > > + gso_size =3D gso_ctx.gso_size; >> > > > > + ipid_delta =3D gso_ctx.ipid_flag =3D=3D RTE_GSO_IPID_INCREASE; >> > > > > + >> > > > > + if (is_ipv4_tcp(pkt->packet_type)) { >> > > > >> > > > Probably we need here: >> > > > If (is_ipv4_tcp(pkt->packet_type) && (gso_ctx->gso_types & >DEV_TX_OFFLOAD_TCP_TSO) !=3D 0) {... >> > > >> > > Sorry, actually it probably should be: >> > > If (pkt->ol_flags & (PKT_TX_TCP_SEG | PKT_TX_IPV4) =3D=3D PKT_TX_IPV= 4 && >> > > (gso_ctx->gso_types & DEV_TX_OFFLOAD_TCP_TSO) !=3D 0) {... >> > >> > I don't quite understand why the GSO library should be aware if the TS= O >> > flag is set or not. Applications can query device TSO capability befor= e >> > they call the GSO library. Do I misundertsand anything? >> > >> > Additionally, we don't need to check if the packet is a TCP/IPv4 packe= t >here? >> >> Well, right now PMD we doesn't rely on ptype to figure out what type of >packet and >> what TX offload have to be performed. >> Instead it looks at TX part of ol_flags, and >> My thought was that as what we doing is actually TSO in SW, it would be = good >> to use the same API here too. >> Also with that approach, by setting ol_flags properly user can use the s= ame >gso_ctx and still >> specify what segmentation to perform on a per-packet basis. >> >> Alternative way is to rely on ptype to distinguish should segmentation b= e >performed on that package or not. >> The only advantage I see here is that if someone would like to add GSO f= or >some new protocol, >> he wouldn't need to introduce new TX flag value for mbuf.ol_flags. >> Though he still would need to update TX_OFFLOAD_* capabilities and proba= bly >packet_type definitions. >> >> So from my perspective first variant (use HW TSO API) is more plausible. >> Wonder what is your and Mark opinions here? > >In the first choice, you mean: >the GSO library uses gso_ctx->gso_types and mbuf->ol_flags to call a speci= fic >GSO >segmentation function (e.g. gso_tcp4_segment(), gso_tunnel_xxx()) for each >input packet. >Applications should parse the packet type, and set an exactly correct >DEV_TX_OFFLOAD_*_TSO >flag to gso_types and ol_flags according to the packet type. That is, the >value of gso_types >is on a per-packet basis. Using gso_ctx->gso_types and mbuf->ol_flags at t= he >same time >is because that DEV_TX_OFFLOAD_*_TSO only tells tunnelling type and the in= ner >L4 type, and >we need to know L3 type by ol_flags. With this design, HW segmentation and= SW >segmentation >are indeed consistent. > >If I understand it correctly, applications need to set 'ol_flags =3D >PKT_TX_IPV4' and >'gso_types =3D DEV_TX_OFFLOAD_VXLAN_TNL_TSO' for a >"ether+ipv4+udp+vxlan+ether+ipv4+ >tcp+payload" packet. But PKT_TX_IPV4 just present the inner L3 type for >tunneled packet. >How about the outer L3 type? Always assume the inner and the outer L3 type= are >the same? Hi Jiayu,=20 If I'm not mistaken, I think what Konstantin is suggesting is as follows:=20 - The DEV_TX_OFFLOAD_*_TSO flags are currently used to describe a NIC's TSO= capabilities; the GSO capabilities may also be described using the same ma= cros, to provide a consistent view of segmentation capabilities across the = HW and SW implementations. - As part of segmentation, it's still a case of checking the packet type, b= ut then setting the appropriate ol_flags in the mbuf, which the GSO library= can use to segment the packet. Thanks, Mark > >Jiayu >> Konstantin