Hi Mark, > -----Original Message----- > From: Kavanagh, Mark B > Sent: Thursday, September 14, 2017 4:52 PM > To: Hu, Jiayu <jiayu...@intel.com>; Ananyev, Konstantin > <konstantin.anan...@intel.com> > Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng....@intel.com> > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support > > >From: Hu, Jiayu > >Sent: Thursday, September 14, 2017 7:07 AM > >To: Ananyev, Konstantin <konstantin.anan...@intel.com> > >Cc: dev@dpdk.org; Kavanagh, Mark B <mark.b.kavan...@intel.com>; Tan, > Jianfeng > ><jianfeng....@intel.com> > >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 <jiayu...@intel.com>; dev@dpdk.org > >> > > > Cc: Kavanagh, Mark B <mark.b.kavan...@intel.com>; Tan, Jianfeng > ><jianfeng....@intel.com> > >> > > > 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 <errno.h> > >> > > > > > >> > > > > +#include <rte_log.h> > >> > > > > + > >> > > > > #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 = 1; > >> > > > > + > >> > > > > if (pkt == NULL || pkts_out == NULL || nb_pkts_out < 1) > >> > > > > return -EINVAL; > >> > > > > > >> > > > > - pkts_out[0] = pkt; > >> > > > > + if (gso_ctx.gso_size >= pkt->pkt_len || > >> > > > > + (pkt->packet_type & gso_ctx.gso_types) != > >> > > > > + pkt->packet_type) { > >> > > > > + pkts_out[0] = pkt; > >> > > > > + return ret; > >> > > > > + } > >> > > > > + > >> > > > > + direct_pool = gso_ctx.direct_pool; > >> > > > > + indirect_pool = gso_ctx.indirect_pool; > >> > > > > + gso_size = gso_ctx.gso_size; > >> > > > > + ipid_delta = gso_ctx.ipid_flag == 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) != 0) {... > >> > > > >> > > Sorry, actually it probably should be: > >> > > If (pkt->ol_flags & (PKT_TX_TCP_SEG | PKT_TX_IPV4) == PKT_TX_IPV4 > && > >> > > (gso_ctx->gso_types & DEV_TX_OFFLOAD_TCP_TSO) != 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 capability before > >> > they call the GSO library. Do I misundertsand anything? > >> > > >> > Additionally, we don't need to check if the packet is a TCP/IPv4 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 would be > good > >> to use the same API here too. > >> Also with that approach, by setting ol_flags properly user can use 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 add 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 and > 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 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 > 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 = > >PKT_TX_IPV4' and > >'gso_types = 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, > > If I'm not mistaken, I think what Konstantin is suggesting is as follows: > > - 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 > macros, to provide a consistent view of segmentation capabilities across the > HW and SW implementations.
Yes, DEV_TX_OFFLOAD_*_TSO stored in gso_types are used to by applications to tell the GSO library what GSO types are required. The GSO library uses ol_flags to decide which segmentation function to use. Thanks, Jiayu > > - As part of segmentation, it's still a case of checking the packet type, but > then setting the appropriate ol_flags in the mbuf, which the GSO library can > use to segment the packet. > > Thanks, > Mark > > > > >Jiayu > >> Konstantin