[dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Nikhil Rao]

RED queue weight is currently specified as a negated log of 2.

Add support for RED queue weight to be specified in double precision
and TM capability flags for double precision and negated log2
RED queue weight support.

Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
---
 lib/librte_ethdev/rte_tm.h               | 41 ++++++++++++++++++++++++++++++--
 drivers/net/softnic/rte_eth_softnic_tm.c |  7 ++++--
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
index 646ef38..b13454d 100644
--- a/lib/librte_ethdev/rte_tm.h
+++ b/lib/librte_ethdev/rte_tm.h
@@ -393,6 +393,22 @@ struct rte_tm_capabilities {
 	 */
 	int cman_wred_byte_mode_supported;
 
+	/** Double precision RED queue weight support. When non-zero, this
+	 * this parameter indicates that RED queue weight in double precision
+	 * format is supported.
+	 * @see struct rte_tm_red_params::wq_is_log2
+	 * @see struct rte_tm_red_params::wq_dp
+	 */
+	int cman_wred_wq_dp_supported;
+
+	/** Negated log2 RED queue weight support. When non-zero, this
+	 * parameter indicates that RED queue weight in negated log2 format
+	 * is supported.
+	 * @see struct rte_tm_red_params::wq_is_log2
+	 * @see struct rte_tm_red_params::wq_log2
+	 */
+	int cman_wred_wq_log2_supported;
+
 	/** Head drop algorithm support. When non-zero, this parameter
 	 * indicates that there is at least one leaf node that supports the head
 	 * drop algorithm, which might not be true for all the leaf nodes.
@@ -841,8 +857,27 @@ struct rte_tm_red_params {
 	 */
 	uint16_t maxp_inv;
 
-	/** Negated log2 of queue weight (wq), i.e. wq = 1 / (2 ^ wq_log2) */
-	uint16_t wq_log2;
+	/** When non-zero, RED queue weight (wq) is negated log2 of queue
+	 * weight
+	 *
+	 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
+	 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
+	 */
+	int wq_is_log2;
+
+	union {
+		/** Double precision queue weight
+		 *
+		 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
+		 */
+		double wq_dp;
+		/** Negated log2 of queue weight (wq),
+		 * i.e. wq = 1 / (2 ^ wq_log2)
+		 *
+		 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
+		 */
+		uint16_t wq_log2;
+	};
 };
 
 /**
@@ -858,6 +893,8 @@ struct rte_tm_red_params {
  *
  * @see struct rte_tm_capabilities::cman_wred_packet_mode_supported
  * @see struct rte_tm_capabilities::cman_wred_byte_mode_supported
+ * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
+ * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
  */
 struct rte_tm_wred_params {
 	/** One set of RED parameters per packet color */
diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c b/drivers/net/softnic/rte_eth_softnic_tm.c
index baaafbe..e96ea8e 100644
--- a/drivers/net/softnic/rte_eth_softnic_tm.c
+++ b/drivers/net/softnic/rte_eth_softnic_tm.c
@@ -469,7 +469,8 @@ struct softnic_tmgr_port *
 	.cman_wred_context_shared_n_max = 0,
 	.cman_wred_context_shared_n_nodes_per_context_max = 0,
 	.cman_wred_context_shared_n_contexts_per_node_max = 0,
-
+	.cman_wred_wq_dp_supported = 0,
+	.cman_wred_wq_log2_supported = WRED_SUPPORTED,
 	.mark_vlan_dei_supported = {0, 0, 0},
 	.mark_ip_ecn_tcp_supported = {0, 0, 0},
 	.mark_ip_ecn_sctp_supported = {0, 0, 0},
@@ -1243,8 +1244,10 @@ struct softnic_tmgr_port *
 	for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
 		uint32_t min_th = profile->red_params[color].min_th;
 		uint32_t max_th = profile->red_params[color].max_th;
+		int wq_is_log2 = profile->red_params[color].wq_is_log2;
 
-		if (min_th > max_th ||
+		if (wq_is_log2 == 0 ||
+			min_th > max_th ||
 			max_th == 0 ||
 			min_th > UINT16_MAX ||
 			max_th > UINT16_MAX)
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Stephen Hemminger]

On Thu, 29 Nov 2018 11:24:42 +0530
Nikhil Rao <nikhil.rao@intel.com> wrote:

> RED queue weight is currently specified as a negated log of 2.
> 
> Add support for RED queue weight to be specified in double precision
> and TM capability flags for double precision and negated log2
> RED queue weight support.
> 
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>

Since this is an ABI break anyway, why not just commit to the new
format?

Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Rao, Nikhil]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, November 29, 2018 11:43 AM
> To: Rao, Nikhil <nikhil.rao@intel.com>
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue
> weight
> 
> On Thu, 29 Nov 2018 11:24:42 +0530
> Nikhil Rao <nikhil.rao@intel.com> wrote:
> 
> > RED queue weight is currently specified as a negated log of 2.
> >
> > Add support for RED queue weight to be specified in double precision
> > and TM capability flags for double precision and negated log2 RED
> > queue weight support.
> >
> > Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> 
> Since this is an ABI break anyway, why not just commit to the new format?

