fix protocol size calculation

PATCH Applied Xiaoyu Min 12 participants 30 messages Nov 16, 2020 – Nov 27, 2020 Network drivers

lore

Share & tools

lore

Xiaoyu Min

author

2020-11-16 07:55 UTC #0 lore raw

cover fix protocol size calculation

From: Xiaoyu Min <[email protected]>

The rte_flow_item_eth, rte_flow_item_vlan, and rte_flow_item_ipv6 are refined. The structs do not exactly represent the real protocol headers any more.

This serial patchs try to fix all related parts due to the changes.

Dekel Peled (1):
net/softnic: update headers size calculation

Xiaoyu Min (4):
net/mlx5: fix protocol size for raw encap judgement app/flow-perf: fix protocol size for raw encap net/bnxt: fix protocol size for VXLAN encap copy net/iavf: fix protocol size for virtchnl copy

 app/test-flow-perf/actions_gen.c           | 136 ++++++++++-----------
 drivers/net/bnxt/tf_ulp/ulp_rte_parser.c   |  11 +-
 drivers/net/iavf/iavf_fdir.c               |   2 +-
 drivers/net/mlx5/mlx5_flow.c               |  12 +-
 drivers/net/mlx5/mlx5_flow.h               |   4 +-
 drivers/net/softnic/rte_eth_softnic_flow.c |   8 +-
 6 files changed, 86 insertions(+), 87 deletions(-)

Signature · 1 line

2.25.1

Xiaoyu Min

author

2020-11-16 07:55 UTC #1 lore raw

PATCH 1/5 net/mlx5: fix protocol size for raw encap judgement

From: Xiaoyu Min <[email protected]>

The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore.
Should use real header instead of the whole struct.

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.

Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

Signed-off-by Xiaoyu Min <[email protected]>

---
 drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
 drivers/net/mlx5/mlx5_flow.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

drivers/net/mlx5/mlx5_flow.c +6 -6

index 324349ed19..34b7a2f137 100644
@@ -3655,8 +3655,8 @@ flow_check_hairpin_split(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
 			raw_encap = actions->conf;
 			if (raw_encap->size >
-			    (sizeof(struct rte_flow_item_eth) +
-			     sizeof(struct rte_flow_item_ipv4)))
+			    (sizeof(struct rte_ether_hdr) +
+			     sizeof(struct rte_ipv4_hdr)))
 				split++;
 			action_n++;
 			break;

@@ -4092,8 +4092,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
 			raw_encap = actions->conf;
 			if (raw_encap->size >
-			    (sizeof(struct rte_flow_item_eth) +
-			     sizeof(struct rte_flow_item_ipv4))) {
+			    (sizeof(struct rte_ether_hdr) +
+			     sizeof(struct rte_ipv4_hdr))) {
 				memcpy(actions_tx, actions,
 				       sizeof(struct rte_flow_action));
 				actions_tx++;

@@ -4107,8 +4107,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_RAW_DECAP:
 			raw_decap = actions->conf;
 			if (raw_decap->size <
-			    (sizeof(struct rte_flow_item_eth) +
-			     sizeof(struct rte_flow_item_ipv4))) {
+			    (sizeof(struct rte_ether_hdr) +
+			     sizeof(struct rte_ipv4_hdr))) {
 				memcpy(actions_tx, actions,
 				       sizeof(struct rte_flow_action));
 				actions_tx++;

drivers/net/mlx5/mlx5_flow.h +2 -2

index 5fac8672fc..3b28c48a1d 100644
@@ -345,8 +345,8 @@ enum mlx5_feature_name {
 #define MLX5_GENEVE_OPT_LEN_0 14
 #define MLX5_GENEVE_OPT_LEN_1 63
 
-#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_flow_item_eth) + \
-					  sizeof(struct rte_flow_item_ipv4))
+#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_ether_hdr) + \
+					  sizeof(struct rte_ipv4_hdr))
 
 /* IPv4 fragment_offset field contains relevant data in bits 2 to 15. */
 #define MLX5_IPV4_FRAG_OFFSET_MASK \

Signature · 1 line

2.25.1

Matan Azrad

Acked-by on v1 1/5

2020-11-17 13:26 UTC #2 lore raw

Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

From: Xiaoyu Min

Quoted 11 lines

The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore.
Should use real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Signed-off-by Xiaoyu Min <[email protected]>

Acked-by Matan Azrad <[email protected]>

Thomas Monjalon

2020-11-22 14:22 UTC #3 lore raw

Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

17/11/2020 14:25, Matan Azrad:

Quoted 6 lines

From: Xiaoyu Min
Quoted 5 lines
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore.
Should use real header instead of the whole struct.

Quoted 7 lines

Quoted 6 lines
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Signed-off-by Xiaoyu Min <[email protected]>
Acked-by Matan Azrad <[email protected]>

It has been partially fixed later (and already merged) by Slava:
http://git.dpdk.org/dpdk/commit/?id=f9210259cac

Thomas Monjalon

2020-11-22 15:33 UTC #4 lore raw

Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

In reply to Xiaoyu Min

16/11/2020 08:55, Xiaoyu Min:

Quoted 59 lines

From: Xiaoyu Min <[email protected]>

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.

Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

Signed-off-by Xiaoyu Min <[email protected]>

---
 drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
 drivers/net/mlx5/mlx5_flow.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

drivers/net/mlx5/mlx5_flow.c +6 -6

index 324349ed19..34b7a2f137 100644
@@ -3655,8 +3655,8 @@ flow_check_hairpin_split(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
 			raw_encap = actions->conf;
 			if (raw_encap->size >
-			    (sizeof(struct rte_flow_item_eth) +
-			     sizeof(struct rte_flow_item_ipv4)))
+			    (sizeof(struct rte_ether_hdr) +
+			     sizeof(struct rte_ipv4_hdr)))
 				split++;
 			action_n++;
 			break;

@@ -4092,8 +4092,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
 			raw_encap = actions->conf;
 			if (raw_encap->size >
-			    (sizeof(struct rte_flow_item_eth) +
-			     sizeof(struct rte_flow_item_ipv4))) {
+			    (sizeof(struct rte_ether_hdr) +
+			     sizeof(struct rte_ipv4_hdr))) {
 				memcpy(actions_tx, actions,
 				       sizeof(struct rte_flow_action));
 				actions_tx++;

@@ -4107,8 +4107,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
 		case RTE_FLOW_ACTION_TYPE_RAW_DECAP:
 			raw_decap = actions->conf;
 			if (raw_decap->size <
-			    (sizeof(struct rte_flow_item_eth) +
-			     sizeof(struct rte_flow_item_ipv4))) {
+			    (sizeof(struct rte_ether_hdr) +
+			     sizeof(struct rte_ipv4_hdr))) {
 				memcpy(actions_tx, actions,
 				       sizeof(struct rte_flow_action));
 				actions_tx++;

drivers/net/mlx5/mlx5_flow.h +0 -0

index 5fac8672fc..3b28c48a1d 100644

drivers/net/mlx5/mlx5_flow.h enum mlx5_feature_name { · line 349

Quoted 7 lines

drivers/net/mlx5/mlx5_flow.h +2 -2

@@ -345,8 +345,8 @@ enum mlx5_feature_name {
 #define MLX5_GENEVE_OPT_LEN_0 14
 #define MLX5_GENEVE_OPT_LEN_1 63
 
-#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_flow_item_eth) + \
-					  sizeof(struct rte_flow_item_ipv4))
+#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct rte_ether_hdr) + \
+					  sizeof(struct rte_ipv4_hdr))

This constant should be used above in hairpin functions. Will change while merging.

Matan Azrad

2020-11-22 16:04 UTC #5 lore raw

Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

From: Thomas Monjalon

Quoted 65 lines

16/11/2020 08:55, Xiaoyu Min:

Quoted 62 lines

From: Xiaoyu Min <[email protected]>

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.

Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN

items")

Signed-off-by Xiaoyu Min <[email protected]>

---
 drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
drivers/net/mlx5/mlx5_flow.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

(quoted diff) +0 -0

b/drivers/net/mlx5/mlx5_flow.c index 324349ed19..34b7a2f137 100644

drivers/net/mlx5/mlx5_flow.c +4 -4

@@ -3655,8 +3655,8 @@ flow_check_hairpin_split(struct rte_eth_dev *dev,
              case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
                      raw_encap = actions->conf;
                      if (raw_encap->size >
-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4)))
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr)))
                              split++;
                      action_n++;
                      break;

