[dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

2021-02-15 Thread Jiawei Zhu 
Fixed issue could occur when Mbuf starvation happens
in a middle of reception of a segmented packet.
In such a situation, after release the segments of that
packet, it does not align consumer index to the next stride.
This would cause receive a wrong segmented packet.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu <17826875...@163.com>
---
 drivers/net/mlx5/mlx5_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
rte_mbuf_raw_free(pkt);
pkt = rep;
}
+   rq_ci >>= sges_n;
+   ++rq_ci;
+   rq_ci <<= sges_n;
break;
}
if (!pkt) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

2021-02-26 Thread Jiawei Zhu 

Hi, Slava

Thanks for reading my patch, my issue may not be clear.
Here I give a possible error.
- we assume segs_n is 4 and we are receiving 4 segments multi-segment 
packet.
- we fail to alloc mbuf when receive the 3th segment，so it will free the 
mbufs which packet chain we have built. Here are the 1st and 2nd segment.
- Rx queue in this stride, the 1st and the 2nd segment are fill the new 
mbuf and there data will be rand, but the 3th and 4th segment are still 
fill the last data. So next if still begin on this stride， it will 
reveice wrong multi-segment packet.


- So we should discarded this packets and pass this stride. After exit 
the loop, we should align the next consumer index.


What Do you thinking?

With best regards
Jiawei

On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:

Hi, Jiawei

Thank you for the patch, but It seems I need some clarifications.
As far I understand the issue:

- we are in the midst of receiving the multi-segment packet
- we have some mbufs allocated and packet chain is partially built
- we fail on allocation replenishing mbuf for the segment
- we free all the mbuf of the built chain
- exit from the rx_burtst loop
- rq_ci is expected to be kept pointing to the beginning of the current
   stride - it is supposed on next rx_burst() invocation we'll continue
   Rx queue handling from the stride where we failed
- on loop exit we see the code:
if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
   return 0;
/* Update the consumer index. */
rxq->rq_ci = rq_ci >> sges_n;
hence, rq_ci is always shifted by sges_n, all increments happened
during failed packet processing are just discarded, it seems no fix is needed.

Did I miss something?

With best regards,
Slava


-----Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Monday, February 15, 2021 12:15
To: dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler ; Slava Ovsiienko ;
Jiawei Zhu <17826875...@163.com>; sta...@dpdk.org
Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx

Fixed issue could occur when Mbuf starvation happens in a middle of
reception of a segmented packet.
In such a situation, after release the segments of that packet, it does not 
align
consumer index to the next stride.
This would cause receive a wrong segmented packet.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu <17826875...@163.com>
---
  drivers/net/mlx5/mlx5_rxtx.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index
2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
rte_mbuf_raw_free(pkt);
pkt = rep;
}
+   rq_ci >>= sges_n;
+   ++rq_ci;
+   rq_ci <<= sges_n;
break;
}
if (!pkt) {
--
1.8.3.1

[dpdk-dev] [PATCH v2] net/mlx5: fix wrong segmented packet in Rx

2021-03-01 Thread Jiawei Zhu 
Fixed issue could occur when Mbuf starvation happens
in a middle of reception of a segmented packet.
In such a situation, after release the segments of that
packet, it does not align consumer index to the next stride.
This would cause receive a wrong segmented packet.

Here is a possible error.
- we assume segs_n is 4 and we are receiving 4
  segments multi-segment packet.
- we fail to alloc mbuf when receive the 3th segment
  so it will free the mbufs which packet chain we have
  built. Here are the 1st and 2nd segment.
- Rx queue in this stride, the 1st and the 2nd segment
  are fill the new mbuf and there data will be rand,
  but the 3th and 4th segment are still fill the last data.
  So next if still begin on this stride, it will receive
  wrong multi-segment packet.
- So we should discarded this packets and pass this stride.
  Before exit the loop, we should align the next consumer index.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu <17826875...@163.com>
---
v2:
* Added extra explanation in commit message.
---
 drivers/net/mlx5/mlx5_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
rte_mbuf_raw_free(pkt);
pkt = rep;
}
+   rq_ci >>= sges_n;
+   ++rq_ci;
+   rq_ci <<= sges_n;
break;
}
if (!pkt) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

