DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/5] fix protocol size calculation
@ 2020-11-16  7:55 Xiaoyu Min
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement Xiaoyu Min
                   ` (5 more replies)
  0 siblings, 6 replies; 30+ messages in thread
From: Xiaoyu Min @ 2020-11-16  7:55 UTC (permalink / raw)
  Cc: dev, Xiaoyu Min

From: Xiaoyu Min <jackmin@nvidia.com>

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(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
@ 2020-11-16  7:55 ` Xiaoyu Min
  2020-11-17 13:25   ` Matan Azrad
  2020-11-22 15:32   ` Thomas Monjalon
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 2/5] app/flow-perf: fix protocol size for raw encap Xiaoyu Min
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 30+ messages in thread
From: Xiaoyu Min @ 2020-11-16  7:55 UTC (permalink / raw)
  To: Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko; +Cc: dev, Xiaoyu Min

From: Xiaoyu Min <jackmin@nvidia.com>

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 <jackmin@nvidia.com>
---
 drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
 drivers/net/mlx5/mlx5_flow.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 324349ed19..34b7a2f137 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -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++;
diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
index 5fac8672fc..3b28c48a1d 100644
--- a/drivers/net/mlx5/mlx5_flow.h
+++ b/drivers/net/mlx5/mlx5_flow.h
@@ -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 \
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH 2/5] app/flow-perf: fix protocol size for raw encap
  2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement Xiaoyu Min
@ 2020-11-16  7:55 ` Xiaoyu Min
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy Xiaoyu Min
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Xiaoyu Min @ 2020-11-16  7:55 UTC (permalink / raw)
  To: Wisam Jaddo; +Cc: dev, Xiaoyu Min

From: Xiaoyu Min <jackmin@nvidia.com>

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 <jackmin@nvidia.com>
---
 app/test-flow-perf/actions_gen.c | 136 +++++++++++++++----------------
 1 file changed, 67 insertions(+), 69 deletions(-)

diff --git a/app/test-flow-perf/actions_gen.c b/app/test-flow-perf/actions_gen.c
index e3a95d7ab2..ac525f6fdb 100644
--- a/app/test-flow-perf/actions_gen.c
+++ b/app/test-flow-perf/actions_gen.c
@@ -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 {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy
  2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement Xiaoyu Min
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 2/5] app/flow-perf: fix protocol size for raw encap Xiaoyu Min
@ 2020-11-16  7:55 ` Xiaoyu Min
  2020-11-16 16:12   ` Ferruh Yigit
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy Xiaoyu Min
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Xiaoyu Min @ 2020-11-16  7:55 UTC (permalink / raw)
  To: Ajit Khaparde, Somnath Kotur; +Cc: dev, Xiaoyu Min

From: Xiaoyu Min <jackmin@nvidia.com>

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 <jackmin@nvidia.com>
---
 drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
index a54c55c5f5..823eeb21b8 100644
--- a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
+++ b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
@@ -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;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
                   ` (2 preceding siblings ...)
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy Xiaoyu Min
@ 2020-11-16  7:55 ` Xiaoyu Min
  2020-11-16 16:23   ` Ferruh Yigit
                     ` (2 more replies)
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation Xiaoyu Min
  2020-11-22 16:11 ` [dpdk-dev] [PATCH 0/5] fix protocol " Thomas Monjalon
  5 siblings, 3 replies; 30+ messages in thread
From: Xiaoyu Min @ 2020-11-16  7:55 UTC (permalink / raw)
  To: Jingjing Wu, Beilei Xing; +Cc: dev, Xiaoyu Min

From: Xiaoyu Min <jackmin@nvidia.com>

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 <jackmin@nvidia.com>
---
 drivers/net/iavf/iavf_fdir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/iavf/iavf_fdir.c b/drivers/net/iavf/iavf_fdir.c
