diff mbox

[dpdk-dev,v2,2/2] net/mlx5: remove redundant checks

Message ID 20180522065631.23204-2-yskoh@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Shahaf Shuler
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Yongseok Koh May 22, 2018, 6:56 a.m. UTC 
  Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
Acked-by: Xueming Li <xuemingl@mellanox.com>
---

v2:
* add missing 'Acked-by' tag.

 drivers/net/mlx5/mlx5_rxtx.c |  6 ++----
 drivers/net/mlx5/mlx5_rxtx.h | 14 +++++++-------
 2 files changed, 9 insertions(+), 11 deletions(-)

Comments

Shahaf Shuler May 22, 2018, 7:43 a.m. UTC | #1 
Hi Koh,

Please see question below

Tuesday, May 22, 2018 9:57 AM, Yongseok Koh:
> Subject: [PATCH v2 2/2] net/mlx5: remove redundant checks
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> Acked-by: Xueming Li <xuemingl@mellanox.com>
> ---
> 
> v2:
> * add missing 'Acked-by' tag.
> 
>  drivers/net/mlx5/mlx5_rxtx.c |  6 ++----  drivers/net/mlx5/mlx5_rxtx.h | 14
> +++++++-------
>  2 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index cdd373e3e..527859461 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -522,7 +522,6 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf
> **pkts, uint16_t pkts_n)
>  		uint16_t ehdr;
>  		uint8_t cs_flags;
>  		uint8_t tso = txq->tso_en && (buf->ol_flags &
> PKT_TX_TCP_SEG);
> -		uint8_t is_vlan = !!(buf->ol_flags & PKT_TX_VLAN_PKT);
>  		uint32_t swp_offsets = 0;
>  		uint8_t swp_types = 0;
>  		uint16_t tso_segsz = 0;
> @@ -566,11 +565,10 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf
> **pkts, uint16_t pkts_n)
>  			rte_prefetch0(
>  			    rte_pktmbuf_mtod(*(pkts + 1), volatile void *));
>  		cs_flags = txq_ol_cksum_to_cs(buf);
> -		txq_mbuf_to_swp(txq, buf, tso, is_vlan,
> -				(uint8_t *)&swp_offsets, &swp_types);
> +		txq_mbuf_to_swp(txq, buf, (uint8_t *)&swp_offsets,
> &swp_types);
>  		raw = ((uint8_t *)(uintptr_t)wqe) + 2 *
> MLX5_WQE_DWORD_SIZE;
>  		/* Replace the Ethernet type by the VLAN if necessary. */
> -		if (is_vlan) {
> +		if (buf->ol_flags & PKT_TX_VLAN_PKT) {
>  			uint32_t vlan = rte_cpu_to_be_32(0x81000000 |
>  							 buf->vlan_tci);
>  			unsigned int len = 2 * ETHER_ADDR_LEN - 2; diff --git
> a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h index
> 1e4b2fdb9..5d9a7dffd 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.h
> +++ b/drivers/net/mlx5/mlx5_rxtx.h
> @@ -651,10 +651,10 @@ mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile
> struct mlx5_wqe *wqe)
>   */
>  static __rte_always_inline void
>  txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf *buf,
> -		 uint8_t tso, uint64_t vlan,
> -		 uint8_t *offsets, uint8_t *swp_types)
> +		uint8_t *offsets, uint8_t *swp_types)
>  {
> -	uint64_t tunnel = buf->ol_flags & PKT_TX_TUNNEL_MASK;
> +	const uint64_t vlan = buf->ol_flags & PKT_TX_VLAN_PKT;
> +	const uint64_t tunnel = buf->ol_flags & PKT_TX_TUNNEL_MASK;
>  	const uint64_t csum_flags = buf->ol_flags & PKT_TX_L4_MASK;
>  	const uint64_t inner_ip =
>  		buf->ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6); @@ -663,8
> +663,8 @@ txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf
> *buf,
>  	uint16_t idx;
>  	uint16_t off;
> 
> -	if (likely(!tunnel || !txq->swp_en ||
> -		   (tunnel != PKT_TX_TUNNEL_UDP && tunnel !=
> PKT_TX_TUNNEL_IP)))
> +	if (likely(!txq->swp_en || (tunnel != PKT_TX_TUNNEL_UDP &&
> +				    tunnel != PKT_TX_TUNNEL_IP)))
>  		return;
>  	/*
>  	 * The index should have:
> @@ -684,7 +684,7 @@ txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct
> rte_mbuf *buf,
>  	 * in if any of SWP offsets is set. Therefore, all of the L3 offsets
>  	 * should be set regardless of HW offload.
>  	 */
> -	off = buf->outer_l2_len + (vlan ? 4 : 0);
> +	off = buf->outer_l2_len + (vlan ? sizeof(struct vlan_hdr) : 0);
>  	offsets[1] = off >> 1; /* Outer L3 offset. */
>  	if (tunnel == PKT_TX_TUNNEL_UDP) {
>  		off += buf->outer_l3_len;
> @@ -693,7 +693,7 @@ txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct
> rte_mbuf *buf,
>  	if (inner_ip) {
>  		off += buf->l2_len;
>  		offsets[3] = off >> 1; /* Inner L3 offset. */
> -		if (csum_flags == PKT_TX_TCP_CKSUM || tso ||
> +		if (csum_flags == PKT_TX_TCP_CKSUM ||

Why are you removing the tso?
Per my understanding application can set only the PKT_TX_TCP_SEG for TSO and this implies TCP checksum. 

>  		    csum_flags == PKT_TX_UDP_CKSUM) {
>  			off += buf->l3_len;
>  			offsets[2] = off >> 1; /* Inner L4 offset. */
> --
> 2.11.0
Yongseok Koh May 22, 2018, 8:11 a.m. UTC | #2 
> On May 22, 2018, at 12:43 AM, Shahaf Shuler <shahafs@mellanox.com> wrote:
> 
> Hi Koh,
> 
> Please see question below
> 
> Tuesday, May 22, 2018 9:57 AM, Yongseok Koh:
>> Subject: [PATCH v2 2/2] net/mlx5: remove redundant checks
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> Acked-by: Xueming Li <xuemingl@mellanox.com>
>> ---
>> 
>> v2:
>> * add missing 'Acked-by' tag.
>> 
>> drivers/net/mlx5/mlx5_rxtx.c |  6 ++----  drivers/net/mlx5/mlx5_rxtx.h | 14
>> +++++++-------
>> 2 files changed, 9 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
>> index cdd373e3e..527859461 100644
>> --- a/drivers/net/mlx5/mlx5_rxtx.c
>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
>> @@ -522,7 +522,6 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf
>> **pkts, uint16_t pkts_n)
>> 		uint16_t ehdr;
>> 		uint8_t cs_flags;
>> 		uint8_t tso = txq->tso_en && (buf->ol_flags &
>> PKT_TX_TCP_SEG);
>> -		uint8_t is_vlan = !!(buf->ol_flags & PKT_TX_VLAN_PKT);
>> 		uint32_t swp_offsets = 0;
>> 		uint8_t swp_types = 0;
>> 		uint16_t tso_segsz = 0;
>> @@ -566,11 +565,10 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf
>> **pkts, uint16_t pkts_n)
>> 			rte_prefetch0(
>> 			    rte_pktmbuf_mtod(*(pkts + 1), volatile void *));
>> 		cs_flags = txq_ol_cksum_to_cs(buf);
>> -		txq_mbuf_to_swp(txq, buf, tso, is_vlan,
>> -				(uint8_t *)&swp_offsets, &swp_types);
>> +		txq_mbuf_to_swp(txq, buf, (uint8_t *)&swp_offsets,
>> &swp_types);
>> 		raw = ((uint8_t *)(uintptr_t)wqe) + 2 *
>> MLX5_WQE_DWORD_SIZE;
>> 		/* Replace the Ethernet type by the VLAN if necessary. */
>> -		if (is_vlan) {
>> +		if (buf->ol_flags & PKT_TX_VLAN_PKT) {
>> 			uint32_t vlan = rte_cpu_to_be_32(0x81000000 |
>> 							 buf->vlan_tci);
>> 			unsigned int len = 2 * ETHER_ADDR_LEN - 2; diff --git
>> a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h index
>> 1e4b2fdb9..5d9a7dffd 100644
>> --- a/drivers/net/mlx5/mlx5_rxtx.h
>> +++ b/drivers/net/mlx5/mlx5_rxtx.h
>> @@ -651,10 +651,10 @@ mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile
>> struct mlx5_wqe *wqe)
>>  */
>> static __rte_always_inline void
>> txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf *buf,
>> -		 uint8_t tso, uint64_t vlan,
>> -		 uint8_t *offsets, uint8_t *swp_types)
>> +		uint8_t *offsets, uint8_t *swp_types)
>> {
>> -	uint64_t tunnel = buf->ol_flags & PKT_TX_TUNNEL_MASK;
>> +	const uint64_t vlan = buf->ol_flags & PKT_TX_VLAN_PKT;
>> +	const uint64_t tunnel = buf->ol_flags & PKT_TX_TUNNEL_MASK;
>> 	const uint64_t csum_flags = buf->ol_flags & PKT_TX_L4_MASK;
>> 	const uint64_t inner_ip =
>> 		buf->ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6); @@ -663,8
>> +663,8 @@ txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf
>> *buf,
>> 	uint16_t idx;
>> 	uint16_t off;
>> 
>> -	if (likely(!tunnel || !txq->swp_en ||
>> -		   (tunnel != PKT_TX_TUNNEL_UDP && tunnel !=
>> PKT_TX_TUNNEL_IP)))
>> +	if (likely(!txq->swp_en || (tunnel != PKT_TX_TUNNEL_UDP &&
>> +				    tunnel != PKT_TX_TUNNEL_IP)))
>> 		return;
>> 	/*
>> 	 * The index should have:
>> @@ -684,7 +684,7 @@ txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct
>> rte_mbuf *buf,
>> 	 * in if any of SWP offsets is set. Therefore, all of the L3 offsets
>> 	 * should be set regardless of HW offload.
>> 	 */
>> -	off = buf->outer_l2_len + (vlan ? 4 : 0);
>> +	off = buf->outer_l2_len + (vlan ? sizeof(struct vlan_hdr) : 0);
>> 	offsets[1] = off >> 1; /* Outer L3 offset. */
>> 	if (tunnel == PKT_TX_TUNNEL_UDP) {
>> 		off += buf->outer_l3_len;
>> @@ -693,7 +693,7 @@ txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct
>> rte_mbuf *buf,
>> 	if (inner_ip) {
>> 		off += buf->l2_len;
>> 		offsets[3] = off >> 1; /* Inner L3 offset. */
>> -		if (csum_flags == PKT_TX_TCP_CKSUM || tso ||
>> +		if (csum_flags == PKT_TX_TCP_CKSUM ||
> 
> Why are you removing the tso?
> Per my understanding application can set only the PKT_TX_TCP_SEG for TSO and this implies TCP checksum. 

Right. I don't know why but I wrongly thought PKT_TX_TCP_SEG enforces PKT_TX_TCP_CKSUM.
Have to restore it.

Thanks,
Yongseok

>> 		    csum_flags == PKT_TX_UDP_CKSUM) {
>> 			off += buf->l3_len;
>> 			offsets[2] = off >> 1; /* Inner L4 offset. */
>> --
>> 2.11.0
diff mbox

Patch

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index cdd373e3e..527859461 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -522,7 +522,6 @@  mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 		uint16_t ehdr;
 		uint8_t cs_flags;
 		uint8_t tso = txq->tso_en && (buf->ol_flags & PKT_TX_TCP_SEG);
-		uint8_t is_vlan = !!(buf->ol_flags & PKT_TX_VLAN_PKT);
 		uint32_t swp_offsets = 0;
 		uint8_t swp_types = 0;
 		uint16_t tso_segsz = 0;
@@ -566,11 +565,10 @@  mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			rte_prefetch0(
 			    rte_pktmbuf_mtod(*(pkts + 1), volatile void *));
 		cs_flags = txq_ol_cksum_to_cs(buf);
-		txq_mbuf_to_swp(txq, buf, tso, is_vlan,
-				(uint8_t *)&swp_offsets, &swp_types);
+		txq_mbuf_to_swp(txq, buf, (uint8_t *)&swp_offsets, &swp_types);
 		raw = ((uint8_t *)(uintptr_t)wqe) + 2 * MLX5_WQE_DWORD_SIZE;
 		/* Replace the Ethernet type by the VLAN if necessary. */
-		if (is_vlan) {
+		if (buf->ol_flags & PKT_TX_VLAN_PKT) {
 			uint32_t vlan = rte_cpu_to_be_32(0x81000000 |
 							 buf->vlan_tci);
 			unsigned int len = 2 * ETHER_ADDR_LEN - 2;
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index 1e4b2fdb9..5d9a7dffd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -651,10 +651,10 @@  mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile struct mlx5_wqe *wqe)
  */
 static __rte_always_inline void
 txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf *buf,
-		 uint8_t tso, uint64_t vlan,
-		 uint8_t *offsets, uint8_t *swp_types)
+		uint8_t *offsets, uint8_t *swp_types)
 {
-	uint64_t tunnel = buf->ol_flags & PKT_TX_TUNNEL_MASK;
+	const uint64_t vlan = buf->ol_flags & PKT_TX_VLAN_PKT;
+	const uint64_t tunnel = buf->ol_flags & PKT_TX_TUNNEL_MASK;
 	const uint64_t csum_flags = buf->ol_flags & PKT_TX_L4_MASK;
 	const uint64_t inner_ip =
 		buf->ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6);
@@ -663,8 +663,8 @@  txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf *buf,
 	uint16_t idx;
 	uint16_t off;
 
-	if (likely(!tunnel || !txq->swp_en ||
-		   (tunnel != PKT_TX_TUNNEL_UDP && tunnel != PKT_TX_TUNNEL_IP)))
+	if (likely(!txq->swp_en || (tunnel != PKT_TX_TUNNEL_UDP &&
+				    tunnel != PKT_TX_TUNNEL_IP)))
 		return;
 	/*
 	 * The index should have:
@@ -684,7 +684,7 @@  txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf *buf,
 	 * in if any of SWP offsets is set. Therefore, all of the L3 offsets
 	 * should be set regardless of HW offload.
 	 */
-	off = buf->outer_l2_len + (vlan ? 4 : 0);
+	off = buf->outer_l2_len + (vlan ? sizeof(struct vlan_hdr) : 0);
 	offsets[1] = off >> 1; /* Outer L3 offset. */
 	if (tunnel == PKT_TX_TUNNEL_UDP) {
 		off += buf->outer_l3_len;
@@ -693,7 +693,7 @@  txq_mbuf_to_swp(struct mlx5_txq_data *txq, struct rte_mbuf *buf,
 	if (inner_ip) {
 		off += buf->l2_len;
 		offsets[3] = off >> 1; /* Inner L3 offset. */
-		if (csum_flags == PKT_TX_TCP_CKSUM || tso ||
+		if (csum_flags == PKT_TX_TCP_CKSUM ||
 		    csum_flags == PKT_TX_UDP_CKSUM) {
 			off += buf->l3_len;
 			offsets[2] = off >> 1; /* Inner L4 offset. */