@@ -4092,8 +4092,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
              case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
                      raw_encap = actions->conf;
                      if (raw_encap->size >
-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4))) {
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr))) {
                              memcpy(actions_tx, actions,
                                     sizeof(struct rte_flow_action));
                              actions_tx++; @@ -4107,8 +4107,8 @@

flow_hairpin_split(struct rte_eth_dev *dev,

              case RTE_FLOW_ACTION_TYPE_RAW_DECAP:
                      raw_decap = actions->conf;
                      if (raw_decap->size <

drivers/net/mlx5/mlx5_flow.c +2 -2

-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4))) {
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr))) {
                              memcpy(actions_tx, actions,
                                     sizeof(struct rte_flow_action));
                              actions_tx++; diff --git

a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h index 5fac8672fc..3b28c48a1d 100644

drivers/net/mlx5/mlx5_flow.h +0 -0

@@ -345,8 +345,8 @@ enum mlx5_feature_name {  #define

MLX5_GENEVE_OPT_LEN_0 14 #define MLX5_GENEVE_OPT_LEN_1 63

Quoted 1 line

-#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct

rte_flow_item_eth) + \

Quoted 7 lines

drivers/net/mlx5/mlx5_flow.h
Quoted 2 lines
drivers/net/mlx5/mlx5_flow.h +1 -1
-                                       sizeof(struct rte_flow_item_ipv4))
+#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct
rte_ether_hdr) + \
Quoted 1 line
+ sizeof(struct rte_ipv4_hdr))
This constant should be used above in hairpin functions. Will change while merging.

Ori Kam

2020-11-23 07:54 UTC #6 lore raw

Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

Quoted 80 lines

-----Original Message-----
From: Matan Azrad <[email protected]>
Sent: Sunday, November 22, 2020 6:05 PM
Subject: RE: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

From: Thomas Monjalon

Quoted 19 lines

16/11/2020 08:55, Xiaoyu Min:
Quoted 18 lines
From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore.
Should use real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Signed-off-by Xiaoyu Min <[email protected]>
---
 drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
drivers/net/mlx5/mlx5_flow.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

Quoted 5 lines

drivers/net/mlx5/mlx5_flow.c flow_check_hairpin_split(struct rte_eth_dev
Quoted 5 lines
(quoted diff) +0 -0
b/drivers/net/mlx5/mlx5_flow.c index 324349ed19..34b7a2f137 100644
drivers/net/mlx5/mlx5_flow.c +0 -0
@@ -3655,8 +3655,8 @@ flow_check_hairpin_split(struct rte_eth_dev

*dev,

Quoted 43 lines

Quoted 39 lines

drivers/net/mlx5/mlx5_flow.c +4 -4

              case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
                      raw_encap = actions->conf;
                      if (raw_encap->size >
-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4)))
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr)))
                              split++;
                      action_n++;
                      break;

@@ -4092,8 +4092,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
              case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
                      raw_encap = actions->conf;
                      if (raw_encap->size >
-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4))) {
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr))) {
                              memcpy(actions_tx, actions,
                                     sizeof(struct rte_flow_action));
                              actions_tx++; @@ -4107,8 +4107,8 @@

flow_hairpin_split(struct rte_eth_dev *dev,

              case RTE_FLOW_ACTION_TYPE_RAW_DECAP:
                      raw_decap = actions->conf;
                      if (raw_decap->size <

drivers/net/mlx5/mlx5_flow.c +2 -2

-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4))) {
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr))) {
                              memcpy(actions_tx, actions,
                                     sizeof(struct rte_flow_action));
                              actions_tx++; diff --git

a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h index 5fac8672fc..3b28c48a1d 100644

drivers/net/mlx5/mlx5_flow.h +0 -0

@@ -345,8 +345,8 @@ enum mlx5_feature_name {  #define

MLX5_GENEVE_OPT_LEN_0 14 #define MLX5_GENEVE_OPT_LEN_1 63

Quoted 1 line

-#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct

rte_flow_item_eth) + \

Quoted 2 lines

drivers/net/mlx5/mlx5_flow.h +1 -1

-                                       sizeof(struct rte_flow_item_ipv4))
+#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct

Quoted 8 lines

Quoted 5 lines
rte_ether_hdr) + \
Quoted 1 line
+ sizeof(struct rte_ipv4_hdr))
This constant should be used above in hairpin functions. Will change while merging.
+1

Why above the hairpin functions? This is correct for all encap decisions.

Best,
Ori

Thomas Monjalon

2020-11-23 08:13 UTC #7 lore raw

Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement

23/11/2020 08:54, Ori Kam:

Quoted 73 lines

From: Matan Azrad <[email protected]>

Quoted 72 lines

From: Thomas Monjalon

Quoted 19 lines

16/11/2020 08:55, Xiaoyu Min:
Quoted 18 lines
From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore.
Should use real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Signed-off-by Xiaoyu Min <[email protected]>
---
 drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
drivers/net/mlx5/mlx5_flow.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

Quoted 5 lines

drivers/net/mlx5/mlx5_flow.c flow_check_hairpin_split(struct rte_eth_dev
Quoted 5 lines
(quoted diff) +0 -0
b/drivers/net/mlx5/mlx5_flow.c index 324349ed19..34b7a2f137 100644
drivers/net/mlx5/mlx5_flow.c +0 -0
@@ -3655,8 +3655,8 @@ flow_check_hairpin_split(struct rte_eth_dev

*dev,

Quoted 46 lines

Quoted 39 lines

drivers/net/mlx5/mlx5_flow.c +4 -4

              case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
                      raw_encap = actions->conf;
                      if (raw_encap->size >
-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4)))
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr)))
                              split++;
                      action_n++;
                      break;

@@ -4092,8 +4092,8 @@ flow_hairpin_split(struct rte_eth_dev *dev,
              case RTE_FLOW_ACTION_TYPE_RAW_ENCAP:
                      raw_encap = actions->conf;
                      if (raw_encap->size >
-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4))) {
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr))) {
                              memcpy(actions_tx, actions,
                                     sizeof(struct rte_flow_action));
                              actions_tx++; @@ -4107,8 +4107,8 @@

flow_hairpin_split(struct rte_eth_dev *dev,

              case RTE_FLOW_ACTION_TYPE_RAW_DECAP:
                      raw_decap = actions->conf;
                      if (raw_decap->size <

drivers/net/mlx5/mlx5_flow.c +2 -2

-                         (sizeof(struct rte_flow_item_eth) +
-                          sizeof(struct rte_flow_item_ipv4))) {
+                         (sizeof(struct rte_ether_hdr) +
+                          sizeof(struct rte_ipv4_hdr))) {
                              memcpy(actions_tx, actions,
                                     sizeof(struct rte_flow_action));
                              actions_tx++; diff --git

a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h index 5fac8672fc..3b28c48a1d 100644

drivers/net/mlx5/mlx5_flow.h +0 -0

@@ -345,8 +345,8 @@ enum mlx5_feature_name {  #define

MLX5_GENEVE_OPT_LEN_0 14 #define MLX5_GENEVE_OPT_LEN_1 63

Quoted 1 line

-#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct

rte_flow_item_eth) + \

drivers/net/mlx5/mlx5_flow.h

Quoted 2 lines

drivers/net/mlx5/mlx5_flow.h +1 -1

-                                       sizeof(struct rte_flow_item_ipv4))
+#define MLX5_ENCAPSULATION_DECISION_SIZE (sizeof(struct

rte_ether_hdr) + \

Quoted 1 line

+ sizeof(struct rte_ipv4_hdr))

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
This constant should be used above in hairpin functions. Will change while merging.
+1
Why above the hairpin functions? This is correct for all encap decisions.

Other functions were already using this macro.
Only hairpin functions were not using it.
So I changed on apply.

Xiaoyu Min

author

2020-11-16 07:56 UTC #8 lore raw

PATCH 2/5 app/flow-perf: fix protocol size for raw encap

In reply to Xiaoyu Min

From: Xiaoyu Min <[email protected]>

The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore so add_*_header functions should use real header instead of the using rte_flow_item_* struct.

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.

Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

Signed-off-by Xiaoyu Min <[email protected]>

---
 app/test-flow-perf/actions_gen.c | 136 +++++++++++++++----------------
 1 file changed, 67 insertions(+), 69 deletions(-)

app/test-flow-perf/actions_gen.c +67 ~~-69~~

index e3a95d7ab2..ac525f6fdb 100644
@@ -12,6 +12,8 @@
 #include <rte_ethdev.h>
 #include <rte_vxlan.h>
 #include <rte_gtp.h>
+#include <rte_gre.h>
+#include <rte_geneve.h>
 
 #include "actions_gen.h"
 #include "flow_gen.h"

@@ -533,27 +535,27 @@ static void
 add_ether_header(uint8_t **header, uint64_t data,
 	__rte_unused struct additional_para para)
 {
-	struct rte_flow_item_eth eth_item;
+	struct rte_ether_hdr eth_hdr;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_ETH)))
 		return;
 