index d683a468c1..7054bde0b9 100644
--- a/drivers/net/iavf/iavf_fdir.c
+++ b/drivers/net/iavf/iavf_fdir.c
@@ -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;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
                   ` (3 preceding siblings ...)
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy Xiaoyu Min
@ 2020-11-16  7:55 ` Xiaoyu Min
  2020-11-16 16:27   ` Ferruh Yigit
  2020-11-18  2:23   ` Zhou, JunX W
  2020-11-22 16:11 ` [dpdk-dev] [PATCH 0/5] fix protocol " Thomas Monjalon
  5 siblings, 2 replies; 30+ messages in thread
From: Xiaoyu Min @ 2020-11-16  7:55 UTC (permalink / raw)
  To: Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, Dekel Peled

From: Dekel Peled <dekelp@nvidia.com>

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 <dekelp@nvidia.com>
---
 drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c b/drivers/net/softnic/rte_eth_softnic_flow.c
index f05ff092fa..7925bad1c0 100644
--- a/drivers/net/softnic/rte_eth_softnic_flow.c
+++ b/drivers/net/softnic/rte_eth_softnic_flow.c
@@ -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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy Xiaoyu Min
@ 2020-11-16 16:12   ` Ferruh Yigit
  2020-11-18  0:34     ` Ajit Khaparde
  0 siblings, 1 reply; 30+ messages in thread
From: Ferruh Yigit @ 2020-11-16 16:12 UTC (permalink / raw)
  To: Xiaoyu Min, Ajit Khaparde, Somnath Kotur; +Cc: dev, Xiaoyu Min

On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> From: Xiaoyu Min <jackmin@nvidia.com>
> 
> 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 <jackmin@nvidia.com>

<...>

> @@ -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);

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy Xiaoyu Min
@ 2020-11-16 16:23   ` Ferruh Yigit
  2020-11-22 13:28     ` Jack Min
  2020-11-23 10:49   ` Ferruh Yigit
  2020-11-27  1:17   ` Xing, Beilei
  2 siblings, 1 reply; 30+ messages in thread
From: Ferruh Yigit @ 2020-11-16 16:23 UTC (permalink / raw)
  To: Xiaoyu Min, Jingjing Wu, Beilei Xing
  Cc: dev, Xiaoyu Min, Thomas Monjalon, Andrew Rybchenko, Ori Kam, Dekel Peled

On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> From: Xiaoyu Min <jackmin@nvidia.com>
> 
> 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 <jackmin@nvidia.com>
> ---
>   drivers/net/iavf/iavf_fdir.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/iavf/iavf_fdir.c b/drivers/net/iavf/iavf_fdir.c
> index d683a468c1..7054bde0b9 100644
> --- a/drivers/net/iavf/iavf_fdir.c
> +++ b/drivers/net/iavf/iavf_fdir.c
> @@ -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 */
};





^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation Xiaoyu Min
@ 2020-11-16 16:27   ` Ferruh Yigit
  2020-11-16 19:09     ` Dekel Peled
  2020-11-18  2:23   ` Zhou, JunX W
  1 sibling, 1 reply; 30+ messages in thread
From: Ferruh Yigit @ 2020-11-16 16:27 UTC (permalink / raw)
  To: Xiaoyu Min, Jasvinder Singh, Cristian Dumitrescu; +Cc: dev, Dekel Peled

On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> From: Dekel Peled <dekelp@nvidia.com>
> 
> 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 <dekelp@nvidia.com>
> ---
>   drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c b/drivers/net/softnic/rte_eth_softnic_flow.c
> index f05ff092fa..7925bad1c0 100644
> --- a/drivers/net/softnic/rte_eth_softnic_flow.c
> +++ b/drivers/net/softnic/rte_eth_softnic_flow.c
> @@ -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;
>   

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?


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-16 16:27   ` Ferruh Yigit
@ 2020-11-16 19:09     ` Dekel Peled
  2020-11-17 10:30       ` Ferruh Yigit
  0 siblings, 1 reply; 30+ messages in thread
From: Dekel Peled @ 2020-11-16 19:09 UTC (permalink / raw)
  To: Ferruh Yigit, Xiaoyu Min, Jasvinder Singh, Cristian Dumitrescu; +Cc: dev

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

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, November 16, 2020 6:27 PM
> To: Xiaoyu Min <jackmin@mellanox.com>; Jasvinder Singh
> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Dekel Peled <dekelp@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size
> calculation
> 
> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> > From: Dekel Peled <dekelp@nvidia.com>
> >
> > 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 <dekelp@nvidia.com>
> > ---
> >   drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c
> > b/drivers/net/softnic/rte_eth_softnic_flow.c
> > index f05ff092fa..7925bad1c0 100644
> > --- a/drivers/net/softnic/rte_eth_softnic_flow.c
> > +++ b/drivers/net/softnic/rte_eth_softnic_flow.c
> > @@ -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;
> >
> 
> 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?


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-16 19:09     ` Dekel Peled
@ 2020-11-17 10:30       ` Ferruh Yigit
  2020-11-17 12:41         ` Dekel Peled
  0 siblings, 1 reply; 30+ messages in thread
