[PATCH] net/ice: fix how ice driver handles flows

[PATCH] net/ice: fix how ice driver handles flows

[Vladimir Medvedkin]

Currently ICE PMD uses group attribute to select the appropriate HW engine
to offload the flow. This behavior violates the rte_flow API, existing
documentation/examples, and reveals hardware specific details.

This patch eliminates the use of the group attribute and runs each engine
parser in the order they work in the HW pipeline.

Fixes: 9c5f0070fa3f ("net/ice: map group to pipeline stage")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
---
 drivers/net/ice/ice_generic_flow.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ice/ice_generic_flow.c b/drivers/net/ice/ice_generic_flow.c
index 50d760004f..666c8f3cb9 100644
--- a/drivers/net/ice/ice_generic_flow.c
+++ b/drivers/net/ice/ice_generic_flow.c
@@ -2295,21 +2295,22 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
 		return 0;
 	}
 
-	parser = get_flow_parser(attr->group);
-	if (parser == NULL) {
-		rte_flow_error_set(error, EINVAL,
-				   RTE_FLOW_ERROR_TYPE_ATTR,
-				   NULL, "NULL attribute.");
-		return -rte_errno;
+	for (int i = 0; i < 3; i++) {
+		parser = get_flow_parser(i);
+		if (parser == NULL) {
+			rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ATTR,
+					NULL, "NULL attribute.");
+			return -rte_errno;
+		}
+		if (ice_parse_engine(ad, flow, parser, attr->priority,
+				pattern, actions, error)) {
+			*engine = parser->engine;
+			return 0;
+		}
 	}
 
-	if (ice_parse_engine(ad, flow, parser, attr->priority,
-			     pattern, actions, error)) {
-		*engine = parser->engine;
-		return 0;
-	} else {
-		return -rte_errno;
-	}
+	return -rte_errno;
 }
 
 static int
-- 
2.43.0

[PATCH v2] net/ice: fix how ice driver handles flows

[Vladimir Medvedkin]

Currently ICE PMD uses group attribute to select the appropriate HW engine
to offload the flow. This behavior violates the rte_flow API, existing
documentation/examples, and reveals hardware specific details.

This patch eliminates the use of the group attribute and runs each engine
parser in the order they work in the HW pipeline.

Fixes: 9c5f0070fa3f ("net/ice: map group to pipeline stage")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
---
 drivers/net/intel/ice/ice_generic_flow.c | 30 ++++++++++++++----------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/intel/ice/ice_generic_flow.c b/drivers/net/intel/ice/ice_generic_flow.c
index 50d760004f..5c34e0385f 100644
--- a/drivers/net/intel/ice/ice_generic_flow.c
+++ b/drivers/net/intel/ice/ice_generic_flow.c
@@ -20,6 +20,8 @@
 
 #define ICE_FLOW_ENGINE_DISABLED(mask, type) ((mask) & BIT(type))
 
+#define ICE_FLOW_ENGINE_NB	3
+
 static struct ice_engine_list engine_list =
 		TAILQ_HEAD_INITIALIZER(engine_list);
 
@@ -2295,21 +2297,23 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
 		return 0;
 	}
 
-	parser = get_flow_parser(attr->group);
-	if (parser == NULL) {
-		rte_flow_error_set(error, EINVAL,
-				   RTE_FLOW_ERROR_TYPE_ATTR,
-				   NULL, "NULL attribute.");
-		return -rte_errno;
-	}
+	for (int i = 0; i < ICE_FLOW_ENGINE_NB; i++) {
+		parser = get_flow_parser(i);
+		if (parser == NULL) {
+			rte_flow_error_set(error, EINVAL,
+					RTE_FLOW_ERROR_TYPE_ATTR,
+					NULL, "NULL attribute.");
+			return -rte_errno;
+		}
 
-	if (ice_parse_engine(ad, flow, parser, attr->priority,
-			     pattern, actions, error)) {
-		*engine = parser->engine;
-		return 0;
-	} else {
-		return -rte_errno;
+		if (ice_parse_engine(ad, flow, parser, attr->priority,
+				pattern, actions, error)) {
+			*engine = parser->engine;
+			return 0;
+		}
 	}
+
+	return -rte_errno;
 }
 
 static int
-- 
2.43.0

Re: [PATCH v2] net/ice: fix how ice driver handles flows

[Bruce Richardson]

On Thu, Jan 30, 2025 at 04:14:42PM +0000, Vladimir Medvedkin wrote:
> Currently ICE PMD uses group attribute to select the appropriate HW engine
> to offload the flow. This behavior violates the rte_flow API, existing
> documentation/examples, and reveals hardware specific details.
> 
> This patch eliminates the use of the group attribute and runs each engine
> parser in the order they work in the HW pipeline.
> 
> Fixes: 9c5f0070fa3f ("net/ice: map group to pipeline stage")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

Thanks, this seems a great idea, to automatically put the flow in place.

Question: is there ever a scenario where a user may need, or want, to
override the automatic choice made here? Or is it just the case that once
the flow is installed, it works the same whatever group it's added into?
I would assume the latter, but just want to confirm.

/Bruce

Re: [PATCH v2] net/ice: fix how ice driver handles flows

[Medvedkin, Vladimir]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

