[dpdk-dev] [PATCH] net/i40e: fix flow RSS queue index check error

[dpdk-dev] [PATCH] net/i40e: fix flow RSS queue index check error

[Wei Zhao]

Ther is a error in queue index check for RSS queue region
configuration.

Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Tested-by: Peng Yuan <yuan.peng@intel.com>
---
 drivers/net/i40e/i40e_flow.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index f4d08bb..fb7ad51 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -4240,6 +4240,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 				return -rte_errno;
 			}
 		}
+
+		if (rss_info->num < rss->num) {
+			rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ACTION,
+				act,
+				"no valid queues");
+			return -rte_errno;
+		}
 	}
 
 	for (n = 0; n < conf_info->queue_region_number; n++) {
@@ -4264,17 +4272,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 				return -rte_errno;
 			}
 
-			if (rss_info->num < rss->num ||
-				rss->queue[0] < rss_info->queue[0] ||
-				(rss->queue[0] + rss->num >
-					rss_info->num + rss_info->queue[0])) {
-				rte_flow_error_set(error, EINVAL,
-					RTE_FLOW_ERROR_TYPE_ACTION,
-					act,
-					"no valid queues");
-				return -rte_errno;
-			}
-
 			for (i = 0; i < info->queue_region_number; i++) {
 				if (info->region[i].queue_num == rss->num &&
 					info->region[i].queue_start_index ==
-- 
2.7.5

Re: [dpdk-dev] [PATCH] net/i40e: fix flow RSS queue index check error

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Sunday, April 8, 2018 1:37 PM
> To: dev@dpdk.org; stable@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: [PATCH] net/i40e: fix flow RSS queue index check error
> 
> Ther is a error in queue index check for RSS queue region configuration.

What is the error?
Would you explain more and please add this in commit log?

Regards
Qi

> 
> Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Tested-by: Peng Yuan <yuan.peng@intel.com>
> ---
>  drivers/net/i40e/i40e_flow.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c index
> f4d08bb..fb7ad51 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
> @@ -4240,6 +4240,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev
> *dev,
>  				return -rte_errno;
>  			}
>  		}
> +
> +		if (rss_info->num < rss->num) {
> +			rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION,
> +				act,
> +				"no valid queues");
> +			return -rte_errno;
> +		}
>  	}
> 
>  	for (n = 0; n < conf_info->queue_region_number; n++) { @@ -4264,17
> +4272,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
>  				return -rte_errno;
>  			}
> 
> -			if (rss_info->num < rss->num ||
> -				rss->queue[0] < rss_info->queue[0] ||
> -				(rss->queue[0] + rss->num >
> -					rss_info->num + rss_info->queue[0])) {
> -				rte_flow_error_set(error, EINVAL,
> -					RTE_FLOW_ERROR_TYPE_ACTION,
> -					act,
> -					"no valid queues");
> -				return -rte_errno;
> -			}
> -
>  			for (i = 0; i < info->queue_region_number; i++) {
>  				if (info->region[i].queue_num == rss->num &&
>  					info->region[i].queue_start_index ==
> --
> 2.7.5

[dpdk-dev] [PATCH v2] net/i40e: fix flow RSS queue index check error

[Wei Zhao]

There is an error in queue index check for RSS queue region
configuration.If the queue index is not continuous sequence
for RSS, but queue region index is continuous sequence, in
this case we can not use the old method for queue index check.
This patch also add comment for flow rss parse function in order
to explain some important info.

Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Tested-by: Peng Yuan <yuan.peng@intel.com>

---

v2:
-merge this with another patch for function comment, add git log info.
---
 drivers/net/i40e/i40e_flow.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index f4d08bb..40f15c2 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused struct rte_eth_dev *dev,
 	return 0;
 }
 
+/**
+ * This function is used to parse rss queue index, total queue number and
+ * hash functions, If the purpose of this configuration is for queue region
+ * configuration, it will set queue_region_conf flag to TRUE, else to FALSE.
+ * In queue region configuration, it also need to parse hardware flowtype
+ * and user_priority from configuration, it will also cheeck the validity
+ * of these parameters. For example, The queue region sizes should
+ * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
+ * hw_flowtype or PCTYPE max index should be 63, the user priority
+ * max index should be 7, and so on. And also, queue index should be
+ * continuous sequence and queue region index should be part of rss
+ * queue index for this port.
+ */
 static int
 i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 			    const struct rte_flow_action *actions,
@@ -4240,6 +4253,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 				return -rte_errno;
 			}
 		}