-	memset(&eth_item, 0, sizeof(struct rte_flow_item_eth));
+	memset(&eth_hdr, 0, sizeof(struct rte_ether_hdr));
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VLAN))
-		eth_item.type = RTE_BE16(RTE_ETHER_TYPE_VLAN);
+		eth_hdr.ether_type = RTE_BE16(RTE_ETHER_TYPE_VLAN);
 	else if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV4))
-		eth_item.type = RTE_BE16(RTE_ETHER_TYPE_IPV4);
+		eth_hdr.ether_type = RTE_BE16(RTE_ETHER_TYPE_IPV4);
 	else if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV6))
-		eth_item.type = RTE_BE16(RTE_ETHER_TYPE_IPV6);
-	memcpy(*header, &eth_item, sizeof(eth_item));
-	*header += sizeof(eth_item);
+		eth_hdr.ether_type = RTE_BE16(RTE_ETHER_TYPE_IPV6);
+	memcpy(*header, &eth_hdr, sizeof(eth_hdr));
+	*header += sizeof(eth_hdr);
 }
 
 static void
 add_vlan_header(uint8_t **header, uint64_t data,
 	__rte_unused struct additional_para para)
 {
-	struct rte_flow_item_vlan vlan_item;
+	struct rte_vlan_hdr vlan_hdr;
 	uint16_t vlan_value;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VLAN)))

@@ -561,22 +563,22 @@ add_vlan_header(uint8_t **header, uint64_t data,
 
 	vlan_value = VLAN_VALUE;
 
-	memset(&vlan_item, 0, sizeof(struct rte_flow_item_vlan));
-	vlan_item.tci = RTE_BE16(vlan_value);
+	memset(&vlan_hdr, 0, sizeof(struct rte_vlan_hdr));
+	vlan_hdr.vlan_tci = RTE_BE16(vlan_value);
 
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV4))
-		vlan_item.inner_type = RTE_BE16(RTE_ETHER_TYPE_IPV4);
+		vlan_hdr.eth_proto = RTE_BE16(RTE_ETHER_TYPE_IPV4);
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV6))
-		vlan_item.inner_type = RTE_BE16(RTE_ETHER_TYPE_IPV6);
-	memcpy(*header, &vlan_item, sizeof(vlan_item));
-	*header += sizeof(vlan_item);
+		vlan_hdr.eth_proto = RTE_BE16(RTE_ETHER_TYPE_IPV6);
+	memcpy(*header, &vlan_hdr, sizeof(vlan_hdr));
+	*header += sizeof(vlan_hdr);
 }
 
 static void
 add_ipv4_header(uint8_t **header, uint64_t data,
 	struct additional_para para)
 {
-	struct rte_flow_item_ipv4 ipv4_item;
+	struct rte_ipv4_hdr ipv4_hdr;
 	uint32_t ip_dst = para.counter;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV4)))

@@ -586,65 +588,64 @@ add_ipv4_header(uint8_t **header, uint64_t data,
 	if (FIXED_VALUES)
 		ip_dst = 1;
 
-	memset(&ipv4_item, 0, sizeof(struct rte_flow_item_ipv4));
-	ipv4_item.hdr.src_addr = RTE_IPV4(127, 0, 0, 1);
-	ipv4_item.hdr.dst_addr = RTE_BE32(ip_dst);
-	ipv4_item.hdr.version_ihl = RTE_IPV4_VHL_DEF;
+	memset(&ipv4_hdr, 0, sizeof(struct rte_ipv4_hdr));
+	ipv4_hdr.src_addr = RTE_IPV4(127, 0, 0, 1);
+	ipv4_hdr.dst_addr = RTE_BE32(ip_dst);
+	ipv4_hdr.version_ihl = RTE_IPV4_VHL_DEF;
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_UDP))
-		ipv4_item.hdr.next_proto_id = RTE_IP_TYPE_UDP;
+		ipv4_hdr.next_proto_id = RTE_IP_TYPE_UDP;
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GRE))
-		ipv4_item.hdr.next_proto_id = RTE_IP_TYPE_GRE;
-	memcpy(*header, &ipv4_item, sizeof(ipv4_item));
-	*header += sizeof(ipv4_item);
+		ipv4_hdr.next_proto_id = RTE_IP_TYPE_GRE;
+	memcpy(*header, &ipv4_hdr, sizeof(ipv4_hdr));
+	*header += sizeof(ipv4_hdr);
 }
 
 static void
 add_ipv6_header(uint8_t **header, uint64_t data,
 	__rte_unused struct additional_para para)
 {
-	struct rte_flow_item_ipv6 ipv6_item;
+	struct rte_ipv6_hdr ipv6_hdr;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV6)))
 		return;
 
-	memset(&ipv6_item, 0, sizeof(struct rte_flow_item_ipv6));
+	memset(&ipv6_hdr, 0, sizeof(struct rte_ipv6_hdr));
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_UDP))
-		ipv6_item.hdr.proto = RTE_IP_TYPE_UDP;
+		ipv6_hdr.proto = RTE_IP_TYPE_UDP;
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GRE))
-		ipv6_item.hdr.proto = RTE_IP_TYPE_GRE;
-	memcpy(*header, &ipv6_item, sizeof(ipv6_item));
-	*header += sizeof(ipv6_item);
+		ipv6_hdr.proto = RTE_IP_TYPE_GRE;
+	memcpy(*header, &ipv6_hdr, sizeof(ipv6_hdr));
+	*header += sizeof(ipv6_hdr);
 }
 
 static void
 add_udp_header(uint8_t **header, uint64_t data,
 	__rte_unused struct additional_para para)
 {
-	struct rte_flow_item_udp udp_item;
+	struct rte_udp_hdr udp_hdr;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_UDP)))
 		return;
 
-	memset(&udp_item, 0, sizeof(struct rte_flow_item_udp));
+	memset(&udp_hdr, 0, sizeof(struct rte_flow_item_udp));
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN))
-		udp_item.hdr.dst_port = RTE_BE16(RTE_VXLAN_DEFAULT_PORT);
+		udp_hdr.dst_port = RTE_BE16(RTE_VXLAN_DEFAULT_PORT);
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN_GPE))
-		udp_item.hdr.dst_port = RTE_BE16(RTE_VXLAN_GPE_UDP_PORT);
+		udp_hdr.dst_port = RTE_BE16(RTE_VXLAN_GPE_UDP_PORT);
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GENEVE))
-		udp_item.hdr.dst_port = RTE_BE16(RTE_GENEVE_UDP_PORT);
+		udp_hdr.dst_port = RTE_BE16(RTE_GENEVE_UDP_PORT);
 	if (data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GTP))
-		udp_item.hdr.dst_port = RTE_BE16(RTE_GTPU_UDP_PORT);
-	 memcpy(*header, &udp_item, sizeof(udp_item));
-	 *header += sizeof(udp_item);
+		udp_hdr.dst_port = RTE_BE16(RTE_GTPU_UDP_PORT);
+	 memcpy(*header, &udp_hdr, sizeof(udp_hdr));
+	 *header += sizeof(udp_hdr);
 }
 
 static void
 add_vxlan_header(uint8_t **header, uint64_t data,
 	struct additional_para para)
 {
-	struct rte_flow_item_vxlan vxlan_item;
+	struct rte_vxlan_hdr vxlan_hdr;
 	uint32_t vni_value = para.counter;
-	uint8_t i;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN)))
 		return;

@@ -653,23 +654,21 @@ add_vxlan_header(uint8_t **header, uint64_t data,
 	if (FIXED_VALUES)
 		vni_value = 1;
 
-	memset(&vxlan_item, 0, sizeof(struct rte_flow_item_vxlan));
+	memset(&vxlan_hdr, 0, sizeof(struct rte_vxlan_hdr));
 
-	for (i = 0; i < 3; i++)
-		vxlan_item.vni[2 - i] = vni_value >> (i * 8);
-	vxlan_item.flags = 0x8;
+	vxlan_hdr.vx_vni = (RTE_BE32(vni_value)) >> 16;
+	vxlan_hdr.vx_flags = 0x8;
 
-	memcpy(*header, &vxlan_item, sizeof(vxlan_item));
-	*header += sizeof(vxlan_item);
+	memcpy(*header, &vxlan_hdr, sizeof(vxlan_hdr));
+	*header += sizeof(vxlan_hdr);
 }
 
 static void
 add_vxlan_gpe_header(uint8_t **header, uint64_t data,
 	struct additional_para para)
 {
-	struct rte_flow_item_vxlan_gpe vxlan_gpe_item;
+	struct rte_vxlan_gpe_hdr vxlan_gpe_hdr;
 	uint32_t vni_value = para.counter;
-	uint8_t i;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN_GPE)))
 		return;

