From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 2EFD6C478 for ; Fri, 23 Oct 2015 11:03:09 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP; 23 Oct 2015 02:03:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,186,1444719600"; d="scan'208";a="817701339" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 23 Oct 2015 02:03:08 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 23 Oct 2015 02:03:07 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 23 Oct 2015 02:03:07 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.96]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.47]) with mapi id 14.03.0248.002; Fri, 23 Oct 2015 17:03:05 +0800 From: "Wang, Xiao W" To: "Zhang, Helin" , "dev@dpdk.org" Thread-Topic: [PATCH v4 1/2] e1000: enable igb TSO support Thread-Index: AQHRC9X0wb6QF5tt40iF04nwI9aVCp530F0AgAD5N/A= Date: Fri, 23 Oct 2015 09:03:05 +0000 Message-ID: References: <1445340132-2630-2-git-send-email-xiao.w.wang@intel.com> <1445414127-11260-1-git-send-email-xiao.w.wang@intel.com> <1445414127-11260-2-git-send-email-xiao.w.wang@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 1/2] e1000: enable igb TSO support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Oct 2015 09:03:09 -0000 Hi, Helin Thanks a lot for your comments, please find my reply inline. > -----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 >=20 >=20 >=20 > > -----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 > > --- > > 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 =3D 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 fir= st 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 ti= mes, for example "tx_offload.l3_len =3D tx_pkt->l3_len;"=20 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. >=20 > Regards, > Helin Since we only use 56 of the 64 bits as mask bits, I think using the union w= ill make it easier to understand the structure of the offload_mask. Thanks, Wang Xiao