+
+		if (rss_info->num < rss->num) {
+			rte_flow_error_set(error, EINVAL,
+				RTE_FLOW_ERROR_TYPE_ACTION,
+				act,
+				"no valid queues");
+			return -rte_errno;
+		}
 	}
 
 	for (n = 0; n < conf_info->queue_region_number; n++) {
@@ -4264,17 +4285,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 				return -rte_errno;
 			}
 
-			if (rss_info->num < rss->num ||
-				rss->queue[0] < rss_info->queue[0] ||
-				(rss->queue[0] + rss->num >
-					rss_info->num + rss_info->queue[0])) {
-				rte_flow_error_set(error, EINVAL,
-					RTE_FLOW_ERROR_TYPE_ACTION,
-					act,
-					"no valid queues");
-				return -rte_errno;
-			}
-
 			for (i = 0; i < info->queue_region_number; i++) {
 				if (info->region[i].queue_num == rss->num &&
 					info->region[i].queue_start_index ==
-- 
2.7.5

Re: [dpdk-dev] [PATCH v2] net/i40e: fix flow RSS queue index check error

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Monday, April 9, 2018 4:38 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [PATCH v2] net/i40e: fix flow RSS queue index check error
> 
> There is an error in queue index check for RSS queue region configuration.If
> the queue index is not continuous sequence for RSS, but queue region index is
> continuous sequence, in this case we can not use the old method for queue
> index check.
> This patch also add comment for flow rss parse function in order to explain
> some important info.
> 
> Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Tested-by: Peng Yuan <yuan.peng@intel.com>
> 
> ---
> 
> v2:
> -merge this with another patch for function comment, add git log info.
> ---
>  drivers/net/i40e/i40e_flow.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c index
> f4d08bb..40f15c2 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
> @@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused struct
> rte_eth_dev *dev,
>  	return 0;
>  }
> 
> +/**
> + * This function is used to parse rss queue index, total queue number
> +and
> + * hash functions, If the purpose of this configuration is for queue
> +region
> + * configuration, it will set queue_region_conf flag to TRUE, else to FALSE.
> + * In queue region configuration, it also need to parse hardware
> +flowtype
> + * and user_priority from configuration, it will also cheeck the
> +validity
> + * of these parameters. For example, The queue region sizes should
> + * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
> + * hw_flowtype or PCTYPE max index should be 63, the user priority
> + * max index should be 7, and so on. And also, queue index should be
> + * continuous sequence and queue region index should be part of rss
> + * queue index for this port.
> + */
>  static int
>  i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
>  			    const struct rte_flow_action *actions, @@ -4240,6
> +4253,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
>  				return -rte_errno;
>  			}
>  		}
> +
> +		if (rss_info->num < rss->num) {
> +			rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION,
> +				act,
> +				"no valid queues");
> +			return -rte_errno;
> +		}
>  	}
> 
>  	for (n = 0; n < conf_info->queue_region_number; n++) { @@ -4264,17
> +4285,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
>  				return -rte_errno;
>  			}
> 
> -			if (rss_info->num < rss->num ||
> -				rss->queue[0] < rss_info->queue[0] ||
> -				(rss->queue[0] + rss->num >
> -					rss_info->num + rss_info->queue[0])) {
> -				rte_flow_error_set(error, EINVAL,
> -					RTE_FLOW_ERROR_TYPE_ACTION,
> -					act,
> -					"no valid queues");
> -				return -rte_errno;
> -			}