@@ -678,38 +677,37 @@ add_vxlan_gpe_header(uint8_t **header, uint64_t data,
 	if (FIXED_VALUES)
 		vni_value = 1;
 
-	memset(&vxlan_gpe_item, 0, sizeof(struct rte_flow_item_vxlan_gpe));
+	memset(&vxlan_gpe_hdr, 0, sizeof(struct rte_vxlan_gpe_hdr));
 
-	for (i = 0; i < 3; i++)
-		vxlan_gpe_item.vni[2 - i] = vni_value >> (i * 8);
-	vxlan_gpe_item.flags = 0x0c;
+	vxlan_gpe_hdr.vx_vni = (RTE_BE32(vni_value)) >> 16;
+	vxlan_gpe_hdr.vx_flags = 0x0c;
 
-	memcpy(*header, &vxlan_gpe_item, sizeof(vxlan_gpe_item));
-	*header += sizeof(vxlan_gpe_item);
+	memcpy(*header, &vxlan_gpe_hdr, sizeof(vxlan_gpe_hdr));
+	*header += sizeof(vxlan_gpe_hdr);
 }
 
 static void
 add_gre_header(uint8_t **header, uint64_t data,
 	__rte_unused struct additional_para para)
 {
-	struct rte_flow_item_gre gre_item;
+	struct rte_gre_hdr gre_hdr;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GRE)))
 		return;
 
-	memset(&gre_item, 0, sizeof(struct rte_flow_item_gre));
+	memset(&gre_hdr, 0, sizeof(struct rte_gre_hdr));
 
-	gre_item.protocol = RTE_BE16(RTE_ETHER_TYPE_TEB);
+	gre_hdr.proto = RTE_BE16(RTE_ETHER_TYPE_TEB);
 
-	memcpy(*header, &gre_item, sizeof(gre_item));
-	*header += sizeof(gre_item);
+	memcpy(*header, &gre_hdr, sizeof(gre_hdr));
+	*header += sizeof(gre_hdr);
 }
 
 static void
 add_geneve_header(uint8_t **header, uint64_t data,
 	struct additional_para para)
 {
-	struct rte_flow_item_geneve geneve_item;
+	struct rte_geneve_hdr geneve_hdr;
 	uint32_t vni_value = para.counter;
 	uint8_t i;

@@ -720,20 +718,20 @@ add_geneve_header(uint8_t **header, uint64_t data,
 	if (FIXED_VALUES)
 		vni_value = 1;
 
-	memset(&geneve_item, 0, sizeof(struct rte_flow_item_geneve));
+	memset(&geneve_hdr, 0, sizeof(struct rte_geneve_hdr));
 
 	for (i = 0; i < 3; i++)
-		geneve_item.vni[2 - i] = vni_value >> (i * 8);
+		geneve_hdr.vni[2 - i] = vni_value >> (i * 8);
 
-	memcpy(*header, &geneve_item, sizeof(geneve_item));
-	*header += sizeof(geneve_item);
+	memcpy(*header, &geneve_hdr, sizeof(geneve_hdr));
+	*header += sizeof(geneve_hdr);
 }
 
 static void
 add_gtp_header(uint8_t **header, uint64_t data,
 	struct additional_para para)
 {
-	struct rte_flow_item_gtp gtp_item;
+	struct rte_gtp_hdr gtp_hdr;
 	uint32_t teid_value = para.counter;
 
 	if (!(data & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GTP)))

@@ -743,13 +741,13 @@ add_gtp_header(uint8_t **header, uint64_t data,
 	if (FIXED_VALUES)
 		teid_value = 1;
 
-	memset(&gtp_item, 0, sizeof(struct rte_flow_item_gtp));
+	memset(&gtp_hdr, 0, sizeof(struct rte_flow_item_gtp));
 
-	gtp_item.teid = RTE_BE32(teid_value);
-	gtp_item.msg_type = 255;
+	gtp_hdr.teid = RTE_BE32(teid_value);
+	gtp_hdr.msg_type = 255;
 
-	memcpy(*header, &gtp_item, sizeof(gtp_item));
-	*header += sizeof(gtp_item);
+	memcpy(*header, &gtp_hdr, sizeof(gtp_hdr));
+	*header += sizeof(gtp_hdr);
 }
 
 static const struct encap_decap_headers {

Signature · 1 line

2.25.1

Xiaoyu Min

author

2020-11-16 07:56 UTC #9 lore raw

PATCH 3/5 net/bnxt: fix protocol size for VXLAN encap copy

In reply to Xiaoyu Min

From: Xiaoyu Min <[email protected]>

The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.

Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

Signed-off-by Xiaoyu Min <[email protected]>

---
 drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +6 -5

index a54c55c5f5..823eeb21b8 100644
@@ -3,6 +3,7 @@
  * All rights reserved.
  */
 
+#include <rte_vxlan.h>
 #include "bnxt.h"
 #include "ulp_template_db_enum.h"
 #include "ulp_template_struct.h"

@@ -1548,7 +1549,7 @@ ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item,
 		buff = &ap->act_details[BNXT_ULP_ACT_PROP_IDX_ENCAP_VTAG];
 		ulp_encap_buffer_copy(buff,
 				      item->spec,
-				      sizeof(struct rte_flow_item_vlan),
+				      sizeof(struct rte_vlan_hdr),
 				      ULP_BUFFER_ALIGN_8_BYTE);
 
 		if (!ulp_rte_item_skip_void(&item, 1))

@@ -1559,15 +1560,15 @@ ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item,
 	if (item->type == RTE_FLOW_ITEM_TYPE_VLAN) {
 		vlan_num++;
 		memcpy(&ap->act_details[BNXT_ULP_ACT_PROP_IDX_ENCAP_VTAG +
-		       sizeof(struct rte_flow_item_vlan)],
+		       sizeof(struct rte_vlan_hdr)],
 		       item->spec,
-		       sizeof(struct rte_flow_item_vlan));
+		       sizeof(struct rte_vlan_hdr));
 		if (!ulp_rte_item_skip_void(&item, 1))
 			return BNXT_TF_RC_ERROR;
 	}
 	/* Update the vlan count and size of more than one */
 	if (vlan_num) {
-		vlan_size = vlan_num * sizeof(struct rte_flow_item_vlan);
+		vlan_size = vlan_num * sizeof(struct rte_vlan_hdr);
 		vlan_num = tfp_cpu_to_be_32(vlan_num);
 		memcpy(&ap->act_details[BNXT_ULP_ACT_PROP_IDX_ENCAP_VTAG_NUM],
 		       &vlan_num,

@@ -1726,7 +1727,7 @@ ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item,
 		BNXT_TF_DBG(ERR, "vxlan encap does not have vni\n");
 		return BNXT_TF_RC_ERROR;
 	}
-	vxlan_size = sizeof(struct rte_flow_item_vxlan);
+	vxlan_size = sizeof(struct rte_vxlan_hdr);
 	/* copy the vxlan details */
 	memcpy(&vxlan_spec, item->spec, vxlan_size);
 	vxlan_spec.flags = 0x08;

Signature · 1 line

2.25.1

Ferruh Yigit

2020-11-16 16:13 UTC #10 lore raw

Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy

Quoted 11 lines · Xiaoyu Min

From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")
Signed-off-by Xiaoyu Min <[email protected]>

<...>

drivers/net/bnxt/tf_ulp/ulp_rte_parser.c ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item, · line 1733

Quoted 10 lines

drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +1 -1

@@ -1726,7 +1727,7 @@ ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item,
  		BNXT_TF_DBG(ERR, "vxlan encap does not have vni\n");
  		return BNXT_TF_RC_ERROR;
  	}
-	vxlan_size = sizeof(struct rte_flow_item_vxlan);
+	vxlan_size = sizeof(struct rte_vxlan_hdr);
  	/* copy the vxlan details */
  	memcpy(&vxlan_spec, item->spec, vxlan_size);
  	vxlan_spec.flags = 0x08;

'vxlan_size' seems used both to copy rt_flow_item [1] and to header [2]. Also ''vxlan_size' is used to copy the 'vxlan_spec'.

Since both "struct rte_flow_item_vxlan" & "struct rte_vxlan_hdr" size is same, this should work fine, but I guess it may be broken if sizes of those two structures changes in the future.

[1]
memcpy(&vxlan_spec, item->spec, vxlan_size);

[2]
ulp_encap_buffer_copy(buff, (const uint8_t *)&vxlan_spec,

	vxlan_size, ULP_BUFFER_ALIGN_8_BYTE);

