Re: [dpdk-dev] [PATCH v4 1/2] e1000: enable igb TSO support

["Wang, Xiao W" <xiao.w.wang@intel.com>]

-----Original Message-----
From: Zhang, Helin 
Sent: Friday, October 23, 2015 10:04 AM
To: Wang, Xiao W; dev@dpdk.org
Cc: Lu, Wenzhuo; Richardson, Bruce
Subject: RE: [PATCH v4 1/2] e1000: enable igb TSO support



> -----Original Message-----
> From: Wang, Xiao W
> Sent: Wednesday, October 21, 2015 3:55 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo; Richardson, Bruce; Zhang, Helin; Wang, Xiao W
> Subject: [PATCH v4 1/2] e1000: enable igb TSO support
> 
> This patch enables igb TSO feature, the feature works on both PF and VF.
> The TCP segmentation offload needs to write the offload related 
> information into the advanced context descriptors, which is similar to checksum offload.
> 
> Signed-off-by: Wang Xiao W <xiao.w.wang@intel.com>
> ---
>  drivers/net/e1000/igb_ethdev.c |   6 +-
>  drivers/net/e1000/igb_rxtx.c   | 200
> +++++++++++++++++++++++++----------------
>  2 files changed, 127 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/net/e1000/igb_ethdev.c 
> b/drivers/net/e1000/igb_ethdev.c index 848ef6e..2e69394 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -1497,7 +1497,8 @@ eth_igb_infos_get(struct rte_eth_dev *dev, 
> struct rte_eth_dev_info *dev_info)
>  		DEV_TX_OFFLOAD_IPV4_CKSUM  |
>  		DEV_TX_OFFLOAD_UDP_CKSUM   |
>  		DEV_TX_OFFLOAD_TCP_CKSUM   |
> -		DEV_TX_OFFLOAD_SCTP_CKSUM;
> +		DEV_TX_OFFLOAD_SCTP_CKSUM  |
> +		DEV_TX_OFFLOAD_TCP_TSO;
> 
>  	switch (hw->mac.type) {
>  	case e1000_82575:
> @@ -1588,7 +1589,8 @@ eth_igbvf_infos_get(struct rte_eth_dev *dev, 
> struct rte_eth_dev_info *dev_info)
>  				DEV_TX_OFFLOAD_IPV4_CKSUM  |
>  				DEV_TX_OFFLOAD_UDP_CKSUM   |
>  				DEV_TX_OFFLOAD_TCP_CKSUM   |
> -				DEV_TX_OFFLOAD_SCTP_CKSUM;
> +				DEV_TX_OFFLOAD_SCTP_CKSUM  |
> +				DEV_TX_OFFLOAD_TCP_TSO;
>  	switch (hw->mac.type) {
>  	case e1000_vfadapt:
>  		dev_info->max_rx_queues = 2;
> diff --git a/drivers/net/e1000/igb_rxtx.c 
> b/drivers/net/e1000/igb_rxtx.c index
> 19905fd..2b8a1c8 100644
> --- a/drivers/net/e1000/igb_rxtx.c
> +++ b/drivers/net/e1000/igb_rxtx.c
> @@ -76,7 +76,8 @@
>  #define IGB_TX_OFFLOAD_MASK (			 \
>  		PKT_TX_VLAN_PKT |		 \
>  		PKT_TX_IP_CKSUM |		 \
> -		PKT_TX_L4_MASK)
> +		PKT_TX_L4_MASK |		 \
> +		PKT_TX_TCP_SEG)
> 
>  static inline struct rte_mbuf *
>  rte_rxmbuf_alloc(struct rte_mempool *mp) @@ -146,32 +147,40 @@ enum 
> igb_advctx_num {  };
> 
>  /** Offload features */
> -union igb_vlan_macip {
> -	uint32_t data;
> +union igb_tx_offload {
> +	uint64_t data;
Adding previous definition back to this union is helpful of using the first 16 bits and second 16 bits?
Of cause, the newly added struct should be kept as is.
	struct {
		uint16_t l2_l3_len;
		uint16_t vlan_tci;
		uint16_t l4_tso_len;
		uint16_t others;
	};

In the patch, we need to quote the l3_len and l2_len separately for some times, for example "tx_offload.l3_len = tx_pkt->l3_len;" 
It's more convenient to define them separately.

>  	struct {
> -		uint16_t l2_l3_len; /**< 7bit L2 and 9b L3 lengths combined */
> -		uint16_t vlan_tci;
> -		/**< VLAN Tag Control Identifier (CPU order). */
> -	} f;
> +		uint64_t l3_len:9; /**< L3 (IP) Header Length. */
> +		uint64_t l2_len:7; /**< L2 (MAC) Header Length. */
> +		uint64_t vlan_tci:16;  /**< VLAN Tag Control Identifier(CPU order). */
> +		uint64_t l4_len:8; /**< L4 (TCP/UDP) Header Length. */
> +		uint64_t tso_segsz:16; /**< TCP TSO segment size. */
> +
> +		/* uint64_t unused:8; */
> +	};
>  };
> 
>  /*
> - * Compare mask for vlan_macip_len.data,
> - * should be in sync with igb_vlan_macip.f layout.
> + * Compare mask for igb_tx_offload.data,
> + * should be in sync with igb_tx_offload layout.
>   * */
> -#define TX_VLAN_CMP_MASK        0xFFFF0000  /**< VLAN length -
> 16-bits. */
> -#define TX_MAC_LEN_CMP_MASK     0x0000FE00  /**< MAC length - 7-bits.
> */
> -#define TX_IP_LEN_CMP_MASK      0x000001FF  /**< IP  length - 9-bits. */
> -/** MAC+IP  length. */
> -#define TX_MACIP_LEN_CMP_MASK   (TX_MAC_LEN_CMP_MASK |
> TX_IP_LEN_CMP_MASK)
> +#define TX_MACIP_LEN_CMP_MASK	0x000000000000FFFFULL /**< L2L3
> header mask. */
> +#define TX_VLAN_CMP_MASK		0x00000000FFFF0000ULL /**< Vlan mask.
> */
> +#define TX_TCP_LEN_CMP_MASK		0x000000FF00000000ULL /**< TCP
> header mask. */
> +#define TX_TSO_MSS_CMP_MASK		0x00FFFF0000000000ULL /**< TSO
> segsz mask. */
> +/** Mac + IP + TCP + Mss mask. */
> +#define TX_TSO_CMP_MASK	\
> +	(TX_MACIP_LEN_CMP_MASK | TX_TCP_LEN_CMP_MASK |
> TX_TSO_MSS_CMP_MASK)
> 
>  /**
>   * Strucutre to check if new context need be built
>   */
>  struct igb_advctx_info {
>  	uint64_t flags;           /**< ol_flags related to context build. */
> -	uint32_t cmp_mask;        /**< compare mask for vlan_macip_lens */
> -	union igb_vlan_macip vlan_macip_lens; /**< vlan, mac & ip length. */
> +	/** tx offload: vlan, tso, l2-l3-l4 lengths. */
> +	union igb_tx_offload tx_offload;
> +	/** compare mask for tx offload. */
> +	union igb_tx_offload tx_offload_mask;
Would above mask be better/clearer replaced by uint64_t directly?
You can think a bit, I am not quite sure if it is really better.

Regards,
Helin

Since we only use 56 of the 64 bits as mask bits, I think using the union will make it easier to understand the
structure of the offload_mask.

Thanks,
Wang Xiao