Is this a code clean?
I didn't see anything wrong with the code you removed, but they just looks redundant.
Btw why you still check (rss_info->num < rss->num) ?, 
previous, I think you already iterate all rss queue to make sure they are matched by a rss_info queue
so it already proved rss_info->num >= rss->num.


>  			for (i = 0; i < info->queue_region_number; i++) {
>  				if (info->region[i].queue_num == rss->num &&
>  					info->region[i].queue_start_index ==
> --
> 2.7.5

Re: [dpdk-dev] [PATCH v2] net/i40e: fix flow RSS queue index check error

[Zhao1, Wei]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, April 9, 2018 9:57 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, April 9, 2018 4:38 PM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [PATCH v2] net/i40e: fix flow RSS queue index check error
> >
> > There is an error in queue index check for RSS queue region
> > configuration.If the queue index is not continuous sequence for RSS,
> > but queue region index is continuous sequence, in this case we can not
> > use the old method for queue index check.
> > This patch also add comment for flow rss parse function in order to
> > explain some important info.
> >
> > Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Tested-by: Peng Yuan <yuan.peng@intel.com>
> >
> > ---
> >
> > v2:
> > -merge this with another patch for function comment, add git log info.
> > ---
> >  drivers/net/i40e/i40e_flow.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_flow.c
> > b/drivers/net/i40e/i40e_flow.c index
> > f4d08bb..40f15c2 100644
> > --- a/drivers/net/i40e/i40e_flow.c
> > +++ b/drivers/net/i40e/i40e_flow.c
> > @@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused
> struct
> > rte_eth_dev *dev,
> >  	return 0;
> >  }
> >
> > +/**
> > + * This function is used to parse rss queue index, total queue number
> > +and
> > + * hash functions, If the purpose of this configuration is for queue
> > +region
> > + * configuration, it will set queue_region_conf flag to TRUE, else to FALSE.
> > + * In queue region configuration, it also need to parse hardware
> > +flowtype
> > + * and user_priority from configuration, it will also cheeck the
> > +validity
> > + * of these parameters. For example, The queue region sizes should
> > + * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
> > + * hw_flowtype or PCTYPE max index should be 63, the user priority
> > + * max index should be 7, and so on. And also, queue index should be
> > + * continuous sequence and queue region index should be part of rss
> > + * queue index for this port.
> > + */
> >  static int
> >  i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> >  			    const struct rte_flow_action *actions, @@ -4240,6
> > +4253,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> >  				return -rte_errno;
> >  			}
> >  		}
> > +
> > +		if (rss_info->num < rss->num) {
> > +			rte_flow_error_set(error, EINVAL,
> > +				RTE_FLOW_ERROR_TYPE_ACTION,
> > +				act,
> > +				"no valid queues");
> > +			return -rte_errno;
> > +		}
> >  	}
> >
> >  	for (n = 0; n < conf_info->queue_region_number; n++) { @@ -
> 4264,17
> > +4285,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> >  				return -rte_errno;
> >  			}
> >
> > -			if (rss_info->num < rss->num ||
> > -				rss->queue[0] < rss_info->queue[0] ||
> > -				(rss->queue[0] + rss->num >
> > -					rss_info->num + rss_info->queue[0]))
> {
> > -				rte_flow_error_set(error, EINVAL,
> > -					RTE_FLOW_ERROR_TYPE_ACTION,
> > -					act,
> > -					"no valid queues");
> > -				return -rte_errno;
> > -			}
> 
> Is this a code clean?
> I didn't see anything wrong with the code you removed, but they just looks
> redundant.
> Btw why you still check (rss_info->num < rss->num) ?, previous, I think you
> already iterate all rss queue to make sure they are matched by a rss_info
> queue so it already proved rss_info->num >= rss->num.

what you said is reasonable, so I will commit v3 To delete this (rss_info->num < rss->num) check.