From: Ferruh Yigit @ 2020-11-17 10:30 UTC (permalink / raw)
  To: Dekel Peled, Xiaoyu Min, Jasvinder Singh, Cristian Dumitrescu; +Cc: dev

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

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

>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, November 16, 2020 6:27 PM
>> To: Xiaoyu Min <jackmin@mellanox.com>; Jasvinder Singh
>> <jasvinder.singh@intel.com>; Cristian Dumitrescu
>> <cristian.dumitrescu@intel.com>
>> Cc: dev@dpdk.org; Dekel Peled <dekelp@nvidia.com>
>> Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size
>> calculation
>>
>> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
>>> From: Dekel Peled <dekelp@nvidia.com>
>>>
>>> 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 <dekelp@nvidia.com>
>>> ---
>>>    drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c
>>> b/drivers/net/softnic/rte_eth_softnic_flow.c
>>> index f05ff092fa..7925bad1c0 100644
>>> --- a/drivers/net/softnic/rte_eth_softnic_flow.c
>>> +++ b/drivers/net/softnic/rte_eth_softnic_flow.c
>>> @@ -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;
>>>
>>
>> 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?
> 
 > 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?

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-17 10:30       ` Ferruh Yigit
@ 2020-11-17 12:41         ` Dekel Peled
  2020-11-17 15:43           ` Singh, Jasvinder
  0 siblings, 1 reply; 30+ messages in thread
From: Dekel Peled @ 2020-11-17 12:41 UTC (permalink / raw)
  To: Ferruh Yigit, Xiaoyu Min, Jasvinder Singh, Cristian Dumitrescu; +Cc: dev

PSB.

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, November 17, 2020 12:30 PM
> To: Dekel Peled <dekelp@nvidia.com>; Xiaoyu Min
> <jackmin@mellanox.com>; Jasvinder Singh <jasvinder.singh@intel.com>;
> Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org
> 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 >
> 
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Monday, November 16, 2020 6:27 PM
> >> To: Xiaoyu Min <jackmin@mellanox.com>; Jasvinder Singh
> >> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> >> <cristian.dumitrescu@intel.com>
> >> Cc: dev@dpdk.org; Dekel Peled <dekelp@nvidia.com>
> >> Subject: Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size
> >> calculation
> >>
> >> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> >>> From: Dekel Peled <dekelp@nvidia.com>
> >>>
> >>> 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 <dekelp@nvidia.com>
> >>> ---
> >>>    drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
> >>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c
> >>> b/drivers/net/softnic/rte_eth_softnic_flow.c
> >>> index f05ff092fa..7925bad1c0 100644
> >>> --- a/drivers/net/softnic/rte_eth_softnic_flow.c
> >>> +++ b/drivers/net/softnic/rte_eth_softnic_flow.c
> >>> @@ -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;
> >>>
> >>
> >> 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?
> >
>  > 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?

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement Xiaoyu Min
@ 2020-11-17 13:25   ` Matan Azrad
  2020-11-22 14:21     ` Thomas Monjalon
  2020-11-22 15:32   ` Thomas Monjalon
  1 sibling, 1 reply; 30+ messages in thread
From: Matan Azrad @ 2020-11-17 13:25 UTC (permalink / raw)
  To: Xiaoyu Min, Shahaf Shuler, Slava Ovsiienko; +Cc: dev, Jack Min



From: Xiaoyu Min
> 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 <jackmin@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-17 12:41         ` Dekel Peled
@ 2020-11-17 15:43           ` Singh, Jasvinder
  0 siblings, 0 replies; 30+ messages in thread
From: Singh, Jasvinder @ 2020-11-17 15:43 UTC (permalink / raw)
  To: Dekel Peled, Yigit, Ferruh, Xiaoyu Min, Dumitrescu, Cristian; +Cc: dev



<snip>
> > >>
> > >> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> > >>> From: Dekel Peled <dekelp@nvidia.com>
> > >>>
> > >>> 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 <dekelp@nvidia.com>
> > >>> ---
> > >>>    drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
> > >>>    1 file changed, 4 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c
> > >>> b/drivers/net/softnic/rte_eth_softnic_flow.c
> > >>> index f05ff092fa..7925bad1c0 100644
> > >>> --- a/drivers/net/softnic/rte_eth_softnic_flow.c
> > >>> +++ b/drivers/net/softnic/rte_eth_softnic_flow.c
> > >>> @@ -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;
> > >>>
> > >>

