[dpdk-dev] [PATCH v3 2/5] gso: add TCP/IPv4 GSO support

[Hu, Jiayu]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, September 15, 2017 4:17 PM
> To: Hu, Jiayu <jiayu.hu at intel.com>; Kavanagh, Mark B
> <mark.b.kavanagh at intel.com>
> Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>
> Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support
> 
> 
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Friday, September 15, 2017 9:16 AM
> > To: Hu, Jiayu <jiayu.hu at intel.com>; Kavanagh, Mark B
> <mark.b.kavanagh at intel.com>
> > Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>
> > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support
> >
> > Hi Jiayu,
> >
> > > -----Original Message-----
> > > From: Hu, Jiayu
> > > Sent: Friday, September 15, 2017 8:55 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Kavanagh,
> Mark B <mark.b.kavanagh at intel.com>
> > > Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>
> > > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support
> > >
> > > Hi Konstantin,
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Friday, September 15, 2017 2:39 AM
> > > > To: Kavanagh, Mark B <mark.b.kavanagh at intel.com>; Hu, Jiayu
> > > > <jiayu.hu at intel.com>
> > > > Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>
> > > > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Kavanagh, Mark B
> > > > > Sent: Thursday, September 14, 2017 4:42 PM
> > > > > To: Hu, Jiayu <jiayu.hu at intel.com>; Ananyev, Konstantin
> > > > <konstantin.ananyev at intel.com>
> > > > > Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>
> > > > > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support
> > > > >
> > > > > >From: Hu, Jiayu
> > > > > >Sent: Thursday, September 14, 2017 11:01 AM
> > > > > >To: Ananyev, Konstantin <konstantin.ananyev at intel.com>;
> Kavanagh,
> > > > Mark B
> > > > > ><mark.b.kavanagh at intel.com>
> > > > > >Cc: dev at dpdk.org; Tan, Jianfeng <jianfeng.tan at intel.com>
> > > > > >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 <jiayu.hu at intel.com>
> > > > > >> Cc: dev at dpdk.org; Kavanagh, Mark B
> <mark.b.kavanagh at intel.com>;
> > > > Tan,
> > > > > >> Jianfeng <jianfeng.tan at intel.com>
> > > > > >> 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 <konstantin.ananyev at intel.com>
> > > > > >> > Cc: dev at dpdk.org; Kavanagh, Mark B
> <mark.b.kavanagh at intel.com>;
> > > > Tan,
> > > > > >> Jianfeng <jianfeng.tan at intel.com>
> > > > > >> > 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 <jiayu.hu at intel.com>
> > > > > >> > > Cc: dev at dpdk.org; Kavanagh, Mark B
> > > > <mark.b.kavanagh at intel.com>;
> > > > > >> Tan,
> > > > > >> > > Jianfeng <jianfeng.tan at intel.com>
> > > > > >> > > 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 <konstantin.ananyev at intel.com>
> > > > > >> > > > Cc: dev at dpdk.org; Kavanagh, Mark B
> > > > <mark.b.kavanagh at intel.com>;
> > > > > >> Tan,
> > > > > >> > > Jianfeng <jianfeng.tan at 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 at intel.com>; dev at dpdk.org
> > > > > >> > > > > > > > Cc: Kavanagh, Mark B <mark.b.kavanagh at intel.com>;
> Tan,
> > > > > >> Jianfeng
> > > > > >> > > <jianfeng.tan at 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?
> > > > > >> > >
> > > > > >> > > 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_*_TSO,
> > > > > >or new macros?
> > > > >
> > > > > Hi Jiayu, Konstantin,
> > > > >
> > > > > I think that the existing macros are fine, as they provide a consistent
> view
> > > > of segmentation capabilities to the application/user.
> > > >
> > > > +1
> > > > I also think it is better to re-use DEV_TX_OFFLOAD_*_TSO.
> > >
> > > There might be an 'issue', if we use 'PKT_TX_TCP_SEG' to tell the
> > > GSO library to segment a packet or not. Given the scenario that
> > > an application only wants to do GSO and doesn't want to use TSO.
> > > The application sets 'mbuf->ol_flags=PKT_TX_TCP_SEG' and doesn't
> > > set mbuf->tso_segsz. Then the GSO library segments the packet, and
> > > all output GSO segments have the same ol_flags as the input packet
> > > (in current GSO library design). Then the output GSO segments are
> > > transmitted to rte_eth_tx_prepare(). If the NIC is i40e, its TX prepare
> function,
> > > i40e_prep_pkts, checks if mbuf->tso_segsz is in the range of
> I40E_MIN_TSO_MSS
> > > and I40E_MAX_TSO_MSS, when PKT_TX_TCP_SEG is set. So an error
> happens in
> > > this scenario, since tso_segsz is 0.
> > >
> > > In fact, it may confuse the PMD driver when set PKT_TX_TCP_SEG but
> don't want
> > > to do TSO. One solution is that the GSO library removes the
> PKT_TX_TCP_SEG flag
> > > for all GSO segments after finishes segmenting.
> >
> > Yes, that was my thought too: after successful segmentation we probably
> > need to cleanup related ol_flags.
> 
> In fact, we just don't need to set these flags in our newly created segments.

+1. PKT_TX_TCP_SEG is not needed, but others, like PKT_TX_IPV4, should be
kept, since they may also be used by other HW offloadings, like csum.

Thanks,
Jiayu
> 
> > Konstantin
> >
> > > Wonder you and Mark's opinion.
> > >
> > > Thanks,
> > > Jiayu
> > > >
> > > > >
> > > > > 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
> > > > > previous 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,
> 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