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

[Kavanagh, Mark B]

>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.

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