Hi Bruce,

On 30/01/2025 17:54, Bruce Richardson wrote:
> On Thu, Jan 30, 2025 at 04:14:42PM +0000, Vladimir Medvedkin wrote:
>> Currently ICE PMD uses group attribute to select the appropriate HW engine
>> to offload the flow. This behavior violates the rte_flow API, existing
>> documentation/examples, and reveals hardware specific details.
>>
>> This patch eliminates the use of the group attribute and runs each engine
>> parser in the order they work in the HW pipeline.
>>
>> Fixes: 9c5f0070fa3f ("net/ice: map group to pipeline stage")
>> Cc:qi.z.zhang@intel.com
>> Cc:stable@dpdk.org
>>
>> Signed-off-by: Vladimir Medvedkin<vladimir.medvedkin@intel.com>
> Thanks, this seems a great idea, to automatically put the flow in place.
>
> Question: is there ever a scenario where a user may need, or want, to
> override the automatic choice made here? Or is it just the case that once
> the flow is installed, it works the same whatever group it's added into?
> I would assume the latter, but just want to confirm.

I don't think a user might have to worry about the internals of the NIC 
filters. Thesefiltersperformdifferent lookups: ACL provides wildcard 
matching, when FDIR in an exact match, so it depends on what kind of 
mask is used for a particular field, for example:

I want to install _exact_match_ rule:

- I use FDIR engine: flow create 0 ingress group 2 pattern eth / ipv4 
dst spec 192.168.0.1 dst mask 255.255.255.255 / end actions drop / end
and it works as expected.

- I use ACL engine: flow create 0 ingress group 2 pattern eth / ipv4 dst 
spec 192.168.0.1 dst mask 255.255.255.255 / end actions drop / end
and parser throws an error: Invalid IPv4 mask.: Invalid argument

I want to install _wildcard_match_ rule:

- I use FDIR engine: flow create 0 ingress group 2 pattern eth / ipv4 
dst spec 192.168.0.1 dst mask 255.255.255.254 / end actions drop / end
doesn't work, Invalid action.: Invalid argument

- I use ACL engine:flow create 0 ingress group 1 pattern eth / ipv4 dst 
spec 192.168.0.1 dst mask 255.255.0.255 / end actions drop / end
and it works as expected

> /Bruce

-- 
Regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 3804 bytes --]

Re: [PATCH v2] net/ice: fix how ice driver handles flows

[Bruce Richardson]

On Thu, Jan 30, 2025 at 04:14:42PM +0000, Vladimir Medvedkin wrote:
> Currently ICE PMD uses group attribute to select the appropriate HW engine
> to offload the flow. This behavior violates the rte_flow API, existing
> documentation/examples, and reveals hardware specific details.
> 
> This patch eliminates the use of the group attribute and runs each engine
> parser in the order they work in the HW pipeline.
> 
> Fixes: 9c5f0070fa3f ("net/ice: map group to pipeline stage")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied to dpdk-next-net-intel,
Thanks,
/Bruce

PS: for future reference, minor feedback inline below.
> ---
>  drivers/net/intel/ice/ice_generic_flow.c | 30 ++++++++++++++----------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/intel/ice/ice_generic_flow.c b/drivers/net/intel/ice/ice_generic_flow.c
> index 50d760004f..5c34e0385f 100644
> --- a/drivers/net/intel/ice/ice_generic_flow.c
> +++ b/drivers/net/intel/ice/ice_generic_flow.c
> @@ -20,6 +20,8 @@
>  
>  #define ICE_FLOW_ENGINE_DISABLED(mask, type) ((mask) & BIT(type))
>  
> +#define ICE_FLOW_ENGINE_NB	3
> +
>  static struct ice_engine_list engine_list =
>  		TAILQ_HEAD_INITIALIZER(engine_list);
>  
> @@ -2295,21 +2297,23 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
>  		return 0;
>  	}
>  
> -	parser = get_flow_parser(attr->group);
> -	if (parser == NULL) {
> -		rte_flow_error_set(error, EINVAL,
> -				   RTE_FLOW_ERROR_TYPE_ATTR,
> -				   NULL, "NULL attribute.");
> -		return -rte_errno;
> -	}
> +	for (int i = 0; i < ICE_FLOW_ENGINE_NB; i++) {
> +		parser = get_flow_parser(i);
> +		if (parser == NULL) {
> +			rte_flow_error_set(error, EINVAL,
> +					RTE_FLOW_ERROR_TYPE_ATTR,
> +					NULL, "NULL attribute.");

Since we allow up to 100 chars per line, this can be kept on two lines if
you want.

> +			return -rte_errno;
> +		}
>  
> -	if (ice_parse_engine(ad, flow, parser, attr->priority,
> -			     pattern, actions, error)) {
> -		*engine = parser->engine;
> -		return 0;
> -	} else {
> -		return -rte_errno;
> +		if (ice_parse_engine(ad, flow, parser, attr->priority,
> +				pattern, actions, error)) {

Similarly here, there is no need to wrap this statement - it all fits in
100 chars.

> +			*engine = parser->engine;
> +			return 0;
> +		}
>  	}
> +
> +	return -rte_errno;
>  }
>  
>  static int
> -- 
> 2.43.0
>