-----Original Message-----
From: Cao, Yahui
Sent: Friday, December 25, 2020 1:22 PM
To: Yan, Zhirun <redacted>; dev@dpdk.org; Zhang, Qi Z
[off-list ref]; Wang, Xiao W [off-list ref]; Guo,
Junfeng [off-list ref]
Cc: Su, Simei <redacted>; Xu, Ting <redacted>; Zhang,
Yuying [off-list ref]
Subject: RE: [PATCH v1 2/5] net/ice: refactor flow pattern parser

quoted

-----Original Message-----
From: Yan, Zhirun <redacted>
Sent: Monday, December 21, 2020 2:52 PM
To: dev@dpdk.org; Zhang, Qi Z <redacted>; Cao, Yahui
[off-list ref]; Wang, Xiao W [off-list ref]; Guo,
Junfeng [off-list ref]
Cc: Su, Simei <redacted>; Xu, Ting <redacted>;
Zhang, Yuying [off-list ref]; Yan, Zhirun
[off-list ref]
Subject: [PATCH v1 2/5] net/ice: refactor flow pattern parser

Distinguish inner/outer input set. And avoid too many nested
conditionals in each type's parser. input_set_f is used for outer
fields, input_set_l is used for inner or non-tunnel fields.

Signed-off-by: Zhirun Yan <redacted>
---
 drivers/net/ice/ice_fdir_filter.c | 467
+++++++++++++++---------------
 1 file changed, 229 insertions(+), 238 deletions(-)

diff --git a/drivers/net/ice/ice_fdir_filter.c

b/drivers/net/ice/ice_fdir_filter.c
index 92b8a7e6ad..1f2576a444 100644

--- a/drivers/net/ice/ice_fdir_filter.c
+++ b/drivers/net/ice/ice_fdir_filter.c

@@ -1646,7 +1646,9 @@ ice_fdir_parse_pattern(__rte_unused struct

ice_adapter *ad,

quoted

 	const struct rte_flow_item_vxlan *vxlan_spec, *vxlan_mask;
 	const struct rte_flow_item_gtp *gtp_spec, *gtp_mask;
 	const struct rte_flow_item_gtp_psc *gtp_psc_spec, *gtp_psc_mask;
-	uint64_t input_set = ICE_INSET_NONE;
+	uint64_t input_set_l = ICE_INSET_NONE;
+	uint64_t input_set_f = ICE_INSET_NONE;
+	uint64_t *input_set;
 	uint8_t flow_type = ICE_FLTR_PTYPE_NONF_NONE;
 	uint8_t  ipv6_addr_mask[16] = {
 		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, @@ -1655,289
+1657,276 @@ ice_fdir_parse_pattern(__rte_unused struct ice_adapter

*ad,

quoted

 	uint32_t vtc_flow_cpu;
 	uint16_t ether_type;
 	enum rte_flow_item_type next_type;
+	bool is_outer = true;
+	struct ice_fdir_extra *p_ext_data;
+	struct ice_fdir_v4 *p_v4;
+	struct ice_fdir_v6 *p_v6;

+	for (item = pattern; item->type != RTE_FLOW_ITEM_TYPE_END;

item++) {

quoted

+		if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN)
+			tunnel_type = ICE_FDIR_TUNNEL_TYPE_VXLAN;
+		if (item->type == RTE_FLOW_ITEM_TYPE_GTPU)
+			tunnel_type = ICE_FDIR_TUNNEL_TYPE_GTPU;
+		if (item->type == RTE_FLOW_ITEM_TYPE_GTP_PSC)
+			tunnel_type = ICE_FDIR_TUNNEL_TYPE_GTPU_EH;
+	}
+
+	/* This loop parse flow pattern and distinguish Non-tunnel and

tunnel

quoted

+	 * flow. input_set_l is used for non-tunnel and tunnel inner part.
+	 */

...

quoted

+		input_set = (tunnel_type && is_outer) ?
+			    &input_set_f : &input_set_l;

[Cao, Yahui] input_set_l used for inner filed or non-tunnel filed looks
confusing, can we just use input_set_l as non-tunnel filed or tunnel outer
field ?

Yes, both are OK.
In my view, Non-tunnel and inner part have no tunnel layer info, they can be the same definition.
But for tunnel type, the outer part is different. It may contain tunnel layer info like VNI in VXLAN. 
Only this part is specific with tunnel type.

quoted

+
+		if (l3 == RTE_FLOW_ITEM_TYPE_IPV4)
+			p_v4 = (tunnel_type && is_outer) ?
+			       &filter->input.ip_outer.v4 :
+			       &filter->input.ip.v4;
+		if (l3 == RTE_FLOW_ITEM_TYPE_IPV6)
+			p_v6 = (tunnel_type && is_outer) ?
+			       &filter->input.ip_outer.v6 :
+			       &filter->input.ip.v6;
+

[Cao, Yahui] p_v4 and p_v6 pointer assignment looks redundant since it will
be assigned in IPV4 and IPV6 switch case statement.

If L3 has no fields, then we could not know whether it is ipv4 or ipv6 for L4 case.
You are right. The p_v4/6 assignment can be set earlier in L3 layer parse.

quoted

 		switch (item_type) {

...

quoted

+
+			p_v4 = (tunnel_type && is_outer) ?
+			       &filter->input.ip_outer.v4 :
+			       &filter->input.ip.v4;

[Cao, Yahui]  it is assigned here again

Yes, I will move this earlier when set flow item type.

quoted

+			p_v4->dst_ip = ipv4_spec->hdr.dst_addr;
+			p_v4->src_ip = ipv4_spec->hdr.src_addr;
+			p_v4->tos = ipv4_spec->hdr.type_of_service;
 			break;

......

quoted

+
+			p_v6 = (tunnel_type && is_outer) ?
+			       &filter->input.ip_outer.v6 :
+			       &filter->input.ip.v6;

[Cao, Yahui]  it is assigned here again

Yes, I will fix it.

quoted

+			rte_memcpy(&p_v6->dst_ip, ipv6_spec-
hdr.dst_addr, 16);
+			rte_memcpy(&p_v6->src_ip, ipv6_spec-
hdr.src_addr, 16);
+			vtc_flow_cpu = rte_be_to_cpu_32(ipv6_spec-
hdr.vtc_flow);
+			p_v6->tc = (uint8_t)(vtc_flow_cpu >>

ICE_FDIR_IPV6_TC_OFFSET);

quoted

+			p_v6->proto = ipv6_spec->hdr.proto;
+			p_v6->hlim = ipv6_spec->hdr.hop_limits;
 			break;

......

quoted

 	filter->tunnel_type = tunnel_type;
 	filter->input.flow_type = flow_type;
-	filter->input_set = input_set;
+	filter->input_set = input_set_l;
+	filter->outer_input_set = input_set_f;

[Cao, Yahui] Correspondingly here, input_set and outer_input_set naming is
also confusing.

input_set is used for tunnel inner part and non-tunnel type.
outer_input_set is only for tunnel outer part.
Thanks.

quoted

 	return 0;
 }
--
2.25.1