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 15:42:01 +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> <2601191342CEEE43887BDE71AB9772584F24ADD2@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772584F24AE4D@irsmsx105.ger.corp.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 mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id C4474374E for ; Thu, 14 Sep 2017 17:42:04 +0200 (CEST) In-Reply-To: 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 11:01 AM >To: Ananyev, Konstantin ; Kavanagh, Mark B > >Cc: dev@dpdk.org; Tan, Jianfeng >Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support > >Hi Konstantin and Mark, > >> -----Original Message----- >> From: Ananyev, Konstantin >> Sent: Thursday, September 14, 2017 5:36 PM >> To: Hu, Jiayu >> Cc: dev@dpdk.org; Kavanagh, Mark B ; Tan, >> Jianfeng >> Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support >> >> >> >> > -----Original Message----- >> > From: Hu, Jiayu >> > Sent: Thursday, September 14, 2017 10:29 AM >> > To: Ananyev, Konstantin >> > Cc: dev@dpdk.org; Kavanagh, Mark B ; Tan, >> Jianfeng >> > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support >> > >> > Hi Konstantin, >> > >> > > -----Original Message----- >> > > From: Ananyev, Konstantin >> > > Sent: Thursday, September 14, 2017 4:47 PM >> > > To: Hu, Jiayu >> > > Cc: dev@dpdk.org; Kavanagh, Mark B ; >> Tan, >> > > Jianfeng >> > > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support >> > > >> > > Hi Jiayu, >> > > >> > > > -----Original Message----- >> > > > From: Hu, Jiayu >> > > > Sent: Thursday, September 14, 2017 7:07 AM >> > > > To: Ananyev, Konstantin >> > > > Cc: dev@dpdk.org; Kavanagh, Mark B ; >> Tan, >> > > Jianfeng >> > > > 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 wrot= e: >> > > > > >> > > > > 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 pa= cket >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_I= NCREASE; >> > > > > > > > > + >> > > > > > > > > + 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_IPV4 >> > > && >> > > > > > > (gso_ctx->gso_types & DEV_TX_OFFLOAD_TCP_TSO) !=3D 0) = {... >> > > > > > >> > > > > > I don't quite understand why the GSO library should be aware i= f >the >> TSO >> > > > > > flag is set or not. Applications can query device TSO capabili= ty >> before >> > > > > > they call the GSO library. Do I misundertsand anything? >> > > > > > >> > > > > > Additionally, we don't need to check if the packet is a TCP/IP= v4 >> packet >> > > 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 w= ould >> be >> > > good >> > > > > to use the same API here too. >> > > > > Also with that approach, by setting ol_flags properly user can u= se >the >> > > same 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 >> be >> > > performed on that package or not. >> > > > > The only advantage I see here is that if someone would like to a= dd >> GSO >> > > for some new protocol, >> > > > > he wouldn't need to introduce new TX flag value for mbuf.ol_flag= s. >> > > > > Though he still would need to update TX_OFFLOAD_* capabilities a= nd >> > > probably 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 >> > > specific 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 corr= ect >> > > 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_fl= ags >> at >> > > the same time >> > > > is because that DEV_TX_OFFLOAD_*_TSO only tells tunnelling type an= d >> the >> > > inner 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 typ= e >for >> > > tunneled packet. >> > > > How about the outer L3 type? Always assume the inner and the outer= L3 >> > > type are the same? >> > > >> > > It think that for that case you'll have to set in ol_flags: >> > > >> > > PKT_TX_IPV4 | PKT_TX_OUTER_IPV4 | PKT_TX_TUNNEL_VXLAN | >> > > PKT_TX_TCP_SEG >> > >> > OK, so it means PKT_TX_TCP_SEG is also used for tunneled TSO. The >> > GSO library doesn't need gso_types anymore. >> >> You still might need gso_ctx.gso_types to let user limit what types of >> segmentation >> that particular gso_ctx supports. >> An alternative would be to assume that each gso_ctx supports all >> currently implemented segmentations. >> This is possible too, but probably not very convenient to the user. > >Hmm, make sense. > >One thing to confirm: the value of gso_types should be DEV_TX_OFFLOAD_*_TS= O, >or new macros? Hi Jiayu, Konstantin, I think that the existing macros are fine, as they provide a consistent vie= w of segmentation capabilities to the application/user. I was initially concerned that they might be too coarse-grained (i.e. only = IPv4 is currently supported, and not IPv6), but as per Konstantin's previou= s example, the DEV_TX_OFFLOAD_*_TSO macros can be used in concert with the = packet type to determine whether a packet should be fragmented or not. Thanks, Mark > >Jiayu >> Konstantin >> >> > >> > The first choice makes HW and SW segmentation are totally the same. >> > Applications just need to parse the packet and set proper ol_flags, an= d >> > the GSO library uses ol_flags to decide which segmentation function to >use. >> > I think it's better than the second choice which depending on ptype to >> > choose segmentation function. >> > >> > Jiayu >> > > >> > > Konstantin >> > > >> > > > >> > > > Jiayu >> > > > > Konstantin