Ajit Khaparde

2020-11-18 00:34 UTC #11 lore raw

Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy

Quoted 37 lines · Ferruh Yigit

Quoted 9 lines · Xiaoyu Min
From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Quoted 2 lines
Signed-off-by Xiaoyu Min <[email protected]>
<...>
Review ulp_rte_vxlan_encap_act_handler(const struct
Quoted 1 line
(quoted diff) +0 -0
@@ -1726,7 +1727,7 @@ ulp_rte_vxlan_encap_act_handler(const struct
rte_flow_action *action_item,
drivers/net/bnxt/tf_ulp/ulp_rte_parser.c ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item, · line 1733
Quoted 9 lines
drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +1 -1
              BNXT_TF_DBG(ERR, "vxlan encap does not have vni\n");
              return BNXT_TF_RC_ERROR;
      }
-     vxlan_size = sizeof(struct rte_flow_item_vxlan);
+     vxlan_size = sizeof(struct rte_vxlan_hdr);
      /* copy the vxlan details */
      memcpy(&vxlan_spec, item->spec, vxlan_size);
      vxlan_spec.flags = 0x08;
'vxlan_size' seems used both to copy rt_flow_item [1] and to header [2]. Also
''vxlan_size' is used to copy the 'vxlan_spec'.
Since both "struct rte_flow_item_vxlan" & "struct rte_vxlan_hdr" size is same,
this should work fine, but I guess it may be broken if sizes of those two structures changes in the future.

Quoted 7 lines

[1]
memcpy(&vxlan_spec, item->spec, vxlan_size);
[2]
ulp_encap_buffer_copy(buff, (const uint8_t *)&vxlan_spec,
        vxlan_size, ULP_BUFFER_ALIGN_8_BYTE);

Also, I feel rte_flow_item_vxlan is a control plane structure while rte_vlan_hdr is more for dataplane.
It's better to keep them separate to allow refining them independently.

Andrew Rybchenko

2020-11-18 06:37 UTC #12 lore raw

Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy

Quoted 42 lines · Ajit Khaparde

Quoted 40 lines · Ferruh Yigit
Quoted 9 lines · Xiaoyu Min
From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Quoted 2 lines
Signed-off-by Xiaoyu Min <[email protected]>
<...>
Review ulp_rte_vxlan_encap_act_handler(const struct
Quoted 1 line
(quoted diff) +0 -0
@@ -1726,7 +1727,7 @@ ulp_rte_vxlan_encap_act_handler(const struct
rte_flow_action *action_item,
drivers/net/bnxt/tf_ulp/ulp_rte_parser.c ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item, · line 1733
Quoted 9 lines
drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +1 -1
              BNXT_TF_DBG(ERR, "vxlan encap does not have vni\n");
              return BNXT_TF_RC_ERROR;
      }
-     vxlan_size = sizeof(struct rte_flow_item_vxlan);
+     vxlan_size = sizeof(struct rte_vxlan_hdr);
      /* copy the vxlan details */
      memcpy(&vxlan_spec, item->spec, vxlan_size);
      vxlan_spec.flags = 0x08;
'vxlan_size' seems used both to copy rt_flow_item [1] and to header [2]. Also
''vxlan_size' is used to copy the 'vxlan_spec'.
Since both "struct rte_flow_item_vxlan" & "struct rte_vxlan_hdr" size is same,
this should work fine, but I guess it may be broken if sizes of those two structures changes in the future.
[1]
memcpy(&vxlan_spec, item->spec, vxlan_size);

Quoted 7 lines

Quoted 4 lines
[2]
ulp_encap_buffer_copy(buff, (const uint8_t *)&vxlan_spec,
        vxlan_size, ULP_BUFFER_ALIGN_8_BYTE);
Also, I feel rte_flow_item_vxlan is a control plane structure while rte_vlan_hdr is more for dataplane.
It's better to keep them separate to allow refining them independently.

I disagree with the point. It is typical duplication which should be fixed. VXLAN item should consist of rte_vxlan_hdr plus extra non header fields. In fact, similar thing should be done for Ethernet flow item spec.

These items are used for VXLAN encap action and in current state of definitions it is tricky to use VXLAN and Ethernet items to construct header to be prepended. It is required to copy field by field since it is bad to rely on the fact that flow item starts from protocol header if it is not specified in corresponding C type definition.

Ajit Khaparde

Acked-by

2020-11-18 17:44 UTC #13 lore raw

Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy

Quoted 57 lines · Andrew Rybchenko

Quoted 42 lines · Ajit Khaparde
Quoted 40 lines · Ferruh Yigit
Quoted 9 lines · Xiaoyu Min
From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items are refined. The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Quoted 2 lines
Signed-off-by Xiaoyu Min <[email protected]>
<...>
Review ulp_rte_vxlan_encap_act_handler(const struct
Quoted 1 line
(quoted diff) +0 -0
@@ -1726,7 +1727,7 @@ ulp_rte_vxlan_encap_act_handler(const struct
rte_flow_action *action_item,
drivers/net/bnxt/tf_ulp/ulp_rte_parser.c ulp_rte_vxlan_encap_act_handler(const struct rte_flow_action *action_item, · line 1733
Quoted 9 lines
drivers/net/bnxt/tf_ulp/ulp_rte_parser.c +1 -1
              BNXT_TF_DBG(ERR, "vxlan encap does not have vni\n");
              return BNXT_TF_RC_ERROR;
      }
-     vxlan_size = sizeof(struct rte_flow_item_vxlan);
+     vxlan_size = sizeof(struct rte_vxlan_hdr);
      /* copy the vxlan details */
      memcpy(&vxlan_spec, item->spec, vxlan_size);
      vxlan_spec.flags = 0x08;
'vxlan_size' seems used both to copy rt_flow_item [1] and to header [2]. Also
''vxlan_size' is used to copy the 'vxlan_spec'.
Since both "struct rte_flow_item_vxlan" & "struct rte_vxlan_hdr" size is same,
this should work fine, but I guess it may be broken if sizes of those two structures changes in the future.
[1]
memcpy(&vxlan_spec, item->spec, vxlan_size);
Quoted 7 lines
Quoted 4 lines
[2]
ulp_encap_buffer_copy(buff, (const uint8_t *)&vxlan_spec,
        vxlan_size, ULP_BUFFER_ALIGN_8_BYTE);
Also, I feel rte_flow_item_vxlan is a control plane structure while rte_vlan_hdr is more for dataplane.
It's better to keep them separate to allow refining them independently.
I disagree with the point. It is typical duplication which should be fixed. VXLAN item should consist of rte_vxlan_hdr plus extra non header fields. In fact, similar thing should be done for Ethernet flow item spec.

Quoted 6 lines

These items are used for VXLAN encap action and in current state of definitions it is tricky to use VXLAN and Ethernet items to construct header to be prepended. It is required to copy field by field since it is bad to rely on the fact that flow item starts from protocol header if it is not specified in corresponding C type definition.

I took a look at the change again. I had an impression that it is changing from specific protocol header to rte flow item, not the other way around. This is fine.
Note that the commit is changing VXLAN and VLAN. But the log suggests only VXLAN. Other than that,

Acked-by Ajit Khaparde <[email protected]>

Xiaoyu Min

author

2020-11-16 07:56 UTC #14 lore raw

PATCH 4/5 net/iavf: fix protocol size for virtchnl copy

In reply to Xiaoyu Min

From: Xiaoyu Min <[email protected]>

The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.

Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

Signed-off-by Xiaoyu Min <[email protected]>

---
 drivers/net/iavf/iavf_fdir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/iavf/iavf_fdir.c +1 -1

index d683a468c1..7054bde0b9 100644
@@ -541,7 +541,7 @@ iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter *ad,
 				VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr, ETH, ETHERTYPE);
 
 				rte_memcpy(hdr->buffer,
-					eth_spec, sizeof(*eth_spec));
+					eth_spec, sizeof(struct rte_ether_hdr));
 			}
 
 			filter->add_fltr.rule_cfg.proto_hdrs.count = ++layer;

Signature · 1 line

2.25.1

Ferruh Yigit

2020-11-16 16:23 UTC #15 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

Quoted 20 lines · Xiaoyu Min

From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")
Signed-off-by Xiaoyu Min <[email protected]>
---
  drivers/net/iavf/iavf_fdir.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/iavf/iavf_fdir.c +0 -0
index d683a468c1..7054bde0b9 100644

drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter *ad, · line 544

Quoted 5 lines

drivers/net/iavf/iavf_fdir.c +1 -1

@@ -541,7 +541,7 @@ iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter *ad,
  				VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr, ETH, ETHERTYPE);
 
  				rte_memcpy(hdr->buffer,
-					eth_spec, sizeof(*eth_spec));
+					eth_spec, sizeof(struct rte_ether_hdr));

