From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hu, Jiayu" Subject: Re: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support Date: Thu, 14 Sep 2017 10:01:03 +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: "Ananyev, Konstantin" , "Kavanagh, Mark B" Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 89BE73195 for ; Thu, 14 Sep 2017 12:01:14 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB9772584F24AE4D@irsmsx105.ger.corp.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" 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 >=20 >=20 >=20 > > -----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 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 pac= ket is > > > freed > > > > > > > > > automatically. > > > > > > > > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rt= e_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_IN= CREASE; > > > > > > > > > + > > > > > > > > > + 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 if= the > TSO > > > > > > flag is set or not. Applications can query device TSO capabilit= y > before > > > > > > they call the GSO library. Do I misundertsand anything? > > > > > > > > > > > > Additionally, we don't need to check if the packet is a TCP/IPv= 4 > 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 wo= uld > be > > > good > > > > > to use the same API here too. > > > > > Also with that approach, by setting ol_flags properly user can us= e 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 segment= ation > be > > > performed on that package or not. > > > > > The only advantage I see here is that if someone would like to ad= d > GSO > > > for 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 an= d > > > 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()) f= or > > > each input packet. > > > > Applications should parse the packet type, and set an exactly corre= ct > > > DEV_TX_OFFLOAD_*_TSO > > > > flag to gso_types and ol_flags according to the packet type. That i= s, the > > > value of gso_types > > > > is on a per-packet basis. Using gso_ctx->gso_types and mbuf->ol_fla= gs > at > > > the same time > > > > is because that DEV_TX_OFFLOAD_*_TSO only tells tunnelling type and > 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 type= 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. >=20 > 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_*_TSO= , or new macros? Jiayu > Konstantin >=20 > > > > The first choice makes HW and SW segmentation are totally the same. > > Applications just need to parse the packet and set proper ol_flags, and > > 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