Acked-by: Jasvinder Singh <jasvinder.singh@intel.com>


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy
  2020-11-16 16:12   ` Ferruh Yigit
@ 2020-11-18  0:34     ` Ajit Khaparde
  2020-11-18  6:37       ` Andrew Rybchenko
  0 siblings, 1 reply; 30+ messages in thread
From: Ajit Khaparde @ 2020-11-18  0:34 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Xiaoyu Min, Somnath Kotur, dpdk-dev, Xiaoyu Min

On Mon, Nov 16, 2020 at 8:13 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> > From: Xiaoyu Min <jackmin@nvidia.com>
> >
> > 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 <jackmin@nvidia.com>
>
> <...>
>
> > @@ -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);
>
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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation Xiaoyu Min
  2020-11-16 16:27   ` Ferruh Yigit
@ 2020-11-18  2:23   ` Zhou, JunX W
  1 sibling, 0 replies; 30+ messages in thread
From: Zhou, JunX W @ 2020-11-18  2:23 UTC (permalink / raw)
  To: Xiaoyu Min, Singh, Jasvinder, Dumitrescu, Cristian; +Cc: dev, Dekel Peled

Tested-by: Zhou, Jun <junx.w.zhou@intel.com>

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xiaoyu Min
Sent: Monday, November 16, 2020 3:55 PM
To: Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
Cc: dev@dpdk.org; Dekel Peled <dekelp@nvidia.com>
Subject: [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation

From: Dekel Peled <dekelp@nvidia.com>

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 <dekelp@nvidia.com>
---
 drivers/net/softnic/rte_eth_softnic_flow.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/softnic/rte_eth_softnic_flow.c b/drivers/net/softnic/rte_eth_softnic_flow.c
index f05ff092fa..7925bad1c0 100644
--- a/drivers/net/softnic/rte_eth_softnic_flow.c
+++ b/drivers/net/softnic/rte_eth_softnic_flow.c
@@ -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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy
  2020-11-18  0:34     ` Ajit Khaparde
@ 2020-11-18  6:37       ` Andrew Rybchenko
  2020-11-18 17:44         ` Ajit Khaparde
  0 siblings, 1 reply; 30+ messages in thread
From: Andrew Rybchenko @ 2020-11-18  6:37 UTC (permalink / raw)
  To: Ajit Khaparde, Ferruh Yigit
  Cc: Xiaoyu Min, Somnath Kotur, dpdk-dev, Xiaoyu Min

On 11/18/20 3:34 AM, Ajit Khaparde wrote:
> On Mon, Nov 16, 2020 at 8:13 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
>>> From: Xiaoyu Min <jackmin@nvidia.com>
>>>
>>> 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 <jackmin@nvidia.com>
>>
>> <...>
>>
>>> @@ -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);
>>
> 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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy
  2020-11-18  6:37       ` Andrew Rybchenko
@ 2020-11-18 17:44         ` Ajit Khaparde
  0 siblings, 0 replies; 30+ messages in thread
From: Ajit Khaparde @ 2020-11-18 17:44 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Ferruh Yigit, Xiaoyu Min, Somnath Kotur, dpdk-dev, Xiaoyu Min

