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

[Ananyev, Konstantin]

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?
Konstantin