Re: [dpdk-dev] [PATCH v2] i40: fix the VXLAN TSO issue

[Zhe Tao <zhe.tao@intel.com>]

On Thu, Jul 07, 2016 at 08:24:43PM +0800, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> > Sent: Thursday, July 07, 2016 11:51 AM
> > To: Tao, Zhe; dev@dpdk.org
> > Cc: Tao, Zhe; Wu, Jingjing
> > Subject: Re: [dpdk-dev] [PATCH v2] i40: fix the VXLAN TSO issue
> > 
> > 
> > > > @@ -215,6 +216,7 @@ parse_vxlan(struct udp_hdr *udp_hdr,
> > > >  		RTE_ETH_IS_TUNNEL_PKT(pkt_type) == 0)
> > > >  		return;
> > > >
> > > > +	*ol_flags |= PKT_TX_TUNNEL_VXLAN;
> 
> Do we always have to setup tunnelling flags here?
> Obviously it would mean an extra CTD per packet and might slowdown things.
this flag is for tunnelling type, and CTD is based on whether we need to do the
external ip offload and TSO.so this flag will not cause one extra CTD.
> In fact, I think current patch wouldn't work correctly if
> TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM is not set.
> So, can we do it only when TSO is enabled or outer IP checksum is enabled?
good comments and I aggree with you to let the APP to make sure all the flags driver needed
is set.
> 
> > > >  	info->is_tunnel = 1;
> > > >  	info->outer_ethertype = info->ethertype;
> > > >  	info->outer_l2_len = info->l2_len;
> > > > @@ -231,7 +233,9 @@ parse_vxlan(struct udp_hdr *udp_hdr,
> > > >
> > > > +				testpmd_ol_flags, ol_flags);
> > > >  		}
> > > >
> > > >  		/* step 4: fill the mbuf meta data (flags and header lengths) */
> > > > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > index 049a813..4c987f2 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -801,6 +801,12 @@ i40e_txd_enable_checksum(uint64_t ol_flags,
> > > >  			union i40e_tx_offload tx_offload,
> > > >  			uint32_t *cd_tunneling)
> > > >  {
> > > > +	/* Tx pkts tunnel type*/
> > > > +	if ((ol_flags & PKT_TX_TUNNEL_MASK) == PKT_TX_TUNNEL_VXLAN)
> > > > +		*cd_tunneling |= I40E_TXD_CTX_UDP_TUNNELING;
> > > > +	else if ((ol_flags & PKT_TX_TUNNEL_MASK) == PKT_TX_TUNNEL_GRE)
> > > > +		*cd_tunneling |= I40E_TXD_CTX_GRE_TUNNELING;
> > > > +
> > 
> > As I understand that fix is needed to enable TSO for tunnelling packets, correct?
> > For that case, should we setup EIPLEN also, no matter is PKT_TX_OUTER_IP_CKSUM
> > is on/off?
> 
> In fact, it seems we have always to setup all 3: EIPLEN, MACLEN and L4TUNLEN,
> when L4TUNT != 0.
L4TUNT only means the tunnel type, but if we don't want to do the checksum,
these fields will not be set
> 
> > 
> > > >  	/* UDP tunneling packet TX checksum offload */
> > > >  	if (ol_flags & PKT_TX_OUTER_IP_CKSUM) {
> > > >
> > > > @@ -1510,7 +1516,8 @@ i40e_calc_context_desc(uint64_t flags)
thanks again for the commets,
I will resend the patch
Zhe