This requires 'struct rte_flow_item_eth' should have 'struct rte_ether_hdr' as first element, and I suspect this usage exists in a few more locations, but I wonder if this assumption is real and documented in somewhere? I am not just talking about 'struct rte_flow_item_eth', but for all 'rte_flow_item_*'...

btw, while checking for the 'struct rte_flow_item_eth', pahole shows it is using 20 bytes, and I suspect this is not the intention with the reserved field:

struct rte_flow_item_eth {
	struct rte_ether_addr      dst;                  /*     0     6 */
	struct rte_ether_addr      src;                  /*     6     6 */
	uint16_t                   type;                 /*    12     2 */

	/* Bitfield combined with previous fields */

	uint32_t                   has_vlan:1;           /*    12:15  4 */

	/* XXX 31 bits hole, try to pack */

	uint32_t                   reserved:31;          /*    16: 1  4 */

	/* size: 20, cachelines: 1, members: 5 */
	/* bit holes: 1, sum bit holes: 31 bits */
	/* bit_padding: 1 bits */
	/* last cacheline: 20 bytes */
};

'has_vlan' seems combined with previous field to make together 32 bits. So the 'reserved' field is occupying a new 32 bits all by itself.

What about changing the struct as following, while we can change the ABI:
struct rte_flow_item_eth {

	struct rte_ether_addr      dst;                  /*     0     6 */
	struct rte_ether_addr      src;                  /*     6     6 */
	uint16_t                   type;                 /*    12     2 */
	uint16_t                   has_vlan:1;           /*    14:15  2 */
	uint16_t                   reserved:15;          /*    14: 0  2 */

	/* size: 16, cachelines: 1, members: 5 */
	/* last cacheline: 16 bytes */
};

Jack Min

2020-11-22 13:28 UTC #16 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

Quoted 37 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Tuesday, November 17, 2020 00:23
To: Xiaoyu Min <[email protected]>; Jingjing Wu <[email protected]>; Beilei Xing <[email protected]>
Cc: [email protected]; Jack Min <[email protected]>; NBU-Contact-Thomas Monjalon <[email protected]>; Andrew Rybchenko <[email protected]>; Ori Kam <[email protected]>; Dekel Peled <[email protected]>
Subject: Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
Quoted 9 lines · Xiaoyu Min
From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct
Quoted 11 lines
Signed-off-by Xiaoyu Min <[email protected]>
---
  drivers/net/iavf/iavf_fdir.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/iavf/iavf_fdir.c +0 -0
index d683a468c1..7054bde0b9 100644
@@ -541,7 +541,7 @@ iavf_fdir_parse_pattern(__rte_unused struct
iavf_adapter *ad,
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct · line 541
Quoted 1 line
drivers/net/iavf/iavf_fdir.c +0 -0
  				VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr,
ETH, ETHERTYPE);
Quoted 1 line

Quoted 10 lines

drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter *ad, · line 544
Quoted 3 lines
rte_memcpy(hdr->buffer,
drivers/net/iavf/iavf_fdir.c +1 -1
-					eth_spec, sizeof(*eth_spec));
+					eth_spec, sizeof(struct rte_ether_hdr));
This requires 'struct rte_flow_item_eth' should have 'struct rte_ether_hdr' as first element, and I suspect this usage exists in a few more locations, but I wonder if this assumption is real and documented in somewhere?
I am not just talking about 'struct rte_flow_item_eth', but for all 'rte_flow_item_*'...

I think this is not documented and this assumption is not real.
I've created one ticket on Bugzilla (https://bugs.dpdk.org/show_bug.cgi?id=581) to track.

Quoted 35 lines

btw, while checking for the 'struct rte_flow_item_eth', pahole shows it is using 20 bytes, and I suspect this is not the intention with the reserved field:

struct rte_flow_item_eth {
	struct rte_ether_addr      dst;                  /*     0     6 */
	struct rte_ether_addr      src;                  /*     6     6 */
	uint16_t                   type;                 /*    12     2 */

	/* Bitfield combined with previous fields */

	uint32_t                   has_vlan:1;           /*    12:15  4 */

	/* XXX 31 bits hole, try to pack */

	uint32_t                   reserved:31;          /*    16: 1  4 */

	/* size: 20, cachelines: 1, members: 5 */
	/* bit holes: 1, sum bit holes: 31 bits */
	/* bit_padding: 1 bits */
	/* last cacheline: 20 bytes */
};

'has_vlan' seems combined with previous field to make together 32 bits. So the 'reserved' field is occupying a new 32 bits all by itself.

What about changing the struct as following, while we can change the ABI:
struct rte_flow_item_eth {

	struct rte_ether_addr      dst;                  /*     0     6 */
	struct rte_ether_addr      src;                  /*     6     6 */
	uint16_t                   type;                 /*    12     2 */
	uint16_t                   has_vlan:1;           /*    14:15  2 */
	uint16_t                   reserved:15;          /*    14: 0  2 */

Quoted 4 lines

	/* size: 16, cachelines: 1, members: 5 */
	/* last cacheline: 16 bytes */
};

Well we probably need to discuss this in next release. It's too late to change this API at this moment.

-Jack

Thomas Monjalon

2020-11-22 14:15 UTC #17 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

22/11/2020 14:28, Jack Min:

Quoted 37 lines

From: Ferruh Yigit <[email protected]>
Quoted 26 lines · Xiaoyu Min
Quoted 9 lines
From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct
Quoted 11 lines
Signed-off-by Xiaoyu Min <[email protected]>
---
  drivers/net/iavf/iavf_fdir.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/iavf/iavf_fdir.c +0 -0
index d683a468c1..7054bde0b9 100644
@@ -541,7 +541,7 @@ iavf_fdir_parse_pattern(__rte_unused struct
iavf_adapter *ad,
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct · line 541
Quoted 1 line
drivers/net/iavf/iavf_fdir.c +0 -0
  				VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr,
ETH, ETHERTYPE);
Quoted 1 line
Quoted 10 lines
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter *ad, · line 544
Quoted 3 lines
rte_memcpy(hdr->buffer,
drivers/net/iavf/iavf_fdir.c +1 -1
-					eth_spec, sizeof(*eth_spec));
+					eth_spec, sizeof(struct rte_ether_hdr));
This requires 'struct rte_flow_item_eth' should have 'struct rte_ether_hdr' as first element, and I suspect this usage exists in a few more locations, but I wonder if this assumption is real and documented in somewhere?
I am not just talking about 'struct rte_flow_item_eth', but for all 'rte_flow_item_*'...

Quoted 2 lines

I think this is not documented and this assumption is not real.
I've created one ticket on Bugzilla (https://bugs.dpdk.org/show_bug.cgi?id=581) to track.