2021-03-01 Thread Jiawei Zhu 

Hi, Slava
Thank you for your agreement. Here is the v2 patch:

https://patches.dpdk.org/project/dpdk/patch/1614617885-2650-1-git-send-email-17826875...@163.com/

With best regards,
Jiawei

On 2021/3/1 5:13 PM, Slava Ovsiienko wrote:

Hi, Jiawei

Thank you for the clarification. I missed the point that we have updated elts 
array
with new allocated mbufs and are not able to retry packet building anymore.
Very good catch, thank you!  Could you, please, add this extra explanation
to the  commit message and send the v2 ?

With best regards,
Slava


-Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Friday, February 26, 2021 18:11
To: Slava Ovsiienko ; dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler ; sta...@dpdk.org
Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx

Hi, Slava

Thanks for reading my patch, my issue may not be clear.
Here I give a possible error.
- we assume segs_n is 4 and we are receiving 4 segments multi-segment
packet.
- we fail to alloc mbuf when receive the 3th segment，so it will free the
mbufs which packet chain we have built. Here are the 1st and 2nd segment.
- Rx queue in this stride, the 1st and the 2nd segment are fill the new mbuf
and there data will be rand, but the 3th and 4th segment are still fill the last
data. So next if still begin on this stride， it will reveice wrong multi-segment
packet.

- So we should discarded this packets and pass this stride. After exit the loop,
we should align the next consumer index.

What Do you thinking?

With best regards
Jiawei

On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:

Hi, Jiawei

Thank you for the patch, but It seems I need some clarifications.
As far I understand the issue:

- we are in the midst of receiving the multi-segment packet
- we have some mbufs allocated and packet chain is partially built
- we fail on allocation replenishing mbuf for the segment
- we free all the mbuf of the built chain
- exit from the rx_burtst loop
- rq_ci is expected to be kept pointing to the beginning of the current
stride - it is supposed on next rx_burst() invocation we'll continue
Rx queue handling from the stride where we failed
- on loop exit we see the code:
 if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
return 0;
 /* Update the consumer index. */
 rxq->rq_ci = rq_ci >> sges_n;
hence, rq_ci is always shifted by sges_n, all increments happened
during failed packet processing are just discarded, it seems no fix is needed.

Did I miss something?

With best regards,
Slava


-----Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Monday, February 15, 2021 12:15
To: dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler ; Slava Ovsiienko
; Jiawei Zhu <17826875...@163.com>;
sta...@dpdk.org
Subject: [PATCH] net/mlx5: fix wrong segmented packet in Rx

Fixed issue could occur when Mbuf starvation happens in a middle of
reception of a segmented packet.
In such a situation, after release the segments of that packet, it
does not align consumer index to the next stride.
This would cause receive a wrong segmented packet.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx
path")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu <17826875...@163.com>
---
   drivers/net/mlx5/mlx5_rxtx.c | 3 +++
   1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c