> 
> 
> >  			for (i = 0; i < info->queue_region_number; i++) {
> >  				if (info->region[i].queue_num == rss->num
> &&
> >  					info->region[i].queue_start_index ==
> > --
> > 2.7.5

Re: [dpdk-dev] [PATCH v2] net/i40e: fix flow RSS queue index check error

[Zhao1, Wei]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, April 9, 2018 9:57 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, April 9, 2018 4:38 PM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [PATCH v2] net/i40e: fix flow RSS queue index check error
> >
> > There is an error in queue index check for RSS queue region
> > configuration.If the queue index is not continuous sequence for RSS,
> > but queue region index is continuous sequence, in this case we can not
> > use the old method for queue index check.
> > This patch also add comment for flow rss parse function in order to
> > explain some important info.
> >
> > Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Tested-by: Peng Yuan <yuan.peng@intel.com>
> >
> > ---
> >
> > v2:
> > -merge this with another patch for function comment, add git log info.
> > ---
> >  drivers/net/i40e/i40e_flow.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_flow.c
> > b/drivers/net/i40e/i40e_flow.c index
> > f4d08bb..40f15c2 100644
> > --- a/drivers/net/i40e/i40e_flow.c
> > +++ b/drivers/net/i40e/i40e_flow.c
> > @@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused
> struct
> > rte_eth_dev *dev,
> >  	return 0;
> >  }
> >
> > +/**
> > + * This function is used to parse rss queue index, total queue number
> > +and
> > + * hash functions, If the purpose of this configuration is for queue
> > +region
> > + * configuration, it will set queue_region_conf flag to TRUE, else to FALSE.
> > + * In queue region configuration, it also need to parse hardware
> > +flowtype
> > + * and user_priority from configuration, it will also cheeck the
> > +validity
> > + * of these parameters. For example, The queue region sizes should
> > + * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
> > + * hw_flowtype or PCTYPE max index should be 63, the user priority
> > + * max index should be 7, and so on. And also, queue index should be
> > + * continuous sequence and queue region index should be part of rss
> > + * queue index for this port.
> > + */
> >  static int
> >  i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> >  			    const struct rte_flow_action *actions, @@ -4240,6
> > +4253,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> >  				return -rte_errno;
> >  			}
> >  		}
> > +
> > +		if (rss_info->num < rss->num) {
> > +			rte_flow_error_set(error, EINVAL,
> > +				RTE_FLOW_ERROR_TYPE_ACTION,
> > +				act,
> > +				"no valid queues");
> > +			return -rte_errno;
> > +		}
> >  	}
> >
> >  	for (n = 0; n < conf_info->queue_region_number; n++) { @@ -
> 4264,17
> > +4285,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> >  				return -rte_errno;
> >  			}
> >
> > -			if (rss_info->num < rss->num ||
> > -				rss->queue[0] < rss_info->queue[0] ||
> > -				(rss->queue[0] + rss->num >
> > -					rss_info->num + rss_info->queue[0]))
> {
> > -				rte_flow_error_set(error, EINVAL,
> > -					RTE_FLOW_ERROR_TYPE_ACTION,
> > -					act,
> > -					"no valid queues");
> > -				return -rte_errno;
> > -			}
> 
> Is this a code clean?
> I didn't see anything wrong with the code you removed, but they just looks
> redundant.
> Btw why you still check (rss_info->num < rss->num) ?, previous, I think you
> already iterate all rss queue to make sure they are matched by a rss_info
> queue so it already proved rss_info->num >= rss->num.

After thinking it over, I think we had better keep this check for (rss_info->num < rss->num)
because customer may input a wrong number for this parameter, although queue index maybe rightly
match rss span. So, v3 may be not need.
 

