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

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

Hi Konstantin,

On Tue, Sep 12, 2017 at 10:17:27PM +0800, Ananyev, Konstantin wrote:
> 
> 
> > -----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?

Thanks,
Jiayu
> 
> Konstantin
> 
> > 
> > > +		ret = gso_tcp4_segment(pkt, gso_size, ipid_delta,
> > > +				direct_pool, indirect_pool,
> > > +				pkts_out, nb_pkts_out);
> > > +	} else
> > > +		RTE_LOG(WARNING, GSO, "Unsupported packet type\n");
> > 
> > Shouldn't we do pkt_out[0] = pkt; here?
> > 
> > > +
> > > +	if (ret > 1) {
> > > +		pkt_seg = pkt;
> > > +		while (pkt_seg) {
> > > +			rte_mbuf_refcnt_update(pkt_seg, -1);
> > > +			pkt_seg = pkt_seg->next;
> > > +		}
> > > +	}
> > >
> > > -	return 1;
> > > +	return ret;
> > >  }
> > > --
> > > 2.7.4