So it probably means this patch is not enough for iavf. It would require iavf maintainers (Cc'ed) to follow up. I will apply the rest of the series.

Ferruh Yigit

2020-11-23 10:03 UTC #18 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

Quoted 38 lines · Thomas Monjalon

22/11/2020 14:28, Jack Min:
Quoted 37 lines
From: Ferruh Yigit <[email protected]>
Quoted 36 lines · Xiaoyu Min
Quoted 9 lines
From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct
Quoted 11 lines
Signed-off-by Xiaoyu Min <[email protected]>
---
   drivers/net/iavf/iavf_fdir.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/iavf/iavf_fdir.c +0 -0
index d683a468c1..7054bde0b9 100644
@@ -541,7 +541,7 @@ iavf_fdir_parse_pattern(__rte_unused struct
iavf_adapter *ad,
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct · line 541
Quoted 1 line
drivers/net/iavf/iavf_fdir.c +0 -0
   				VIRTCHNL_ADD_PROTO_HDR_FIELD_BIT(hdr,
ETH, ETHERTYPE);
drivers/net/iavf/iavf_fdir.c iavf_fdir_parse_pattern(__rte_unused struct iavf_adapter *ad, · line 544
Quoted 4 lines
drivers/net/iavf/iavf_fdir.c +1 -1
   				rte_memcpy(hdr->buffer,
-					eth_spec, sizeof(*eth_spec));
+					eth_spec, sizeof(struct rte_ether_hdr));
This requires 'struct rte_flow_item_eth' should have 'struct rte_ether_hdr' as first element, and I suspect this usage exists in a few more locations, but I wonder if this assumption is real and documented in somewhere?
I am not just talking about 'struct rte_flow_item_eth', but for all 'rte_flow_item_*'...

Quoted 7 lines

Quoted 2 lines
I think this is not documented and this assumption is not real.
I've created one ticket on Bugzilla (https://bugs.dpdk.org/show_bug.cgi?id=581) to track.
So it probably means this patch is not enough for iavf. It would require iavf maintainers (Cc'ed) to follow up. I will apply the rest of the series.

I think the iavf patch is OK.

My comment was general, on how new fields added to the "struct rte_flow_item_eth", which is causing 4 bytes of waste unintentionally.

And if we don't fix it in this release, we won't able to fix it until next release.

I think first thing is to get the iavf fix.

Later we can discuss to get the struct change in this release or not.

Ferruh Yigit

Reviewed-by on v1 4/5

2020-11-23 10:50 UTC #19 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

In reply to Xiaoyu Min

Quoted 11 lines · Xiaoyu Min

From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")
Signed-off-by Xiaoyu Min <[email protected]>

Reviewed-by Ferruh Yigit <[email protected]>

Thomas Monjalon

Applied

2020-11-24 21:58 UTC #20 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

23/11/2020 11:49, Ferruh Yigit:

Quoted 7 lines · Xiaoyu Min

Quoted 6 lines
From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.

Quoted 7 lines

Quoted 5 lines
Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")
Signed-off-by Xiaoyu Min <[email protected]>
Reviewed-by Ferruh Yigit <[email protected]>

Applied

Xing, Beilei

Acked-by on v1 4/5

2020-11-27 01:17 UTC #21 lore raw

Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy

In reply to Xiaoyu Min

Quoted 15 lines

-----Original Message-----
From: Xiaoyu Min <[email protected]>
Sent: Monday, November 16, 2020 3:55 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Xiaoyu Min <[email protected]>
Subject: [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
From: Xiaoyu Min <[email protected]>
The rte_flow_item_vlan items are refined.
The structs do not exactly represent the packet bits captured on the wire anymore so should only copy real header instead of the whole struct.

Quoted 6 lines

Replace the rte_flow_item_* with the existing corresponding rte_*_hdr.
Fixes 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
Signed-off-by Xiaoyu Min <[email protected]>

Acked-by Beilei Xing <[email protected]>

Xiaoyu Min

author

2020-11-16 07:56 UTC #22 lore raw

PATCH 5/5 net/softnic: update headers size calculation

In reply to Xiaoyu Min

From: Dekel Peled <[email protected]>

The rte_flow_item_eth and rte_flow_item_vlan items were updated in [1]. The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header data. The size to use for match should be the header data size only, and not the size of the whole struct.

This patch replaces the rte_flow_item_* with the corresponding rte_*_hdr.

[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6 item")

Signed-off-by Dekel Peled <[email protected]>

---
 drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/net/softnic/rte_eth_softnic_flow.c +4 -4

index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum rte_flow_item_type type,
 
 	case RTE_FLOW_ITEM_TYPE_ETH:
 		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
 		return 1; /* TRUE */
 
 	case RTE_FLOW_ITEM_TYPE_VLAN:
 		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
 		return 1;
 
 	case RTE_FLOW_ITEM_TYPE_IPV4:
 		*mask = &rte_flow_item_ipv4_mask;
-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
 		return 1;
 
 	case RTE_FLOW_ITEM_TYPE_IPV6:
 		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
 		return 1;
 
 	case RTE_FLOW_ITEM_TYPE_ICMP:

Signature · 1 line

2.25.1

Ferruh Yigit

2020-11-16 16:27 UTC #23 lore raw

Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

Quoted 40 lines · Xiaoyu Min

From: Dekel Peled <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items were updated in [1]. The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header data. The size to use for match should be the header data size only, and not the size of the whole struct.
This patch replaces the rte_flow_item_* with the corresponding rte_*_hdr.
[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")
[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6 item")
Signed-off-by Dekel Peled <[email protected]>
---
  drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum rte_flow_item_type type,
 
  	case RTE_FLOW_ITEM_TYPE_ETH:
  		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
  		return 1; /* TRUE */
 
  	case RTE_FLOW_ITEM_TYPE_VLAN:
  		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
  		return 1;
 
  	case RTE_FLOW_ITEM_TYPE_IPV4:
  		*mask = &rte_flow_item_ipv4_mask;

drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum rte_flow_item_type type, · line 189

Quoted 10 lines

drivers/net/softnic/rte_eth_softnic_flow.c +2 -2

-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
  		return 1;
 
  	case RTE_FLOW_ITEM_TYPE_IPV6:
  		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
  		return 1;

As far as I can see the 'flow_item_is_proto' sets the size to be used over 'rte_flow_item_*" types, the original values seems correct to me, am I missing something.

Can you please elaborate why the change is needed?

Dekel Peled

2020-11-16 19:09 UTC #24 lore raw

Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

Hi Ferruh,

The failure occur at hash_key_mask_is_same() which uses the size returned by flow_item_is_proto() for comparison. Since the rte_flow_item_* of some types is now different than the rte_*_hdr size of these types, need to use the rte_*_hdr size.

Regards,
Dekel

Quoted 73 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Monday, November 16, 2020 6:27 PM
To: Xiaoyu Min <[email protected]>; Jasvinder Singh <[email protected]>; Cristian Dumitrescu <[email protected]>
Cc: [email protected]; Dekel Peled <[email protected]>
Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
Quoted 3 lines · Xiaoyu Min
From: Dekel Peled <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items were updated in
[1].
Quoted 6 lines
The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header data. The size to use for match should be the header data size only, and not the size of the whole struct.
This patch replaces the rte_flow_item_* with the corresponding
rte_*_hdr.
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum rte_flow_item_type
Quoted 19 lines
[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6 item")
Signed-off-by Dekel Peled <[email protected]>
---
  drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/softnic/rte_eth_softnic_flow.c +0 -0
index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum rte_flow_item_type
type,
Quoted 15 lines
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
  	case RTE_FLOW_ITEM_TYPE_ETH:
  		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
  		return 1; /* TRUE */
 
  	case RTE_FLOW_ITEM_TYPE_VLAN:
  		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
  		return 1;
 
  	case RTE_FLOW_ITEM_TYPE_IPV4:
  		*mask = &rte_flow_item_ipv4_mask;
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum rte_flow_item_type type, · line 189
Quoted 10 lines
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
  		return 1;
 
  	case RTE_FLOW_ITEM_TYPE_IPV6:
  		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
  		return 1;
 
As far as I can see the 'flow_item_is_proto' sets the size to be used over 'rte_flow_item_*" types, the original values seems correct to me, am I missing something.
Can you please elaborate why the change is needed?

Ferruh Yigit

2020-11-17 10:30 UTC #25 lore raw

Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

On 11/16/2020 7:09 PM, Dekel Peled wrote:

< Please don't top post, reply moved to the bottom >

Quoted 72 lines

Quoted 72 lines
-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Monday, November 16, 2020 6:27 PM
To: Xiaoyu Min <[email protected]>; Jasvinder Singh <[email protected]>; Cristian Dumitrescu <[email protected]>
Cc: [email protected]; Dekel Peled <[email protected]>
Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
Quoted 3 lines · Xiaoyu Min
From: Dekel Peled <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items were updated in
[1].
Quoted 6 lines
The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header data. The size to use for match should be the header data size only, and not the size of the whole struct.
This patch replaces the rte_flow_item_* with the corresponding
rte_*_hdr.
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum rte_flow_item_type
Quoted 19 lines
[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6 item")
Signed-off-by Dekel Peled <[email protected]>
---
   drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
   1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/softnic/rte_eth_softnic_flow.c +0 -0
index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum rte_flow_item_type
type,
Quoted 15 lines
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
   	case RTE_FLOW_ITEM_TYPE_ETH:
   		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
   		return 1; /* TRUE */
 
   	case RTE_FLOW_ITEM_TYPE_VLAN:
   		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
   		return 1;
 
   	case RTE_FLOW_ITEM_TYPE_IPV4:
   		*mask = &rte_flow_item_ipv4_mask;
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum rte_flow_item_type type, · line 189
Quoted 10 lines
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
   		return 1;
 
   	case RTE_FLOW_ITEM_TYPE_IPV6:
   		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
   		return 1;
 
As far as I can see the 'flow_item_is_proto' sets the size to be used over 'rte_flow_item_*" types, the original values seems correct to me, am I missing something.

Quoted 7 lines

Quoted 1 line
Can you please elaborate why the change is needed?
Hi Ferruh,
The failure occur at hash_key_mask_is_same() which uses the size returned by flow_item_is_proto() for comparison. Since the rte_flow_item_* of some types is now different than the rte_*_hdr size of these types, need to use the rte_*_hdr size.

Got it.

Using "rte_*_hdr" structs size will strip the new additions to the "rte_flow_item_*" structs and won't able to use them.
But at least using old sizes should preserve old functionality and prevent any unexpected side affect, so I am OK to continue with this change but code needs to be updated later to benefit from latest "rte_flow_item_*" structs.

Cristian, Jasvinder,

Can you please acknowledge that you are aware of the reason of this change and possible updates may be required later?

Dekel Peled

2020-11-17 12:41 UTC #26 lore raw

Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

PSB.

Quoted 106 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Tuesday, November 17, 2020 12:30 PM
To: Dekel Peled <[email protected]>; Xiaoyu Min <[email protected]>; Jasvinder Singh <[email protected]>; Cristian Dumitrescu <[email protected]>
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
On 11/16/2020 7:09 PM, Dekel Peled wrote:
< Please don't top post, reply moved to the bottom >
Quoted 35 lines
Quoted 35 lines
-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Monday, November 16, 2020 6:27 PM
To: Xiaoyu Min <[email protected]>; Jasvinder Singh <[email protected]>; Cristian Dumitrescu <[email protected]>
Cc: [email protected]; Dekel Peled <[email protected]>
Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
Quoted 3 lines · Xiaoyu Min
From: Dekel Peled <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items were updated in
[1].
Quoted 6 lines
The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header data. The size to use for match should be the header data size only, and not the size of the whole struct.
This patch replaces the rte_flow_item_* with the corresponding
rte_*_hdr.
Quoted 13 lines
[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6 item")
Signed-off-by Dekel Peled <[email protected]>
---
   drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
   1 file changed, 4 insertions(+), 4 deletions(-)
Quoted 6 lines
Quoted 6 lines
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum
Quoted 6 lines
drivers/net/softnic/rte_eth_softnic_flow.c +0 -0
index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum
rte_flow_item_type
Quoted 31 lines
Quoted 31 lines
type,
Quoted 15 lines
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
   	case RTE_FLOW_ITEM_TYPE_ETH:
   		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
   		return 1; /* TRUE */
 
   	case RTE_FLOW_ITEM_TYPE_VLAN:
   		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
   		return 1;
 
   	case RTE_FLOW_ITEM_TYPE_IPV4:
   		*mask = &rte_flow_item_ipv4_mask;
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum rte_flow_item_type type, · line 189
Quoted 10 lines
drivers/net/softnic/rte_eth_softnic_flow.c +2 -2
-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
   		return 1;
 
   	case RTE_FLOW_ITEM_TYPE_IPV6:
   		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
   		return 1;
 
As far as I can see the 'flow_item_is_proto' sets the size to be used over 'rte_flow_item_*" types, the original values seems correct to me, am I missing something.
Quoted 5 lines
Quoted 1 line
Can you please elaborate why the change is needed?
Hi Ferruh,
The failure occur at hash_key_mask_is_same() which uses the size
returned by
flow_item_is_proto() for comparison.
Quoted 1 line
Since the rte_flow_item_* of some types is now different than the
rte_*_hdr size of these types, need to use the rte_*_hdr size.
Quoted 1 line
Got it.
Using "rte_*_hdr" structs size will strip the new additions to the "rte_flow_item_*" structs and won't able to use them.
But at least using old sizes should preserve old functionality and prevent any unexpected side affect, so I am OK to continue with this change but code needs to be updated later to benefit from latest "rte_flow_item_*" structs.

Quoted 4 lines

Cristian, Jasvinder,
Can you please acknowledge that you are aware of the reason of this change and possible updates may be required later?

Please see related issues fixed by this patch:
https://bugs.dpdk.org/show_bug.cgi?id=564
https://bugs.dpdk.org/show_bug.cgi?id=565

Singh, Jasvinder

Acked-by

2020-11-17 15:44 UTC #27 lore raw

Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

<snip>

Quoted 62 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · Xiaoyu Min
Quoted 3 lines
From: Dekel Peled <[email protected]>
The rte_flow_item_eth and rte_flow_item_vlan items were updated in
[1].
Quoted 2 lines
The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header
data.
Quoted 53 lines
Quoted 19 lines
Quoted 19 lines
Quoted 4 lines
The size to use for match should be the header data size only, and not the size of the whole struct.
This patch replaces the rte_flow_item_* with the corresponding
rte_*_hdr.
Quoted 14 lines
[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN
items")
[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6
item")
Signed-off-by Dekel Peled <[email protected]>
---
   drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
   1 file changed, 4 insertions(+), 4 deletions(-)
Quoted 6 lines
Quoted 6 lines
drivers/net/softnic/rte_eth_softnic_flow.c flow_item_is_proto(enum
Quoted 6 lines
drivers/net/softnic/rte_eth_softnic_flow.c +0 -0
index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum
rte_flow_item_type
Quoted 27 lines
Quoted 27 lines
type,
Quoted 25 lines
drivers/net/softnic/rte_eth_softnic_flow.c +4 -4
   	case RTE_FLOW_ITEM_TYPE_ETH:
   		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
   		return 1; /* TRUE */
 
   	case RTE_FLOW_ITEM_TYPE_VLAN:
   		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
   		return 1;
 
   	case RTE_FLOW_ITEM_TYPE_IPV4:
   		*mask = &rte_flow_item_ipv4_mask;
-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
   		return 1;
 
   	case RTE_FLOW_ITEM_TYPE_IPV6:
   		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
   		return 1;
 

Acked-by Jasvinder Singh <[email protected]>

Zhou, JunX W

Tested-by on v1 5/5

2020-11-18 02:23 UTC #28 lore raw

Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

In reply to Xiaoyu Min

Tested-by Zhou, Jun <[email protected]>

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Xiaoyu Min
Sent: Monday, November 16, 2020 3:55 PM
To: Singh, Jasvinder <[email protected]>; Dumitrescu, Cristian <[email protected]>
Cc: [email protected]; Dekel Peled <[email protected]>
Subject: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

From: Dekel Peled <[email protected]>

The rte_flow_item_eth and rte_flow_item_vlan items were updated in [1].
The rte_flow_item_ipv6 item was updated in [2].
The structs now contain additional metadata following the header data.
The size to use for match should be the header data size only, and not the size of the whole struct.

This patch replaces the rte_flow_item_* with the corresponding rte_*_hdr.

[1]:commit 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")

[2]: commit ad976bd40d28 ("ethdev: add extensions attributes to IPv6 item")

Signed-off-by Dekel Peled <[email protected]>

---
 drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/net/softnic/rte_eth_softnic_flow.c +4 -4

index f05ff092fa..7925bad1c0 100644
@@ -169,22 +169,22 @@ flow_item_is_proto(enum rte_flow_item_type type,
 
 	case RTE_FLOW_ITEM_TYPE_ETH:
 		*mask = &rte_flow_item_eth_mask;
-		*size = sizeof(struct rte_flow_item_eth);
+		*size = sizeof(struct rte_ether_hdr);
 		return 1; /* TRUE */
 
 	case RTE_FLOW_ITEM_TYPE_VLAN:
 		*mask = &rte_flow_item_vlan_mask;
-		*size = sizeof(struct rte_flow_item_vlan);
+		*size = sizeof(struct rte_vlan_hdr);
 		return 1;
 
 	case RTE_FLOW_ITEM_TYPE_IPV4:
 		*mask = &rte_flow_item_ipv4_mask;
-		*size = sizeof(struct rte_flow_item_ipv4);
+		*size = sizeof(struct rte_ipv4_hdr);
 		return 1;
 
 	case RTE_FLOW_ITEM_TYPE_IPV6:
 		*mask = &rte_flow_item_ipv6_mask;
-		*size = sizeof(struct rte_flow_item_ipv6);
+		*size = sizeof(struct rte_ipv6_hdr);
 		return 1;
 
 	case RTE_FLOW_ITEM_TYPE_ICMP:

--
2.25.1

Thomas Monjalon

2020-11-22 16:11 UTC #29 lore raw

In reply to Xiaoyu Min

16/11/2020 08:55, Xiaoyu Min:

Quoted 8 lines

From: Xiaoyu Min <[email protected]>
The rte_flow_item_eth, rte_flow_item_vlan, and rte_flow_item_ipv6 are refined. The structs do not exactly represent the real protocol headers any more.
This serial patchs try to fix all related parts due to the changes.

Quoted 7 lines

Dekel Peled (1):
net/softnic: update headers size calculation
Xiaoyu Min (4):
net/mlx5: fix protocol size for raw encap judgement app/flow-perf: fix protocol size for raw encap net/bnxt: fix protocol size for VXLAN encap copy

Applied except the iavf patch (as discussed in the thread):

Quoted 1 line

net/iavf: fix protocol size for virtchnl copy

A follow-up is required for iavf PMD,
and in general to make things clearer.
A proposal is to have rte_net structs as first field of rte_flow items.