> 
> 
> >  			for (i = 0; i < info->queue_region_number; i++) {
> >  				if (info->region[i].queue_num == rss->num
> &&
> >  					info->region[i].queue_start_index ==
> > --
> > 2.7.5

Re: [dpdk-dev] [PATCH v2] net/i40e: fix flow RSS queue index check error

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, April 10, 2018 10:19 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check error
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, April 9, 2018 9:57 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check error
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Monday, April 9, 2018 4:38 PM
> > > To: dev@dpdk.org
> > > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > > <wei.zhao1@intel.com>
> > > Subject: [PATCH v2] net/i40e: fix flow RSS queue index check error
> > >
> > > There is an error in queue index check for RSS queue region
> > > configuration.If the queue index is not continuous sequence for RSS,
> > > but queue region index is continuous sequence, in this case we can
> > > not use the old method for queue index check.
> > > This patch also add comment for flow rss parse function in order to
> > > explain some important info.
> > >
> > > Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > Tested-by: Peng Yuan <yuan.peng@intel.com>
> > >
> > > ---
> > >
> > > v2:
> > > -merge this with another patch for function comment, add git log info.
> > > ---
> > >  drivers/net/i40e/i40e_flow.c | 32 +++++++++++++++++++++-----------
> > >  1 file changed, 21 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_flow.c
> > > b/drivers/net/i40e/i40e_flow.c index
> > > f4d08bb..40f15c2 100644
> > > --- a/drivers/net/i40e/i40e_flow.c
> > > +++ b/drivers/net/i40e/i40e_flow.c
> > > @@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused
> > struct
> > > rte_eth_dev *dev,
> > >  	return 0;
> > >  }
> > >
> > > +/**
> > > + * This function is used to parse rss queue index, total queue
> > > +number and
> > > + * hash functions, If the purpose of this configuration is for
> > > +queue region
> > > + * configuration, it will set queue_region_conf flag to TRUE, else to FALSE.
> > > + * In queue region configuration, it also need to parse hardware
> > > +flowtype
> > > + * and user_priority from configuration, it will also cheeck the
> > > +validity
> > > + * of these parameters. For example, The queue region sizes should
> > > + * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
> > > + * hw_flowtype or PCTYPE max index should be 63, the user priority
> > > + * max index should be 7, and so on. And also, queue index should
> > > +be
> > > + * continuous sequence and queue region index should be part of rss
> > > + * queue index for this port.
> > > + */
> > >  static int
> > >  i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > >  			    const struct rte_flow_action *actions, @@ -4240,6
> > > +4253,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > >  				return -rte_errno;
> > >  			}
> > >  		}
> > > +
> > > +		if (rss_info->num < rss->num) {
> > > +			rte_flow_error_set(error, EINVAL,
> > > +				RTE_FLOW_ERROR_TYPE_ACTION,
> > > +				act,
> > > +				"no valid queues");
> > > +			return -rte_errno;
> > > +		}
> > >  	}
> > >
> > >  	for (n = 0; n < conf_info->queue_region_number; n++) { @@ -
> > 4264,17
> > > +4285,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > >  				return -rte_errno;
> > >  			}
> > >
> > > -			if (rss_info->num < rss->num ||
> > > -				rss->queue[0] < rss_info->queue[0] ||
> > > -				(rss->queue[0] + rss->num >
> > > -					rss_info->num + rss_info->queue[0]))
> > {
> > > -				rte_flow_error_set(error, EINVAL,
> > > -					RTE_FLOW_ERROR_TYPE_ACTION,
> > > -					act,
> > > -					"no valid queues");
> > > -				return -rte_errno;
> > > -			}
> >
> > Is this a code clean?
> > I didn't see anything wrong with the code you removed, but they just
> > looks redundant.
> > Btw why you still check (rss_info->num < rss->num) ?, previous, I
> > think you already iterate all rss queue to make sure they are matched
> > by a rss_info queue so it already proved rss_info->num >= rss->num.
> 
> After thinking it over, I think we had better keep this check for (rss_info->num
> < rss->num) because customer may input a wrong number for this parameter,
> although queue index maybe rightly match rss span. So, v3 may be not need.