On Tue, Nov 17, 2020 at 10:37 PM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 11/18/20 3:34 AM, Ajit Khaparde wrote:
> > On Mon, Nov 16, 2020 at 8:13 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >
> >> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> >>> From: Xiaoyu Min <jackmin@nvidia.com>
> >>>
> >>> 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 <jackmin@nvidia.com>
> >>
> >> <...>
> >>
> >>> @@ -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);
> >>
> > 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.
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 <ajit.khaparde@broadcom.com>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-16 16:23   ` Ferruh Yigit
@ 2020-11-22 13:28     ` Jack Min
  2020-11-22 14:15       ` Thomas Monjalon
  0 siblings, 1 reply; 30+ messages in thread
From: Jack Min @ 2020-11-22 13:28 UTC (permalink / raw)
  To: Ferruh Yigit, Xiaoyu Min, Jingjing Wu, Beilei Xing
  Cc: dev, NBU-Contact-Thomas Monjalon, Andrew Rybchenko, Ori Kam, Dekel Peled

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, November 17, 2020 00:23
> To: Xiaoyu Min <jackmin@mellanox.com>; Jingjing Wu <jingjing.wu@intel.com>;
> Beilei Xing <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Jack Min <jackmin@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Ori Kam <orika@nvidia.com>; Dekel Peled
> <dekelp@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
> 
> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> > From: Xiaoyu Min <jackmin@nvidia.com>
> >
> > 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 <jackmin@nvidia.com>
> > ---
> >   drivers/net/iavf/iavf_fdir.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/iavf/iavf_fdir.c b/drivers/net/iavf/iavf_fdir.c
> > index d683a468c1..7054bde0b9 100644
> > --- a/drivers/net/iavf/iavf_fdir.c
> > +++ b/drivers/net/iavf/iavf_fdir.c
> > @@ -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_*'...
> 
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.

> 
> 
> 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 */
> };
> 

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

-Jack


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-22 13:28     ` Jack Min
@ 2020-11-22 14:15       ` Thomas Monjalon
  2020-11-23 10:02         ` Ferruh Yigit
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Monjalon @ 2020-11-22 14:15 UTC (permalink / raw)
  To: Ferruh Yigit, Xiaoyu Min, Jingjing Wu, Beilei Xing
  Cc: dev, Andrew Rybchenko, Ori Kam, Dekel Peled

22/11/2020 14:28, Jack Min:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> > > From: Xiaoyu Min <jackmin@nvidia.com>
> > >
> > > 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 <jackmin@nvidia.com>
> > > ---
> > >   drivers/net/iavf/iavf_fdir.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/iavf/iavf_fdir.c b/drivers/net/iavf/iavf_fdir.c
> > > index d683a468c1..7054bde0b9 100644
> > > --- a/drivers/net/iavf/iavf_fdir.c
> > > +++ b/drivers/net/iavf/iavf_fdir.c
> > > @@ -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_*'...
> > 
> 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.



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-17 13:25   ` Matan Azrad
@ 2020-11-22 14:21     ` Thomas Monjalon
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Monjalon @ 2020-11-22 14:21 UTC (permalink / raw)
  To: Xiaoyu Min, Slava Ovsiienko
  Cc: dev, Matan Azrad, orika, suanmingm, rasland, asafp

17/11/2020 14:25, Matan Azrad:
> From: Xiaoyu Min
> > 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 <jackmin@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>

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




^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement Xiaoyu Min
  2020-11-17 13:25   ` Matan Azrad
@ 2020-11-22 15:32   ` Thomas Monjalon
  2020-11-22 16:04     ` Matan Azrad
  1 sibling, 1 reply; 30+ messages in thread
From: Thomas Monjalon @ 2020-11-22 15:32 UTC (permalink / raw)
  To: Matan Azrad, Viacheslav Ovsiienko
  Cc: dev, Xiaoyu Min, orika, suanmingm, asafp

16/11/2020 08:55, Xiaoyu Min:
> From: Xiaoyu Min <jackmin@nvidia.com>
> 
> 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 <jackmin@nvidia.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
>  drivers/net/mlx5/mlx5_flow.h |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 324349ed19..34b7a2f137 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -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++;
> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index 5fac8672fc..3b28c48a1d 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -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.





^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-22 15:32   ` Thomas Monjalon
@ 2020-11-22 16:04     ` Matan Azrad
  2020-11-23  7:54       ` Ori Kam
  0 siblings, 1 reply; 30+ messages in thread
From: Matan Azrad @ 2020-11-22 16:04 UTC (permalink / raw)
  To: NBU-Contact-Thomas Monjalon, Slava Ovsiienko
  Cc: dev, Xiaoyu Min, orika, Suanming Mou, Asaf Penso



From: Thomas Monjalon
 
> 16/11/2020 08:55, Xiaoyu Min:
> > From: Xiaoyu Min <jackmin@nvidia.com>
> >
> > 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 <jackmin@nvidia.com>
> > ---
> >  drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
> > drivers/net/mlx5/mlx5_flow.h |  4 ++--
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > b/drivers/net/mlx5/mlx5_flow.c index 324349ed19..34b7a2f137 100644
> > --- a/drivers/net/mlx5/mlx5_flow.c
> > +++ b/drivers/net/mlx5/mlx5_flow.c
> > @@ -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++; diff --git
> > a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h index
> > 5fac8672fc..3b28c48a1d 100644
> > --- a/drivers/net/mlx5/mlx5_flow.h
> > +++ b/drivers/net/mlx5/mlx5_flow.h
> > @@ -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.