b/drivers/net/mlx5/mlx5_rxtx.c index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
rte_mbuf_raw_free(pkt);
pkt = rep;
}
+   rq_ci >>= sges_n;
+   ++rq_ci;
+   rq_ci <<= sges_n;
break;
}
if (!pkt) {
--
1.8.3.1

[dpdk-dev] [PATCH v3] net/mlx5: fix wrong segmented packet in Rx

2021-03-02 Thread Jiawei Zhu 
The issue occurred if mbuf starvation happened
in the middle of segmented packet reception.
In such a situation, after release the segments of
packet being received, code did not advance the
consumer index to the next stride. This caused
the receiving of the wrong segmented packet data.

The possible error scenario:
- we assume segs_n is 4 and we are receiving 4
  segments of multi-segment packet.
- we fail to allocate mbuf while receiving the 3rd segment,
  and this frees the mbufs of the packet chain we have built.
  There are the 1st and 2nd segments in the chain.
- the 1st and the 2nd segments of this stride of Rx queue
  are filled up (in elts array) with the new allocated
  mbufs and their data are random (the 3rd and 4th
  segments still contain the valid data of the packet though).
- on the next iteration of stride processing we get
  the wrong two segments of the multi-segment packet.

Hence, we should skip these mbufs in the stride and
we should advance the consumer index on loop exit.

Fixes: 15a756b63734 ("net/mlx5: fix possible NULL dereference in Rx path")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu <17826875...@163.com>
---
v3:
* Reword the commit message a little bit.

v2:
* Added extra explanation in commit message.
---
 drivers/net/mlx5/mlx5_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 2e4b87c..e3ce9fd 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1480,6 +1480,9 @@ enum mlx5_txcmp_code {
rte_mbuf_raw_free(pkt);
pkt = rep;
}
+   rq_ci >>= sges_n;
+   ++rq_ci;
+   rq_ci <<= sges_n;
break;
}
if (!pkt) {
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix wrong segmented packet in Rx

2021-03-02 Thread Jiawei Zhu 

Hi, Slava

Thanks a lot for your correction and advice. I'm not very good at 
grammar 😁.

Here is new patch and used your words.😃

https://patches.dpdk.org/project/dpdk/patch/1614619190-3846-1-git-send-email-17826875...@163.com/

With best regards,
Jiawei

On 2021/3/2 4:10 PM, Slava Ovsiienko wrote:

Hi, Jiawei

Thanks a lot for the update.
There are some common points for the commit messages of fixing patches:
- the bug/error/issue should be described in PAST tense (what we HAD before the 
fix)
- what fix is doing should be described in PRESENT tense (what we HAVE right 
now, after fix apply)

Also, can we fix some typos in the message and reword it a little bit?
What do you think about something like this:

The issue occurred if mbuf starvation happened
in the middle of segmented packet reception.
In such a situation, after release the segments of
packet being received, code did not advance the
consumer index to the next stride. This caused
the receiving of the wrong segmented packet data.

The possible error scenario:
- we assume segs_n is 4 and we are receiving 4
   segments of multi-segment packet.
- we fail to allocate mbuf while receiving the 3rd segment,
   and this frees the mbufs of the packet chain we have built.
   There are the 1st and 2nd segments in the chain.
- the 1st and the 2nd segments of this stride of Rx queue
   are filled up (in elts array) with the new allocated
   mbufs and their data are random (the 3rd and 4th
   segments still contain the valid data of the packet though).
- on the next iteration of stride processing we get
   the wrong two segments of the multi-segment packet.

Hence, we should skip these mbufs in the stride and
we should advance the consumer index on loop exit.

With best regards,
Slava


-Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Monday, March 1, 2021 19:02
To: Slava Ovsiienko ; dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler ; sta...@dpdk.org
Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx

Hi, Slava
Thank you for your agreement. Here is the v2 patch:

https://patches.dpdk.org/project/dpdk/patch/1614617885-2650-1-git-send-
email-17826875...@163.com/

With best regards,
Jiawei

On 2021/3/1 5:13 PM, Slava Ovsiienko wrote:

Hi, Jiawei

Thank you for the clarification. I missed the point that we have
updated elts array with new allocated mbufs and are not able to retry

packet building anymore.

Very good catch, thank you!  Could you, please, add this extra
explanation to the  commit message and send the v2 ?

With best regards,
Slava


-Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Friday, February 26, 2021 18:11
To: Slava Ovsiienko ; dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler ; sta...@dpdk.org
Subject: Re: [PATCH] net/mlx5: fix wrong segmented packet in Rx

Hi, Slava

Thanks for reading my patch, my issue may not be clear.
Here I give a possible error.
- we assume segs_n is 4 and we are receiving 4 segments multi-segment
packet.
- we fail to alloc mbuf when receive the 3th segment，so it will free
the mbufs which packet chain we have built. Here are the 1st and 2nd

segment.

- Rx queue in this stride, the 1st and the 2nd segment are fill the
new mbuf and there data will be rand, but the 3th and 4th segment are
still fill the last data. So next if still begin on this stride， it
will reveice wrong multi-segment packet.

- So we should discarded this packets and pass this stride. After
exit the loop, we should align the next consumer index.

What Do you thinking?

With best regards
Jiawei

On 2021/2/24 9:20 PM, Slava Ovsiienko wrote:

Hi, Jiawei

Thank you for the patch, but It seems I need some clarifications.
As far I understand the issue:

- we are in the midst of receiving the multi-segment packet
- we have some mbufs allocated and packet chain is partially built
- we fail on allocation replenishing mbuf for the segment
- we free all the mbuf of the built chain
- exit from the rx_burtst loop
- rq_ci is expected to be kept pointing to the beginning of the current
 stride - it is supposed on next rx_burst() invocation we'll continue
 Rx queue handling from the stride where we failed
- on loop exit we see the code:
  if (unlikely((i == 0) && ((rq_ci >> sges_n) == rxq->rq_ci)))
 return 0;
  /* Update the consumer index. */
  rxq->rq_ci = rq_ci >> sges_n;
hence, rq_ci is always shifted by sges_n, all increments happened
during failed packet processing are just discarded, it seems no fix is

needed.


Did I miss something?

With best regards,
Slava


-Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Monday, February 15, 2021 12:15
To: dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ;

Shahaf

Shuler ; Slava Ovsiienko
; Jiawei Zhu <17826875...@163.com>;
sta...@dpdk.org
Subject: [PATCH] net/mlx5: fix wrong segmented packet in R

[dpdk-dev] [PATCH] net/mlx5: add Rx checksum offload flag return bad

2021-03-22 Thread Jiawei Zhu 
From: Jiawei Zhu 

When open the rx checksum offload and receive the wrong checksum,
add the ol_flags return bad. And it's not best to use multiplication
and division here.

Signed-off-by: Jiawei Zhu 
---
 drivers/net/mlx5/mlx5_rxtx.c  | 17 ++---
 drivers/net/mlx5/mlx5_utils.h |  6 --
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index e3ce9fd..9233af8 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1325,13 +1325,16 @@ enum mlx5_txcmp_code {
uint32_t ol_flags = 0;
uint16_t flags = rte_be_to_cpu_16(cqe->hdr_type_etc);
 
-   ol_flags =
-   TRANSPOSE(flags,
- MLX5_CQE_RX_L3_HDR_VALID,
- PKT_RX_IP_CKSUM_GOOD) |
-   TRANSPOSE(flags,
- MLX5_CQE_RX_L4_HDR_VALID,
- PKT_RX_L4_CKSUM_GOOD);
+   if (flags & MLX5_CQE_RX_L3_HDR_VALID)
+   ol_flags |= PKT_RX_IP_CKSUM_GOOD;
+   else
+   ol_flags |= PKT_RX_IP_CKSUM_BAD;
+
+   if (flags & MLX5_CQE_RX_L4_HDR_VALID)
+   ol_flags |= PKT_RX_IP_CKSUM_GOOD;
+   else
+   ol_flags |= PKT_RX_IP_CKSUM_BAD;
+
return ol_flags;
 }
 
diff --git a/drivers/net/mlx5/mlx5_utils.h b/drivers/net/mlx5/mlx5_utils.h
index 7a62187..2f71a23 100644
--- a/drivers/net/mlx5/mlx5_utils.h
+++ b/drivers/net/mlx5/mlx5_utils.h
@@ -44,12 +44,6 @@
 #define NB_SEGS(m) ((m)->nb_segs)
 #define PORT(m) ((m)->port)
 
-/* Transpose flags. Useful to convert IBV to DPDK flags. */
-#define TRANSPOSE(val, from, to) \
-   (((from) >= (to)) ? \
-(((val) & (from)) / ((from) / (to))) : \
-(((val) & (from)) * ((to) / (from
-
 /*
  * For the case which data is linked with sequence increased index, the
  * array table will be more efficiect than hash table once need to serarch
-- 
1.8.3.1

[dpdk-dev] [PATCH] net/mlx5: add Rx checksum offload flag return bad

2021-03-23 Thread Jiawei Zhu 
From: Jiawei Zhu 

When open the rx checksum offload and receive the wrong checksum,
add the ol_flags return bad. And it's not best to use multiplication
and division here.

Signed-off-by: Jiawei Zhu 
---
 drivers/net/mlx5/mlx5_rxtx.c  | 17 ++---
 drivers/net/mlx5/mlx5_utils.h |  6 --
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index e3ce9fd..dcc0ebc 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1325,13 +1325,16 @@ enum mlx5_txcmp_code {
uint32_t ol_flags = 0;
uint16_t flags = rte_be_to_cpu_16(cqe->hdr_type_etc);
 
-   ol_flags =
-   TRANSPOSE(flags,
- MLX5_CQE_RX_L3_HDR_VALID,
- PKT_RX_IP_CKSUM_GOOD) |
-   TRANSPOSE(flags,
- MLX5_CQE_RX_L4_HDR_VALID,
- PKT_RX_L4_CKSUM_GOOD);
+   if (flags & MLX5_CQE_RX_L3_HDR_VALID)
+   ol_flags |= PKT_RX_IP_CKSUM_GOOD;
+   else
+   ol_flags |= PKT_RX_IP_CKSUM_BAD;
+
+   if (flags & MLX5_CQE_RX_L4_HDR_VALID)
+   ol_flags |= PKT_RX_L4_CKSUM_GOOD;
+   else
+   ol_flags |= PKT_RX_L4_CKSUM_BAD;
+
return ol_flags;
 }
 
diff --git a/drivers/net/mlx5/mlx5_utils.h b/drivers/net/mlx5/mlx5_utils.h
index 7a62187..2f71a23 100644
--- a/drivers/net/mlx5/mlx5_utils.h
+++ b/drivers/net/mlx5/mlx5_utils.h
@@ -44,12 +44,6 @@
 #define NB_SEGS(m) ((m)->nb_segs)
 #define PORT(m) ((m)->port)
 
-/* Transpose flags. Useful to convert IBV to DPDK flags. */
-#define TRANSPOSE(val, from, to) \
-   (((from) >= (to)) ? \
-(((val) & (from)) / ((from) / (to))) : \
-(((val) & (from)) * ((to) / (from
-
 /*
  * For the case which data is linked with sequence increased index, the
  * array table will be more efficiect than hash table once need to serarch
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: add Rx checksum offload flag return bad

2021-03-24 Thread Jiawei Zhu 

Hi，Slava

Thanks for your explain，the multiplications and divisions are in the 
TRANSPOSE，not in the rte_be_to_cpu_16. So I think use if-else directly 
could improves the performance. And the second point about the bad sum，I 
agree with you.


With best regards,
Jiawei

On 2021/3/24 12:33 上午, Slava Ovsiienko wrote:

Hi, Jiawei


-Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Monday, March 22, 2021 17:46
To: dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler ; Slava Ovsiienko 
Subject: [PATCH] net/mlx5: add Rx checksum offload flag return bad

From: Jiawei Zhu 

When open the rx checksum offload and receive the wrong checksum, add
the ol_flags return bad. And it's not best to use multiplication and division
here.


I'm sorry, there should be no any multiplications and divisions - the arguments
are constants (can be determined at compilation time) and ara power of 2,
hence compiler engages simple shifts. For example (I did rxq_cq_to_ol_flags not 
inline to
get the listing for x86-64):

   29   rxq_cq_to_ol_flags:
   21   /*
   22* An architecture-optimized byte swap for a 16-bit 
value.
   23*
   24* Do not use this function directly. The preferred 
function is rte_bswap16().
   25*/
   26   static inline uint16_t rte_arch_bswap16(uint16_t _x)
   27   {
   28   uint16_t x = _x;
   29 0034 86D6 asm volatile ("xchgb %b[x1],%h[x2]"
   30 : [x1] "=Q" (x)
   37   
   38 0036 89D0  movl %edx,%eax
   39 0038 6681E200  andw $512,%dx
   39  02
   40 003d 66250004  andw $1024,%ax
   41 0041 0FB7D2movzwl %dx,%edx
   42 0044 0FB7C0movzwl %ax,%eax
   43 0047 48C1EA02  shrq $2,%rdx
   44 004b 48C1E802  shrq $2,%rax
   45 004f 09D0  orl %edx,%eax
   46 0051 C3ret

As we can see - there is no any mul/div and no any branches, that
improves the performance.



Signed-off-by: Jiawei Zhu 
---
  drivers/net/mlx5/mlx5_rxtx.c  | 17 ++---
drivers/net/mlx5/mlx5_utils.h |  6 --
  2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index e3ce9fd..9233af8 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1325,13 +1325,16 @@ enum mlx5_txcmp_code {
uint32_t ol_flags = 0;
uint16_t flags = rte_be_to_cpu_16(cqe->hdr_type_etc);

-   ol_flags =
-   TRANSPOSE(flags,
- MLX5_CQE_RX_L3_HDR_VALID,
- PKT_RX_IP_CKSUM_GOOD) |
-   TRANSPOSE(flags,
- MLX5_CQE_RX_L4_HDR_VALID,
- PKT_RX_L4_CKSUM_GOOD);
+   if (flags & MLX5_CQE_RX_L3_HDR_VALID)
+   ol_flags |= PKT_RX_IP_CKSUM_GOOD;
+   else
+   ol_flags |= PKT_RX_IP_CKSUM_BAD;
+


If MLX5_CQE_RX_L3_HDR_VALID is not set - it does not always mean the sum is bad.
If might happen if HW just did not recognize the packet format (for example, for
some tunnels)

With best regards,
Slava



+   if (flags & MLX5_CQE_RX_L4_HDR_VALID)
+   ol_flags |= PKT_RX_IP_CKSUM_GOOD;
+   else
+   ol_flags |= PKT_RX_IP_CKSUM_BAD;
+
return ol_flags;
  }

diff --git a/drivers/net/mlx5/mlx5_utils.h b/drivers/net/mlx5/mlx5_utils.h
index 7a62187..2f71a23 100644
--- a/drivers/net/mlx5/mlx5_utils.h
+++ b/drivers/net/mlx5/mlx5_utils.h
@@ -44,12 +44,6 @@
  #define NB_SEGS(m) ((m)->nb_segs)
  #define PORT(m) ((m)->port)

-/* Transpose flags. Useful to convert IBV to DPDK flags. */ -#define
TRANSPOSE(val, from, to) \
-   (((from) >= (to)) ? \
-(((val) & (from)) / ((from) / (to))) : \
-(((val) & (from)) * ((to) / (from
-
  /*
   * For the case which data is linked with sequence increased index, the
   * array table will be more efficiect than hash table once need to serarch
--
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: add Rx checksum offload flag return bad

2021-03-28 Thread Jiawei Zhu 

Hi, Slava

Thanks for your detailed explanation！You are right，I didn't look 
carefully!


With best regards,
Jiawei

On 2021/3/25 7:55 下午, Slava Ovsiienko wrote:

Hi, Jiawei


-Original Message-
From: Jiawei Zhu <17826875...@163.com>
Sent: Wednesday, March 24, 2021 18:22
To: Slava Ovsiienko ; dev@dpdk.org
Cc: zhujiawe...@huawei.com; Matan Azrad ; Shahaf
Shuler 
Subject: Re: [PATCH] net/mlx5: add Rx checksum offload flag return bad

Hi，Slava

Thanks for your explain，the multiplications and divisions are in the
TRANSPOSE，not in the rte_be_to_cpu_16.


[SO]
Yes, TRANSPOSE is the macro with mul and div operators. But, these ones
are translated by compiler to the simple shifts (due to operands are power of 
2).
The only place where TRANSPOSE is used is the rxq_cq_to_ol_flags() routine.
I've compiled this one  and provided the assembly listing - please see one
in my previous reply. It illustrates how TRASPOSE was compiled to and
presents the x86 code - we see only shifts:

43 0047 48C1EA02 shrq $2,%rdx
44 004b 48C1E802 shrq $2,%rax

No any mul/div, exactly as we expected.


So I think use if-else directly could improves the performance.


[SO]
The if/else construction is usually compiled to conditional jumps, the branch
prediction in CPU over the various ingress traffic patterns  (we are analyzing 
the
flags of the received packets) might not work well and we’ll get performance 
penalty.
Hence, it seems the best practice is not to have the conditional jumps at all.
The existing code follows this approach as we can see from the assembly listing 
- there
is no conditional jumps.

With best regards,
Slava

PS. Just removed embarrassing details from the listing - this is merely the 
resulting code
of rxq_cq_to_ol_flags(). I removed static and made this one non-inline to see 
the
isolated piece of code:

rxq_cq_to_ol_flags:
   movzwl 28(%rdi),%edx   // endianness conversion optimized out at all
   movl %edx,%eax
   andw $512,%dx
   andw $1024,%ax
   movzwl %dx,%edx
   movzwl %ax,%eax
   shrq $2,%rdx
   shrq $2,%rax
   orl %edx,%eax
   ret

PPS. As we can see - the shift values are the same for both flags, so there 
might be some area to optimize
(we could have only one shift and only one masking with AND)

[dpdk-dev] [PATCH] net/virtio-user: fix error run close(0)

2020-11-28 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu 
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..1bfd223 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -284,7 +284,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1

[dpdk-dev] [PATCH] net/virtio-user: fix error run close(0)

2020-11-30 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org

Signed-off-by: Jiawei Zhu 
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..1bfd223 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -284,7 +284,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/virtio-user: fix run close(0) and close callfd

2020-12-11 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).
And when the code fails in kickfd creationg,
it will leaves one callfd not closed.

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org:

Signed-off-by: Jiawei Zhu <17826875...@163.com>
---
v2:
* Add close callfd when fail in kickfd creation before break.
---
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..e1cbad0 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -276,6 +276,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
if (kickfd < 0) {
+   close(callfd);
PMD_DRV_LOG(ERR, "kickfd error, %s", strerror(errno));
break;
}
@@ -284,7 +285,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/virtio-user: fix run close(0) and close callfd

2020-12-11 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).
And when the code fails in kickfd creation,
it will leaves one callfd not closed.

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org:

Signed-off-by: Jiawei Zhu 
---
v2:
* Add close callfd when fail in kickfd creation before break.
---
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..e1cbad0 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -276,6 +276,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
if (kickfd < 0) {
+   close(callfd);
PMD_DRV_LOG(ERR, "kickfd error, %s", strerror(errno));
break;
}
@@ -284,7 +285,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/virtio-user: fix run close(0) and close callfd

2020-12-11 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).
And when the code fails in kickfd creation,
it will leaves one callfd not closed.

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org:

Signed-off-by: Jiawei Zhu 
---
v2:
* Add close callfd when fail in kickfd creation before break.
---
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..e1cbad0 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -276,6 +276,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
if (kickfd < 0) {
+   close(callfd);
PMD_DRV_LOG(ERR, "kickfd error, %s", strerror(errno));
break;
}
@@ -284,7 +285,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/virtio-user: fix run close(0) and close callfd

2020-12-11 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).
And when the code fails in kickfd creation,
it will leaves one callfd not closed.

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org:

Signed-off-by: Jiawei Zhu 
---
v2:
* Add close callfd when fail in kickfd creation before break.
---
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..e1cbad0 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -276,6 +276,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
if (kickfd < 0) {
+   close(callfd);
PMD_DRV_LOG(ERR, "kickfd error, %s", strerror(errno));
break;
}
@@ -284,7 +285,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1

[dpdk-dev] [PATCH v2] net/virtio-user: fix run close(0) and close callfd

2020-12-11 Thread Jiawei Zhu 
From: Jiawei Zhu 

When i < VIRTIO_MAX_VIRTQUEUES and j == i,
dev->callfds[i] and dev->kickfds[i] are default 0.
So it will close(0), close the standard input (stdin).
And when the code fails in kickfd creation,
it will leaves one callfd not closed.

Fixes: e6e7ad8b3024 ("net/virtio-user: move eventfd open/close into 
init/uninit")
Cc: sta...@dpdk.org:

Signed-off-by: Jiawei Zhu 
---
v2:
* Add close callfd when fail in kickfd creation before break.
---
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 053f026..e1cbad0 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -276,6 +276,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
if (kickfd < 0) {
+   close(callfd);
PMD_DRV_LOG(ERR, "kickfd error, %s", strerror(errno));
break;
}
@@ -284,7 +285,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
}
 
if (i < VIRTIO_MAX_VIRTQUEUES) {
-   for (j = 0; j <= i; ++j) {
+   for (j = 0; j < i; ++j) {
close(dev->callfds[j]);
close(dev->kickfds[j]);
}
-- 
1.8.3.1