If rss->num > rss_info->num, it should already failed in below code, so I still think it's no necessary.

			/* each rss queue must match a rss_info queue */
			for (i = 0; i < rss->num; i++) {
                        for (j = 0; j < rss_info->num; j++) {
                                if (rss->queue[i] == rss_info->queue[j])
                                        break;
                        }
                        if (j == rss_info->num) {
                                rte_flow_error_set(error, EINVAL,
                                        RTE_FLOW_ERROR_TYPE_ACTION,
                                        act,
                                        "no valid queues");
                                return -rte_errno;
                        }
                }
				
				/* rss queue must be continues */
                for (i = 0; i < rss->num - 1; i++) {
                        if (rss->queue[i + 1] != rss->queue[i] + 1) {
                                rte_flow_error_set(error, EINVAL,
                                        RTE_FLOW_ERROR_TYPE_ACTION,
                                        act,
                                        "no valid queues");
                                return -rte_errno;
                        }
                }

> 
> 
> >
> >
> > >  			for (i = 0; i < info->queue_region_number; i++) {
> > >  				if (info->region[i].queue_num == rss->num
> > &&
> > >  					info->region[i].queue_start_index ==
> > > --
> > > 2.7.5

Re: [dpdk-dev] [PATCH v2] net/i40e: fix flow RSS queue index check error

[Zhao1, Wei]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, April 10, 2018 6:45 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Tuesday, April 10, 2018 10:19 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check error
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Monday, April 9, 2018 9:57 PM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org
> > > Subject: RE: [PATCH v2] net/i40e: fix flow RSS queue index check
> > > error
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Zhao1, Wei
> > > > Sent: Monday, April 9, 2018 4:38 PM
> > > > To: dev@dpdk.org
> > > > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1,
> > > > Wei <wei.zhao1@intel.com>
> > > > Subject: [PATCH v2] net/i40e: fix flow RSS queue index check error
> > > >
> > > > There is an error in queue index check for RSS queue region
> > > > configuration.If the queue index is not continuous sequence for
> > > > RSS, but queue region index is continuous sequence, in this case
> > > > we can not use the old method for queue index check.
> > > > This patch also add comment for flow rss parse function in order
> > > > to explain some important info.
> > > >
> > > > Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> > > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > > Tested-by: Peng Yuan <yuan.peng@intel.com>
> > > >
> > > > ---
> > > >
> > > > v2:
> > > > -merge this with another patch for function comment, add git log info.
> > > > ---
> > > >  drivers/net/i40e/i40e_flow.c | 32
> > > > +++++++++++++++++++++-----------
> > > >  1 file changed, 21 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/net/i40e/i40e_flow.c
> > > > b/drivers/net/i40e/i40e_flow.c index
> > > > f4d08bb..40f15c2 100644
> > > > --- a/drivers/net/i40e/i40e_flow.c
> > > > +++ b/drivers/net/i40e/i40e_flow.c
> > > > @@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused
> > > struct
> > > > rte_eth_dev *dev,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +/**
> > > > + * This function is used to parse rss queue index, total queue
> > > > +number and
> > > > + * hash functions, If the purpose of this configuration is for
> > > > +queue region
> > > > + * configuration, it will set queue_region_conf flag to TRUE, else to
> FALSE.
> > > > + * In queue region configuration, it also need to parse hardware
> > > > +flowtype
> > > > + * and user_priority from configuration, it will also cheeck the
> > > > +validity
> > > > + * of these parameters. For example, The queue region sizes
> > > > +should
> > > > + * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
> > > > + * hw_flowtype or PCTYPE max index should be 63, the user
> > > > +priority
> > > > + * max index should be 7, and so on. And also, queue index should
> > > > +be
> > > > + * continuous sequence and queue region index should be part of
> > > > +rss
> > > > + * queue index for this port.
> > > > + */
> > > >  static int
> > > >  i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > > >  			    const struct rte_flow_action *actions, @@ -4240,6
> > > > +4253,14 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > > >  				return -rte_errno;
> > > >  			}
> > > >  		}
> > > > +
> > > > +		if (rss_info->num < rss->num) {
> > > > +			rte_flow_error_set(error, EINVAL,
> > > > +				RTE_FLOW_ERROR_TYPE_ACTION,
> > > > +				act,
> > > > +				"no valid queues");
> > > > +			return -rte_errno;
> > > > +		}
> > > >  	}
> > > >
> > > >  	for (n = 0; n < conf_info->queue_region_number; n++) { @@ -
> > > 4264,17
> > > > +4285,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > > >  				return -rte_errno;
> > > >  			}
> > > >
> > > > -			if (rss_info->num < rss->num ||
> > > > -				rss->queue[0] < rss_info->queue[0] ||
> > > > -				(rss->queue[0] + rss->num >
> > > > -					rss_info->num + rss_info->queue[0]))
> > > {
> > > > -				rte_flow_error_set(error, EINVAL,
> > > > -					RTE_FLOW_ERROR_TYPE_ACTION,
> > > > -					act,
> > > > -					"no valid queues");
> > > > -				return -rte_errno;
> > > > -			}
> > >
> > > Is this a code clean?
> > > I didn't see anything wrong with the code you removed, but they just
> > > looks redundant.
> > > Btw why you still check (rss_info->num < rss->num) ?, previous, I
> > > think you already iterate all rss queue to make sure they are
> > > matched by a rss_info queue so it already proved rss_info->num >= rss-
> >num.
> >
> > After thinking it over, I think we had better keep this check for
> > (rss_info->num < rss->num) because customer may input a wrong number
> > for this parameter, although queue index maybe rightly match rss span. So,
> v3 may be not need.
> 
> 
> If rss->num > rss_info->num, it should already failed in below code, so I still
> think it's no necessary.
> 
> 			/* each rss queue must match a rss_info queue */
> 			for (i = 0; i < rss->num; i++) {
>                         for (j = 0; j < rss_info->num; j++) {
>                                 if (rss->queue[i] == rss_info->queue[j])
>                                         break;
>                         }
>                         if (j == rss_info->num) {
>                                 rte_flow_error_set(error, EINVAL,
>                                         RTE_FLOW_ERROR_TYPE_ACTION,
>                                         act,
>                                         "no valid queues");
>                                 return -rte_errno;
>                         }
>                 }
> 
> 				/* rss queue must be continues */
>                 for (i = 0; i < rss->num - 1; i++) {
>                         if (rss->queue[i + 1] != rss->queue[i] + 1) {
>                                 rte_flow_error_set(error, EINVAL,
>                                         RTE_FLOW_ERROR_TYPE_ACTION,
>                                         act,
>                                         "no valid queues");
>                                 return -rte_errno;
>                         }
>                 }