+1
 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 0/5] fix protocol size calculation
  2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
                   ` (4 preceding siblings ...)
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation Xiaoyu Min
@ 2020-11-22 16:11 ` Thomas Monjalon
  5 siblings, 0 replies; 30+ messages in thread
From: Thomas Monjalon @ 2020-11-22 16:11 UTC (permalink / raw)
  To: Xiaoyu Min; +Cc: dev, ferruh.yigit, andrew.rybchenko, orika

16/11/2020 08:55, Xiaoyu Min:
> From: Xiaoyu Min <jackmin@nvidia.com>
> 
> 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

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

>   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.



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-22 16:04     ` Matan Azrad
@ 2020-11-23  7:54       ` Ori Kam
  2020-11-23  8:12         ` Thomas Monjalon
  0 siblings, 1 reply; 30+ messages in thread
From: Ori Kam @ 2020-11-23  7:54 UTC (permalink / raw)
  To: Matan Azrad, NBU-Contact-Thomas Monjalon, Slava Ovsiienko
  Cc: dev, Jack Min, Ori Kam, Suanming Mou, Asaf Penso



> -----Original Message-----
> From: Matan Azrad <matan@nvidia.com>
> 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
> 
> > 16/11/2020 08:55, Xiaoyu Min:
> > > From: Xiaoyu Min <jackmin@nvidia.com>
> > >
> > > 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 <jackmin@nvidia.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
> > > drivers/net/mlx5/mlx5_flow.h |  4 ++--
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index 324349ed19..34b7a2f137 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -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++; diff --git
> > > a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h index
> > > 5fac8672fc..3b28c48a1d 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.h
> > > +++ b/drivers/net/mlx5/mlx5_flow.h
> > > @@ -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.
> 
> +1
> 

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

Best,
Ori


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement
  2020-11-23  7:54       ` Ori Kam
@ 2020-11-23  8:12         ` Thomas Monjalon
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Monjalon @ 2020-11-23  8:12 UTC (permalink / raw)
  To: Matan Azrad, Slava Ovsiienko, Ori Kam
  Cc: dev, Jack Min, Ori Kam, Suanming Mou, Asaf Penso

23/11/2020 08:54, Ori Kam:
> From: Matan Azrad <matan@nvidia.com>
> > From: Thomas Monjalon
> > > 16/11/2020 08:55, Xiaoyu Min:
> > > > From: Xiaoyu Min <jackmin@nvidia.com>
> > > >
> > > > 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 <jackmin@nvidia.com>
> > > > ---
> > > >  drivers/net/mlx5/mlx5_flow.c | 12 ++++++------
> > > > drivers/net/mlx5/mlx5_flow.h |  4 ++--
> > > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > > b/drivers/net/mlx5/mlx5_flow.c index 324349ed19..34b7a2f137 100644
> > > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > > @@ -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++; diff --git
> > > > a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h index
> > > > 5fac8672fc..3b28c48a1d 100644
> > > > --- a/drivers/net/mlx5/mlx5_flow.h
> > > > +++ b/drivers/net/mlx5/mlx5_flow.h
> > > > @@ -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.
> > 
> > +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.



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-22 14:15       ` Thomas Monjalon
@ 2020-11-23 10:02         ` Ferruh Yigit
  0 siblings, 0 replies; 30+ messages in thread
From: Ferruh Yigit @ 2020-11-23 10:02 UTC (permalink / raw)
  To: Thomas Monjalon, Xiaoyu Min, Jingjing Wu, Beilei Xing
  Cc: dev, Andrew Rybchenko, Ori Kam, Dekel Peled

On 11/22/2020 2:15 PM, Thomas Monjalon wrote:
> 22/11/2020 14:28, Jack Min:
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
>>>> From: Xiaoyu Min <jackmin@nvidia.com>
>>>>
>>>> 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 <jackmin@nvidia.com>
>>>> ---
>>>>    drivers/net/iavf/iavf_fdir.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/iavf/iavf_fdir.c b/drivers/net/iavf/iavf_fdir.c
>>>> index d683a468c1..7054bde0b9 100644
>>>> --- a/drivers/net/iavf/iavf_fdir.c
>>>> +++ b/drivers/net/iavf/iavf_fdir.c
>>>> @@ -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_*'...
>>>
>> 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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy Xiaoyu Min
  2020-11-16 16:23   ` Ferruh Yigit
