Re: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support

["Hu, Jiayu" <jiayu.hu@intel.com>]

Hi Mark,

> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Thursday, September 14, 2017 4:52 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@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.ananyev@intel.com>
> >Cc: dev@dpdk.org; Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Tan,
> Jianfeng
> ><jianfeng.tan@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.hu@intel.com>; dev@dpdk.org
> >> > > > Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; Tan, Jianfeng
> ><jianfeng.tan@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