Hi Stephen,

Can you please provide more detail on your comment ?  are you suggesting replacing the wq_log2/wq_dp with a double ?

Thanks,
Nikhil

Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Stephen Hemminger]

On Thu, 29 Nov 2018 11:24:42 +0530
Nikhil Rao <nikhil.rao@intel.com> wrote:

>  
> +	/** Double precision RED queue weight support. When non-zero, this
> +	 * this parameter indicates that RED queue weight in double precision
> +	 * format is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_dp
> +	 */
> +	int cman_wred_wq_dp_supported;
> +
> +	/** Negated log2 RED queue weight support. When non-zero, this
> +	 * parameter indicates that RED queue weight in negated log2 format
> +	 * is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_log2
> +	 */
> +	int cman_wred_wq_log2_supported;
> +

One suggestion:
Since these are flag values use u8. Takes less space.
And maybe use pahole utility to find existing hole to put
them in.

Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Stephen Hemminger]

On Mon, 10 Dec 2018 05:43:37 +0000
"Rao, Nikhil" <nikhil.rao@intel.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Thursday, November 29, 2018 11:43 AM
> > To: Rao, Nikhil <nikhil.rao@intel.com>
> > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue
> > weight
> > 
> > On Thu, 29 Nov 2018 11:24:42 +0530
> > Nikhil Rao <nikhil.rao@intel.com> wrote:
> >   
> > > RED queue weight is currently specified as a negated log of 2.
> > >
> > > Add support for RED queue weight to be specified in double precision
> > > and TM capability flags for double precision and negated log2 RED
> > > queue weight support.
> > >
> > > Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>  
> > 
> > Since this is an ABI break anyway, why not just commit to the new format?  
> 
> Hi Stephen,
> 
> Can you please provide more detail on your comment ?  are you suggesting replacing the wq_log2/wq_dp with a double ?
> 
> Thanks,
> Nikhil


My comment is that since you are changing a structure layout, which would
break existing users; why not go farther and just fix the API to a better
version. I don't think any projects use this code anyway,
see my talk (https://github.com/shemminger/dpdk-metrics).

Isn't floating point going to be expensive. Or is it only during the setup
process, not enqueue/dequeue.

Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Dumitrescu, Cristian]

Hi Nikhil,