Ok, I will commit v3, thank you for your explanation.

> 
> >
> >
> > >
> > >
> > > >  			for (i = 0; i < info->queue_region_number; i++) {
> > > >  				if (info->region[i].queue_num == rss->num
> > > &&
> > > >  					info->region[i].queue_start_index ==
> > > > --
> > > > 2.7.5

[dpdk-dev] [PATCH v3] net/i40e: add comment and clean code for flow RSS

[Wei Zhao]

This patch add comment for flow rss parse function in order
to explain some important info. This patch also delete some
useless code for queue index check for RSS queue region
configuration.

Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Tested-by: Peng Yuan <yuan.peng@intel.com>

---

v2:
-merge this with another patch for function comment, add git log info.

v3:
-add more comment code and clean all useless parameter check code.

Signed-off-by: Zhao Wei <wei.zhao1@intel.com>
---
 drivers/net/i40e/i40e_flow.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index f4d08bb..5f78a15 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -4171,6 +4171,19 @@ i40e_flow_parse_rss_pattern(__rte_unused struct rte_eth_dev *dev,
 	return 0;
 }
 
+/**
+ * This function is used to parse rss queue index, total queue number and
+ * hash functions, If the purpose of this configuration is for queue region
+ * configuration, it will set queue_region_conf flag to TRUE, else to FALSE.
+ * In queue region configuration, it also need to parse hardware flowtype
+ * and user_priority from configuration, it will also cheeck the validity
+ * of these parameters. For example, The queue region sizes should
+ * be any of the following values: 1, 2, 4, 8, 16, 32, 64, the
+ * hw_flowtype or PCTYPE max index should be 63, the user priority
+ * max index should be 7, and so on. And also, queue index should be
+ * continuous sequence and queue region index should be part of rss
+ * queue index for this port.
+ */
 static int
 i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 			    const struct rte_flow_action *actions,
@@ -4216,6 +4229,12 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 		}
 	}
 