@ 2020-11-23 10:49   ` Ferruh Yigit
  2020-11-24 21:58     ` Thomas Monjalon
  2020-11-27  1:17   ` Xing, Beilei
  2 siblings, 1 reply; 30+ messages in thread
From: Ferruh Yigit @ 2020-11-23 10:49 UTC (permalink / raw)
  To: Xiaoyu Min, Jingjing Wu, Beilei Xing; +Cc: dev, Xiaoyu Min

On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> From: Xiaoyu Min <jackmin@nvidia.com>
> 
> 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 <jackmin@nvidia.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-23 10:49   ` Ferruh Yigit
@ 2020-11-24 21:58     ` Thomas Monjalon
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Monjalon @ 2020-11-24 21:58 UTC (permalink / raw)
  To: Xiaoyu Min, Ferruh Yigit; +Cc: Jingjing Wu, Beilei Xing, dev

23/11/2020 11:49, Ferruh Yigit:
> On 11/16/2020 7:55 AM, Xiaoyu Min wrote:
> > From: Xiaoyu Min <jackmin@nvidia.com>
> > 
> > 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 <jackmin@nvidia.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
  2020-11-16  7:55 ` [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy Xiaoyu Min
  2020-11-16 16:23   ` Ferruh Yigit
  2020-11-23 10:49   ` Ferruh Yigit
@ 2020-11-27  1:17   ` Xing, Beilei
  2 siblings, 0 replies; 30+ messages in thread
From: Xing, Beilei @ 2020-11-27  1:17 UTC (permalink / raw)
  To: Xiaoyu Min, Wu, Jingjing; +Cc: dev, Xiaoyu Min



> -----Original Message-----
> From: Xiaoyu Min <jackmin@mellanox.com>
> Sent: Monday, November 16, 2020 3:55 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Xiaoyu Min <jackmin@nvidia.com>
> Subject: [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy
> 
> From: Xiaoyu Min <jackmin@nvidia.com>
> 
> 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 <jackmin@nvidia.com>

Acked-by: Beilei Xing <beilei.xing@intel.com>

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2020-11-27  1:17 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-16  7:55 [dpdk-dev] [PATCH 0/5] fix protocol size calculation Xiaoyu Min
2020-11-16  7:55 ` [dpdk-dev] [PATCH 1/5] net/mlx5: fix protocol size for raw encap judgement Xiaoyu Min
2020-11-17 13:25   ` Matan Azrad
2020-11-22 14:21     ` Thomas Monjalon
2020-11-22 15:32   ` Thomas Monjalon
2020-11-22 16:04     ` Matan Azrad
2020-11-23  7:54       ` Ori Kam
2020-11-23  8:12         ` Thomas Monjalon
2020-11-16  7:55 ` [dpdk-dev] [PATCH 2/5] app/flow-perf: fix protocol size for raw encap Xiaoyu Min
2020-11-16  7:55 ` [dpdk-dev] [PATCH 3/5] net/bnxt: fix protocol size for VXLAN encap copy Xiaoyu Min
2020-11-16 16:12   ` Ferruh Yigit
2020-11-18  0:34     ` Ajit Khaparde
2020-11-18  6:37       ` Andrew Rybchenko
2020-11-18 17:44         ` Ajit Khaparde
2020-11-16  7:55 ` [dpdk-dev] [PATCH 4/5] net/iavf: fix protocol size for virtchnl copy Xiaoyu Min
2020-11-16 16:23   ` Ferruh Yigit
2020-11-22 13:28     ` Jack Min
2020-11-22 14:15       ` Thomas Monjalon
2020-11-23 10:02         ` Ferruh Yigit
2020-11-23 10:49   ` Ferruh Yigit
2020-11-24 21:58     ` Thomas Monjalon
2020-11-27  1:17   ` Xing, Beilei
2020-11-16  7:55 ` [dpdk-dev] [PATCH 5/5] net/softnic: update headers size calculation Xiaoyu Min
2020-11-16 16:27   ` Ferruh Yigit
2020-11-16 19:09     ` Dekel Peled
2020-11-17 10:30       ` Ferruh Yigit
2020-11-17 12:41         ` Dekel Peled
2020-11-17 15:43           ` Singh, Jasvinder
2020-11-18  2:23   ` Zhou, JunX W
2020-11-22 16:11 ` [dpdk-dev] [PATCH 0/5] fix protocol " Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).