> -----Original Message-----
> From: Rao, Nikhil
> Sent: Thursday, November 29, 2018 5:55 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>
> Subject: [PATCH] ethdev: support double precision RED queue weight
> 
> RED queue weight is currently specified as a negated log of 2.
> 
> Add support for RED queue weight to be specified in double precision
> and TM capability flags for double precision and negated log2
> RED queue weight support.
> 
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> ---
>  lib/librte_ethdev/rte_tm.h               | 41
> ++++++++++++++++++++++++++++++--
>  drivers/net/softnic/rte_eth_softnic_tm.c |  7 ++++--
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index 646ef38..b13454d 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -393,6 +393,22 @@ struct rte_tm_capabilities {
>  	 */
>  	int cman_wred_byte_mode_supported;
> 
> +	/** Double precision RED queue weight support. When non-zero,
> this
> +	 * this parameter indicates that RED queue weight in double precision
> +	 * format is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_dp
> +	 */
> +	int cman_wred_wq_dp_supported;
> +
> +	/** Negated log2 RED queue weight support. When non-zero, this
> +	 * parameter indicates that RED queue weight in negated log2 format
> +	 * is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_log2
> +	 */
> +	int cman_wred_wq_log2_supported;
> +
>  	/** Head drop algorithm support. When non-zero, this parameter
>  	 * indicates that there is at least one leaf node that supports the
> head
>  	 * drop algorithm, which might not be true for all the leaf nodes.
> @@ -841,8 +857,27 @@ struct rte_tm_red_params {
>  	 */
>  	uint16_t maxp_inv;
> 
> -	/** Negated log2 of queue weight (wq), i.e. wq = 1 / (2 ^ wq_log2)
> */
> -	uint16_t wq_log2;
> +	/** When non-zero, RED queue weight (wq) is negated log2 of
> queue
> +	 * weight
> +	 *
> +	 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
> +	 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
> +	 */
> +	int wq_is_log2;
> +

Anonymous unions have to be declared with RTE_STD_C11:

RTE_STD_C11
union {
...
};

Look for some examples of anonymous unions in DPDK.

> +	union {
> +		/** Double precision queue weight
> +		 *
> +		 * @see struct
> rte_tm_capabilities::cman_wred_wq_dp_supported
> +		 */
> +		double wq_dp;
> +		/** Negated log2 of queue weight (wq),
> +		 * i.e. wq = 1 / (2 ^ wq_log2)
> +		 *
> +		 * @see struct
> rte_tm_capabilities::cman_wred_wq_log2_supported
> +		 */
> +		uint16_t wq_log2;
> +	};
>  };
> 
>  /**
> @@ -858,6 +893,8 @@ struct rte_tm_red_params {
>   *
>   * @see struct rte_tm_capabilities::cman_wred_packet_mode_supported
>   * @see struct rte_tm_capabilities::cman_wred_byte_mode_supported
> + * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
> + * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
>   */
>  struct rte_tm_wred_params {
>  	/** One set of RED parameters per packet color */
> diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c
> b/drivers/net/softnic/rte_eth_softnic_tm.c
> index baaafbe..e96ea8e 100644
> --- a/drivers/net/softnic/rte_eth_softnic_tm.c
> +++ b/drivers/net/softnic/rte_eth_softnic_tm.c
> @@ -469,7 +469,8 @@ struct softnic_tmgr_port *
>  	.cman_wred_context_shared_n_max = 0,
>  	.cman_wred_context_shared_n_nodes_per_context_max = 0,
>  	.cman_wred_context_shared_n_contexts_per_node_max = 0,
> -
> +	.cman_wred_wq_dp_supported = 0,
> +	.cman_wred_wq_log2_supported = WRED_SUPPORTED,
>  	.mark_vlan_dei_supported = {0, 0, 0},
>  	.mark_ip_ecn_tcp_supported = {0, 0, 0},
>  	.mark_ip_ecn_sctp_supported = {0, 0, 0},
> @@ -1243,8 +1244,10 @@ struct softnic_tmgr_port *
>  	for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
>  		uint32_t min_th = profile->red_params[color].min_th;
>  		uint32_t max_th = profile->red_params[color].max_th;
> +		int wq_is_log2 = profile->red_params[color].wq_is_log2;
> 
> -		if (min_th > max_th ||
> +		if (wq_is_log2 == 0 ||
> +			min_th > max_th ||
>  			max_th == 0 ||
>  			min_th > UINT16_MAX ||
>  			max_th > UINT16_MAX)
> --
> 1.8.3.1

Do we need changes in test-pmd as well?

Regards,
Cristian

Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue weight

[Rao, Nikhil]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, December 10, 2018 9:31 PM

Hi Stephen,

> To: Rao, Nikhil <nikhil.rao@intel.com>
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: support double precision RED queue
> weight
> 
> On Mon, 10 Dec 2018 05:43:37 +0000
> "Rao, Nikhil" <nikhil.rao@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Thursday, November 29, 2018 11:43 AM
> > > To: Rao, Nikhil <nikhil.rao@intel.com>
> > > Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh,
> > > Jasvinder <jasvinder.singh@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] ethdev: support double precision RED
> > > queue weight
> > >
> > > On Thu, 29 Nov 2018 11:24:42 +0530
> > > Nikhil Rao <nikhil.rao@intel.com> wrote:
> > >
> > > > RED queue weight is currently specified as a negated log of 2.
> > > >
> > > > Add support for RED queue weight to be specified in double
> > > > precision and TM capability flags for double precision and negated
> > > > log2 RED queue weight support.
> > > >
> > > > Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> > >
> > > Since this is an ABI break anyway, why not just commit to the new format?
> >
> > Hi Stephen,
> >
> > Can you please provide more detail on your comment ?  are you suggesting
> replacing the wq_log2/wq_dp with a double ?
> >
> > Thanks,
> > Nikhil
> 
> 
> My comment is that since you are changing a structure layout, which would
> break existing users; why not go farther and just fix the API to a better
> version. I don't think any projects use this code anyway, see my talk
> (https://github.com/shemminger/dpdk-metrics).

Sorry for the delay.  I don't get your reference to a better version,  the structure supports both formats using a union,
 similar to how it's done in other DPDK APIs (struct rte_crypto_sym_xform for example).  If you have a suggestion for a better layout,
please let me know.
 
> 
> Isn't floating point going to be expensive. Or is it only during the setup
> process, not enqueue/dequeue.

Yes, floating point may end up to be more expensive (and the RED computation is invoked at every enqueue)

Nikhil

[dpdk-dev] [PATCH v2] ethdev: support double precision RED queue weight

[Nikhil Rao]

RED queue weight is currently specified as a negated log of 2.

Add support for RED queue weight to be specified in double precision
and TM capability flags for double precision and negated log2
RED queue weight support.

Update the softnic PMD and testpmd for the new tm capability flags and
the struct rte_tm_red_params::wq_is_log2 flag.

Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
---
 lib/librte_ethdev/rte_tm.h               | 40 ++++++++++++++++++++++++++++++--
 app/test-pmd/cmdline_tm.c                |  8 ++++++-
 drivers/net/softnic/rte_eth_softnic_tm.c |  7 ++++--
 3 files changed, 50 insertions(+), 5 deletions(-)

v2:
* added RTE_STD_C11 for anonymnous union
* testpmd: added cman_wred_wq_log2/wq_dp_supported to "show port tm cap" cli command
* testpmd: set wq_is_log2 for WRED profiles added on testpmd cli

Cristian, 
I assume the testpmd changes will be upstreamed via the dpdk-next-qos tree.

diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
index c4a788b..4fad31b 100644
--- a/lib/librte_ethdev/rte_tm.h
+++ b/lib/librte_ethdev/rte_tm.h
@@ -391,6 +391,22 @@ struct rte_tm_capabilities {
 	 */
 	int cman_wred_byte_mode_supported;
 
+	/** Double precision RED queue weight support. When non-zero, this
+	 * this parameter indicates that RED queue weight in double precision
+	 * format is supported.
+	 * @see struct rte_tm_red_params::wq_is_log2
+	 * @see struct rte_tm_red_params::wq_dp
+	 */
+	int cman_wred_wq_dp_supported;
+
+	/** Negated log2 RED queue weight support. When non-zero, this
+	 * parameter indicates that RED queue weight in negated log2 format
+	 * is supported.
+	 * @see struct rte_tm_red_params::wq_is_log2
+	 * @see struct rte_tm_red_params::wq_log2
+	 */
+	int cman_wred_wq_log2_supported;
+
 	/** Head drop algorithm support. When non-zero, this parameter
 	 * indicates that there is at least one leaf node that supports the head
 	 * drop algorithm, which might not be true for all the leaf nodes.
@@ -839,8 +855,28 @@ struct rte_tm_red_params {
 	 */
 	uint16_t maxp_inv;
 
-	/** Negated log2 of queue weight (wq), i.e. wq = 1 / (2 ^ wq_log2) */
-	uint16_t wq_log2;
+	/** When non-zero, RED queue weight (wq) is negated log2 of queue
+	 * weight
+	 *
+	 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
+	 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
+	 */
+	int wq_is_log2;
+
+	RTE_STD_C11
+	union {
+		/** Double precision queue weight
+		 *
+		 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
+		 */
+		double wq_dp;
+		/** Negated log2 of queue weight (wq),
+		 * i.e. wq = 1 / (2 ^ wq_log2)
+		 *
+		 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
+		 */
+		uint16_t wq_log2;
+	};
 };
 
 /**
diff --git a/app/test-pmd/cmdline_tm.c b/app/test-pmd/cmdline_tm.c
index 1012084..afc1d63 100644
--- a/app/test-pmd/cmdline_tm.c
+++ b/app/test-pmd/cmdline_tm.c
@@ -283,6 +283,10 @@ static void cmd_show_port_tm_cap_parsed(void *parsed_result,
 		cap.sched_wfq_n_groups_max);
 	printf("cap.sched_wfq_weight_max %" PRIu32 "\n",
 		cap.sched_wfq_weight_max);
+	printf("cap.cman_wred_wq_dp_supported %" PRId32 "\n",
+		cap.cman_wred_wq_dp_supported);
+	printf("cap.cman_wred_wq_log2_supported %" PRId32 "\n",
+		cap.cman_wred_wq_log2_supported);
 	printf("cap.cman_head_drop_supported %" PRId32 "\n",
 		cap.cman_head_drop_supported);
 	printf("cap.cman_wred_context_n_max %" PRIu32 "\n",
@@ -1285,7 +1289,7 @@ static void cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
 	wp.red_params[color].max_th = res->max_th_g;
 	wp.red_params[color].maxp_inv = res->maxp_inv_g;
 	wp.red_params[color].wq_log2 = res->wq_log2_g;
-
+	wp.red_params[color].wq_is_log2 = 1;
 
 	/* WRED Params  (Yellow Color)*/
 	color = RTE_TM_YELLOW;
@@ -1293,6 +1297,7 @@ static void cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
 	wp.red_params[color].max_th = res->max_th_y;
 	wp.red_params[color].maxp_inv = res->maxp_inv_y;
 	wp.red_params[color].wq_log2 = res->wq_log2_y;
+	wp.red_params[color].wq_is_log2 = 1;
 
 	/* WRED Params  (Red Color)*/
 	color = RTE_TM_RED;
@@ -1300,6 +1305,7 @@ static void cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
 	wp.red_params[color].max_th = res->max_th_r;
 	wp.red_params[color].maxp_inv = res->maxp_inv_r;
 	wp.red_params[color].wq_log2 = res->wq_log2_r;
+	wp.red_params[color].wq_is_log2 = 1;
 
 	ret = rte_tm_wred_profile_add(port_id, wred_profile_id, &wp, &error);
 	if (ret != 0) {
diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c b/drivers/net/softnic/rte_eth_softnic_tm.c
index baaafbe..e96ea8e 100644
--- a/drivers/net/softnic/rte_eth_softnic_tm.c
+++ b/drivers/net/softnic/rte_eth_softnic_tm.c
@@ -469,7 +469,8 @@ struct softnic_tmgr_port *
 	.cman_wred_context_shared_n_max = 0,
 	.cman_wred_context_shared_n_nodes_per_context_max = 0,
 	.cman_wred_context_shared_n_contexts_per_node_max = 0,
-
+	.cman_wred_wq_dp_supported = 0,
+	.cman_wred_wq_log2_supported = WRED_SUPPORTED,
 	.mark_vlan_dei_supported = {0, 0, 0},
 	.mark_ip_ecn_tcp_supported = {0, 0, 0},
 	.mark_ip_ecn_sctp_supported = {0, 0, 0},
@@ -1243,8 +1244,10 @@ struct softnic_tmgr_port *
 	for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
 		uint32_t min_th = profile->red_params[color].min_th;
 		uint32_t max_th = profile->red_params[color].max_th;
+		int wq_is_log2 = profile->red_params[color].wq_is_log2;
 
-		if (min_th > max_th ||
+		if (wq_is_log2 == 0 ||
+			min_th > max_th ||
 			max_th == 0 ||
 			min_th > UINT16_MAX ||
 			max_th > UINT16_MAX)
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] ethdev: support double precision RED queue weight

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Rao, Nikhil
> Sent: Thursday, January 10, 2019 4:36 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org; Rao, Nikhil
> <nikhil.rao@intel.com>
> Subject: [PATCH v2] ethdev: support double precision RED queue weight
> 
> RED queue weight is currently specified as a negated log of 2.
> 
> Add support for RED queue weight to be specified in double precision
> and TM capability flags for double precision and negated log2
> RED queue weight support.
> 
> Update the softnic PMD and testpmd for the new tm capability flags and
> the struct rte_tm_red_params::wq_is_log2 flag.
> 
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> ---
>  lib/librte_ethdev/rte_tm.h               | 40
> ++++++++++++++++++++++++++++++--
>  app/test-pmd/cmdline_tm.c                |  8 ++++++-
>  drivers/net/softnic/rte_eth_softnic_tm.c |  7 ++++--
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> v2:
> * added RTE_STD_C11 for anonymnous union
> * testpmd: added cman_wred_wq_log2/wq_dp_supported to "show port
> tm cap" cli command
> * testpmd: set wq_is_log2 for WRED profiles added on testpmd cli
> 
> Cristian,
> I assume the testpmd changes will be upstreamed via the dpdk-next-qos
> tree.
> 
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index c4a788b..4fad31b 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -391,6 +391,22 @@ struct rte_tm_capabilities {
>  	 */
>  	int cman_wred_byte_mode_supported;
> 
> +	/** Double precision RED queue weight support. When non-zero,
> this
> +	 * this parameter indicates that RED queue weight in double precision
> +	 * format is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_dp
> +	 */
> +	int cman_wred_wq_dp_supported;
> +
> +	/** Negated log2 RED queue weight support. When non-zero, this
> +	 * parameter indicates that RED queue weight in negated log2 format
> +	 * is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_log2
> +	 */
> +	int cman_wred_wq_log2_supported;
> +
>  	/** Head drop algorithm support. When non-zero, this parameter
>  	 * indicates that there is at least one leaf node that supports the
> head
>  	 * drop algorithm, which might not be true for all the leaf nodes.
> @@ -839,8 +855,28 @@ struct rte_tm_red_params {
>  	 */
>  	uint16_t maxp_inv;
> 
> -	/** Negated log2 of queue weight (wq), i.e. wq = 1 / (2 ^ wq_log2)
> */
> -	uint16_t wq_log2;
> +	/** When non-zero, RED queue weight (wq) is negated log2 of
> queue
> +	 * weight
> +	 *
> +	 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
> +	 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
> +	 */
> +	int wq_is_log2;
> +
> +	RTE_STD_C11
> +	union {
> +		/** Double precision queue weight
> +		 *
> +		 * @see struct
> rte_tm_capabilities::cman_wred_wq_dp_supported
> +		 */
> +		double wq_dp;
> +		/** Negated log2 of queue weight (wq),
> +		 * i.e. wq = 1 / (2 ^ wq_log2)
> +		 *
> +		 * @see struct
> rte_tm_capabilities::cman_wred_wq_log2_supported
> +		 */
> +		uint16_t wq_log2;
> +	};
>  };
> 
>  /**
> diff --git a/app/test-pmd/cmdline_tm.c b/app/test-pmd/cmdline_tm.c
> index 1012084..afc1d63 100644
> --- a/app/test-pmd/cmdline_tm.c
> +++ b/app/test-pmd/cmdline_tm.c
> @@ -283,6 +283,10 @@ static void cmd_show_port_tm_cap_parsed(void
> *parsed_result,
>  		cap.sched_wfq_n_groups_max);
>  	printf("cap.sched_wfq_weight_max %" PRIu32 "\n",
>  		cap.sched_wfq_weight_max);
> +	printf("cap.cman_wred_wq_dp_supported %" PRId32 "\n",
> +		cap.cman_wred_wq_dp_supported);
> +	printf("cap.cman_wred_wq_log2_supported %" PRId32 "\n",
> +		cap.cman_wred_wq_log2_supported);
>  	printf("cap.cman_head_drop_supported %" PRId32 "\n",
>  		cap.cman_head_drop_supported);
>  	printf("cap.cman_wred_context_n_max %" PRIu32 "\n",
> @@ -1285,7 +1289,7 @@ static void
> cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
>  	wp.red_params[color].max_th = res->max_th_g;
>  	wp.red_params[color].maxp_inv = res->maxp_inv_g;
>  	wp.red_params[color].wq_log2 = res->wq_log2_g;
> -
> +	wp.red_params[color].wq_is_log2 = 1;
> 
>  	/* WRED Params  (Yellow Color)*/
>  	color = RTE_TM_YELLOW;
> @@ -1293,6 +1297,7 @@ static void
> cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
>  	wp.red_params[color].max_th = res->max_th_y;
>  	wp.red_params[color].maxp_inv = res->maxp_inv_y;
>  	wp.red_params[color].wq_log2 = res->wq_log2_y;
> +	wp.red_params[color].wq_is_log2 = 1;
> 
>  	/* WRED Params  (Red Color)*/
>  	color = RTE_TM_RED;
> @@ -1300,6 +1305,7 @@ static void
> cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
>  	wp.red_params[color].max_th = res->max_th_r;
>  	wp.red_params[color].maxp_inv = res->maxp_inv_r;
>  	wp.red_params[color].wq_log2 = res->wq_log2_r;
> +	wp.red_params[color].wq_is_log2 = 1;
> 
>  	ret = rte_tm_wred_profile_add(port_id, wred_profile_id, &wp,
> &error);
>  	if (ret != 0) {
> diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c
> b/drivers/net/softnic/rte_eth_softnic_tm.c
> index baaafbe..e96ea8e 100644
> --- a/drivers/net/softnic/rte_eth_softnic_tm.c
> +++ b/drivers/net/softnic/rte_eth_softnic_tm.c
> @@ -469,7 +469,8 @@ struct softnic_tmgr_port *
>  	.cman_wred_context_shared_n_max = 0,
>  	.cman_wred_context_shared_n_nodes_per_context_max = 0,
>  	.cman_wred_context_shared_n_contexts_per_node_max = 0,
> -
> +	.cman_wred_wq_dp_supported = 0,
> +	.cman_wred_wq_log2_supported = WRED_SUPPORTED,
>  	.mark_vlan_dei_supported = {0, 0, 0},
>  	.mark_ip_ecn_tcp_supported = {0, 0, 0},
>  	.mark_ip_ecn_sctp_supported = {0, 0, 0},
> @@ -1243,8 +1244,10 @@ struct softnic_tmgr_port *
>  	for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
>  		uint32_t min_th = profile->red_params[color].min_th;
>  		uint32_t max_th = profile->red_params[color].max_th;
> +		int wq_is_log2 = profile->red_params[color].wq_is_log2;
> 
> -		if (min_th > max_th ||
> +		if (wq_is_log2 == 0 ||
> +			min_th > max_th ||
>  			max_th == 0 ||
>  			min_th > UINT16_MAX ||
>  			max_th > UINT16_MAX)
> --
> 1.8.3.1


Hi Nikhil,

Code looks good to me, but there is a problem: you are introducing new API without providing an implementation for it, and this is no longer allowed by the DPDK Tech Board [1][2].

Would it be possible to provide an implementation for this new API? A SW implementation in librte_sched/rte_red.[hc] or divers/net/softnic would be enough to tick this box.

Thanks,
Cristian

[1] https://mails.dpdk.org/archives/dev/2018-December/122213.html
[2] https://mails.dpdk.org/archives/dev/2018-November/118697.html

Re: [dpdk-dev] [PATCH v2] ethdev: support double precision RED queue weight

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Rao, Nikhil
> Sent: Thursday, January 10, 2019 4:36 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org; Rao, Nikhil
> <nikhil.rao@intel.com>
> Subject: [PATCH v2] ethdev: support double precision RED queue weight
> 
> RED queue weight is currently specified as a negated log of 2.
> 
> Add support for RED queue weight to be specified in double precision
> and TM capability flags for double precision and negated log2
> RED queue weight support.
> 
> Update the softnic PMD and testpmd for the new tm capability flags and
> the struct rte_tm_red_params::wq_is_log2 flag.
> 
> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> ---
>  lib/librte_ethdev/rte_tm.h               | 40
> ++++++++++++++++++++++++++++++--
>  app/test-pmd/cmdline_tm.c                |  8 ++++++-
>  drivers/net/softnic/rte_eth_softnic_tm.c |  7 ++++--
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> v2:
> * added RTE_STD_C11 for anonymnous union
> * testpmd: added cman_wred_wq_log2/wq_dp_supported to "show port
> tm cap" cli command
> * testpmd: set wq_is_log2 for WRED profiles added on testpmd cli
> 
> Cristian,
> I assume the testpmd changes will be upstreamed via the dpdk-next-qos
> tree.
> 
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index c4a788b..4fad31b 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -391,6 +391,22 @@ struct rte_tm_capabilities {
>  	 */
>  	int cman_wred_byte_mode_supported;
> 
> +	/** Double precision RED queue weight support. When non-zero,
> this
> +	 * this parameter indicates that RED queue weight in double precision
> +	 * format is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_dp
> +	 */
> +	int cman_wred_wq_dp_supported;
> +
> +	/** Negated log2 RED queue weight support. When non-zero, this
> +	 * parameter indicates that RED queue weight in negated log2 format
> +	 * is supported.
> +	 * @see struct rte_tm_red_params::wq_is_log2
> +	 * @see struct rte_tm_red_params::wq_log2
> +	 */
> +	int cman_wred_wq_log2_supported;
> +
>  	/** Head drop algorithm support. When non-zero, this parameter
>  	 * indicates that there is at least one leaf node that supports the
> head
>  	 * drop algorithm, which might not be true for all the leaf nodes.
> @@ -839,8 +855,28 @@ struct rte_tm_red_params {
>  	 */
>  	uint16_t maxp_inv;
> 
> -	/** Negated log2 of queue weight (wq), i.e. wq = 1 / (2 ^ wq_log2)
> */
> -	uint16_t wq_log2;
> +	/** When non-zero, RED queue weight (wq) is negated log2 of
> queue
> +	 * weight
> +	 *
> +	 * @see struct rte_tm_capabilities::cman_wred_wq_dp_supported
> +	 * @see struct rte_tm_capabilities::cman_wred_wq_log2_supported
> +	 */
> +	int wq_is_log2;
> +
> +	RTE_STD_C11
> +	union {
> +		/** Double precision queue weight
> +		 *
> +		 * @see struct
> rte_tm_capabilities::cman_wred_wq_dp_supported
> +		 */
> +		double wq_dp;
> +		/** Negated log2 of queue weight (wq),
> +		 * i.e. wq = 1 / (2 ^ wq_log2)
> +		 *
> +		 * @see struct
> rte_tm_capabilities::cman_wred_wq_log2_supported
> +		 */
> +		uint16_t wq_log2;
> +	};
>  };
> 
>  /**
> diff --git a/app/test-pmd/cmdline_tm.c b/app/test-pmd/cmdline_tm.c
> index 1012084..afc1d63 100644
> --- a/app/test-pmd/cmdline_tm.c
> +++ b/app/test-pmd/cmdline_tm.c
> @@ -283,6 +283,10 @@ static void cmd_show_port_tm_cap_parsed(void
> *parsed_result,
>  		cap.sched_wfq_n_groups_max);
>  	printf("cap.sched_wfq_weight_max %" PRIu32 "\n",
>  		cap.sched_wfq_weight_max);
> +	printf("cap.cman_wred_wq_dp_supported %" PRId32 "\n",
> +		cap.cman_wred_wq_dp_supported);
> +	printf("cap.cman_wred_wq_log2_supported %" PRId32 "\n",
> +		cap.cman_wred_wq_log2_supported);
>  	printf("cap.cman_head_drop_supported %" PRId32 "\n",
>  		cap.cman_head_drop_supported);
>  	printf("cap.cman_wred_context_n_max %" PRIu32 "\n",
> @@ -1285,7 +1289,7 @@ static void
> cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
>  	wp.red_params[color].max_th = res->max_th_g;
>  	wp.red_params[color].maxp_inv = res->maxp_inv_g;
>  	wp.red_params[color].wq_log2 = res->wq_log2_g;
> -
> +	wp.red_params[color].wq_is_log2 = 1;
> 
>  	/* WRED Params  (Yellow Color)*/
>  	color = RTE_TM_YELLOW;
> @@ -1293,6 +1297,7 @@ static void
> cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
>  	wp.red_params[color].max_th = res->max_th_y;
>  	wp.red_params[color].maxp_inv = res->maxp_inv_y;
>  	wp.red_params[color].wq_log2 = res->wq_log2_y;
> +	wp.red_params[color].wq_is_log2 = 1;
> 
>  	/* WRED Params  (Red Color)*/
>  	color = RTE_TM_RED;
> @@ -1300,6 +1305,7 @@ static void
> cmd_add_port_tm_node_wred_profile_parsed(void *parsed_result,
>  	wp.red_params[color].max_th = res->max_th_r;
>  	wp.red_params[color].maxp_inv = res->maxp_inv_r;
>  	wp.red_params[color].wq_log2 = res->wq_log2_r;
> +	wp.red_params[color].wq_is_log2 = 1;
> 
>  	ret = rte_tm_wred_profile_add(port_id, wred_profile_id, &wp,
> &error);
>  	if (ret != 0) {
> diff --git a/drivers/net/softnic/rte_eth_softnic_tm.c
> b/drivers/net/softnic/rte_eth_softnic_tm.c
> index baaafbe..e96ea8e 100644
> --- a/drivers/net/softnic/rte_eth_softnic_tm.c
> +++ b/drivers/net/softnic/rte_eth_softnic_tm.c
> @@ -469,7 +469,8 @@ struct softnic_tmgr_port *
>  	.cman_wred_context_shared_n_max = 0,
>  	.cman_wred_context_shared_n_nodes_per_context_max = 0,
>  	.cman_wred_context_shared_n_contexts_per_node_max = 0,
> -
> +	.cman_wred_wq_dp_supported = 0,
> +	.cman_wred_wq_log2_supported = WRED_SUPPORTED,
>  	.mark_vlan_dei_supported = {0, 0, 0},
>  	.mark_ip_ecn_tcp_supported = {0, 0, 0},
>  	.mark_ip_ecn_sctp_supported = {0, 0, 0},
> @@ -1243,8 +1244,10 @@ struct softnic_tmgr_port *
>  	for (color = RTE_TM_GREEN; color < RTE_TM_COLORS; color++) {
>  		uint32_t min_th = profile->red_params[color].min_th;
>  		uint32_t max_th = profile->red_params[color].max_th;
> +		int wq_is_log2 = profile->red_params[color].wq_is_log2;
> 
> -		if (min_th > max_th ||
> +		if (wq_is_log2 == 0 ||
> +			min_th > max_th ||
>  			max_th == 0 ||
>  			min_th > UINT16_MAX ||
>  			max_th > UINT16_MAX)
> --
> 1.8.3.1


Hi Nikhil,

Code looks good to me, but there is a problem: you are introducing new API without providing an implementation for it, and this is no longer allowed by the DPDK Tech Board [1][2].

Would it be possible to provide an implementation for this new API? A SW implementation in librte_sched/rte_red.[hc] or divers/net/softnic would be enough to tick this box.

Thanks,
Cristian

[1] https://mails.dpdk.org/archives/dev/2018-December/122213.html
[2] https://mails.dpdk.org/archives/dev/2018-November/118697.html