+	/**
+	 * Do some queue region related parameters check
+	 * in order to keep queue index for queue region to be
+	 * continuous sequence and also to be part of RSS
+	 * queue index for this port.
+	 */
 	if (conf_info->queue_region_number) {
 		for (i = 0; i < rss->num; i++) {
 			for (j = 0; j < rss_info->num; j++) {
@@ -4242,6 +4261,7 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 		}
 	}
 
+	/* Parse queue region related parameters from configuration */
 	for (n = 0; n < conf_info->queue_region_number; n++) {
 		if (conf_info->region[n].user_priority_num ||
 				conf_info->region[n].flowtype_num) {
@@ -4264,17 +4284,6 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 				return -rte_errno;
 			}
 
-			if (rss_info->num < rss->num ||
-				rss->queue[0] < rss_info->queue[0] ||
-				(rss->queue[0] + rss->num >
-					rss_info->num + rss_info->queue[0])) {
-				rte_flow_error_set(error, EINVAL,
-					RTE_FLOW_ERROR_TYPE_ACTION,
-					act,
-					"no valid queues");
-				return -rte_errno;
-			}
-
 			for (i = 0; i < info->queue_region_number; i++) {
 				if (info->region[i].queue_num == rss->num &&
 					info->region[i].queue_start_index ==
@@ -4329,6 +4338,9 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 		rss_config->queue_region_conf = TRUE;
 	}
 
+	/**
+	 * Return function if this flow is used for queue region configuration
+	 */
 	if (rss_config->queue_region_conf)
 		return 0;
 
@@ -4349,6 +4361,8 @@ i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
 			return -rte_errno;
 		}
 	}
+
+	/* Parse RSS related parameters from configuration */
 	if (rss->rss_conf)
 		rss_config->rss_conf = *rss->rss_conf;
 	else
-- 
2.7.5

Re: [dpdk-dev] [PATCH v3] net/i40e: add comment and clean code for flow RSS

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhao1, Wei
> Sent: Wednesday, April 11, 2018 11:25 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [PATCH v3] net/i40e: add comment and clean code for flow RSS
> 
> This patch add comment for flow rss parse function in order to explain some
> important info. This patch also delete some useless code for queue index
> check for RSS queue region configuration.
> 
> Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Tested-by: Peng Yuan <yuan.peng@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

S /"Tested-by: Peng Yuan" / "Tested-by: Yuan Peng" 
 

Re: [dpdk-dev] [PATCH v3] net/i40e: add comment and clean code for flow RSS

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zhang, Qi Z
> Sent: Wednesday, April 11, 2018 8:30 PM
> To: Zhao1, Wei; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] net/i40e: add comment and clean code for
> flow RSS
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Wednesday, April 11, 2018 11:25 AM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [PATCH v3] net/i40e: add comment and clean code for flow RSS
> >
> > This patch add comment for flow rss parse function in order to explain
> > some important info. This patch also delete some useless code for
> > queue index check for RSS queue region configuration.
> >
> > Fixes: ecad87d22383 ("net/i40e: move RSS to flow API")
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Tested-by: Peng Yuan <yuan.peng@intel.com>
> 
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Applied to dpdk-next-net-intel, thanks!

/Helin
> 
> S /"Tested-by: Peng Yuan" / "Tested-by: Yuan Peng"
>