[1/2] ethdev: add tm cap for private shaper packet mode

Message ID 20200330160019.29674-1-ndabilpuram@marvell.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [1/2] ethdev: add tm cap for private shaper packet mode |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS

Commit Message

Nithin Dabilpuram March 30, 2020, 4 p.m. UTC
  Some NIC hardware have private shaper attached to
every node and has a limitation where packet mode is applied
both to the scheduling of a node's children using WFQ and
shaping of traffic out of the private shaper.
This cannot be expressed using existing capabilities or configurations.

So this patch adds a tm capability that if set by a PMD implies that
packet mode when configured is even applied to private shaper
connected to that node. This also implies the limitation
that all the SP children of that node should have same mode
at any point of time i.e either packet mode or byte mode and
same applies to private shaper in that NIC PMD.

This patch also adds missing capability that tells whether PMD
supports wfq weight mode or not.

Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---
 lib/librte_ethdev/rte_tm.h | 62 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)
  

Comments

Nithin Dabilpuram April 7, 2020, 7:30 a.m. UTC | #1
Ping.

On Mon, Mar 30, 2020 at 09:30:18PM +0530, Nithin Dabilpuram wrote:
> Some NIC hardware have private shaper attached to
> every node and has a limitation where packet mode is applied
> both to the scheduling of a node's children using WFQ and
> shaping of traffic out of the private shaper.
> This cannot be expressed using existing capabilities or configurations.
> 
> So this patch adds a tm capability that if set by a PMD implies that
> packet mode when configured is even applied to private shaper
> connected to that node. This also implies the limitation
> that all the SP children of that node should have same mode
> at any point of time i.e either packet mode or byte mode and
> same applies to private shaper in that NIC PMD.
> 
> This patch also adds missing capability that tells whether PMD
> supports wfq weight mode or not.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
>  lib/librte_ethdev/rte_tm.h | 62 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index f9c0cf3..50bcea6 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -339,6 +339,20 @@ struct rte_tm_capabilities {
>  	 */
>  	uint32_t sched_wfq_weight_max;
>  
> +	/** WFQ weight mode supported. Non-zero value indicates wfq weight mode
> +	 * is supported and a SP child (even a wfq group) can be configured to
> +	 * use packet-mode or byte-mode for weight calculations.
> +	 */
> +	int sched_wfq_weight_mode_supported;
> +
> +	/** Private shaper and scheduler weight mode.
> +	 * When non-zero value indicates that all SP children should have
> +	 * same weight mode and the same mode applies to private
> +	 * shaper as well. This is only valid if
> +	 * *sched_wfq_weight_mode_supported* is set.
> +	 */
> +	int sched_shaper_private_weight_mode;
> +
>  	/** WRED packet mode support. When non-zero, this parameter indicates
>  	 * that there is at least one leaf node that supports the WRED packet
>  	 * mode, which might not be true for all the leaf nodes. In packet
> @@ -554,6 +568,21 @@ struct rte_tm_level_capabilities {
>  			 */
>  			uint32_t sched_wfq_weight_max;
>  
> +			/** WFQ weight mode supported. Non-zero value indicates
> +			 * wfq weight mode is supported and a SP child
> +			 * (even a wfq group) can be configured to use
> +			 * packet-mode or byte-mode for weight calculations.
> +			 */
> +			int sched_wfq_weight_mode_supported;
> +
> +			/** Private shaper and scheduler weight mode.
> +			 * When non-zero value indicates that all SP children
> +			 * should have same weight mode and the same mode
> +			 * applies to private shaper as well. This is only
> +			 * valid if *sched_wfq_weight_mode_supported* is set.
> +			 */
> +			int sched_shaper_private_weight_mode;
> +
>  			/** Mask of statistics counter types supported by the
>  			 * non-leaf nodes on this level. Every supported
>  			 * statistics counter type is supported by at least one
> @@ -735,6 +764,21 @@ struct rte_tm_node_capabilities {
>  			 * WFQ weight, so WFQ is reduced to FQ.
>  			 */
>  			uint32_t sched_wfq_weight_max;
> +
> +			/** WFQ weight mode supported. Non-zero value indicates
> +			 * wfq weight mode is supported and a SP child
> +			 * (even a wfq group) can be configured to use
> +			 * packet-mode or byte-mode for weight calculations.
> +			 */
> +			int sched_wfq_weight_mode_supported;
> +
> +			/** Private shaper and scheduler weight mode.
> +			 * When non-zero value indicates that all SP children
> +			 * should have same weight mode and the same mode
> +			 * applies to private shaper as well. This is only
> +			 * valid if *sched_wfq_weight_mode_supported* is set.
> +			 */
> +			int sched_shaper_private_weight_mode;
>  		} nonleaf;
>  
>  		/** Items valid only for leaf nodes. */
> @@ -836,10 +880,19 @@ struct rte_tm_wred_params {
>   * Token bucket
>   */
>  struct rte_tm_token_bucket {
> -	/** Token bucket rate (bytes per second) */
> +	/** Token bucket rate. This is in "bytes per second" by default.
> +	 * For private shaper attached to node that is set in packet mode
> +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> +	 * this is interpreted as "packets per second".
> +	 */
>  	uint64_t rate;
>  
> -	/** Token bucket size (bytes), a.k.a. max burst size */
> +	/** Token bucket size, a.k.a. max burst size.
> +	 * This is in "bytes" by default.
> +	 * For private shaper attached to node that is set in packet mode
> +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> +	 * this is interpreted as "packets".
> +	 */
>  	uint64_t size;
>  };
>  
> @@ -924,7 +977,10 @@ struct rte_tm_node_params {
>  			 * indicates that WFQ is to be used for all priorities.
>  			 * When non-NULL, it points to a pre-allocated array of
>  			 * *n_sp_priorities* values, with non-zero value for
> -			 * byte-mode and zero for packet-mode.
> +			 * byte-mode and zero for packet-mode. The same mode is
> +			 * used for private shaper connected to this node if
> +			 * tm capability *sched_shaper_private_weight_mode* is
> +			 * true.
>  			 */
>  			int *wfq_weight_mode;
>  
> -- 
> 2.8.4
>
  
Cristian Dumitrescu April 7, 2020, 4:31 p.m. UTC | #2
Hi Nithin,

> -----Original Message-----
> From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> Sent: Monday, March 30, 2020 5:00 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; kkanas@marvell.com; Nithin
> Dabilpuram <ndabilpuram@marvell.com>
> Subject: [PATCH 1/2] ethdev: add tm cap for private shaper packet mode
> 
> Some NIC hardware have private shaper attached to
> every node and has a limitation where packet mode is applied
> both to the scheduling of a node's children using WFQ and
> shaping of traffic out of the private shaper.
> This cannot be expressed using existing capabilities or configurations.
> 
> So this patch adds a tm capability that if set by a PMD implies that
> packet mode when configured is even applied to private shaper
> connected to that node. This also implies the limitation
> that all the SP children of that node should have same mode
> at any point of time i.e either packet mode or byte mode and
> same applies to private shaper in that NIC PMD.
> 
> This patch also adds missing capability that tells whether PMD
> supports wfq weight mode or not.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
>  lib/librte_ethdev/rte_tm.h | 62
> +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index f9c0cf3..50bcea6 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -339,6 +339,20 @@ struct rte_tm_capabilities {
>  	 */
>  	uint32_t sched_wfq_weight_max;
> 
> +	/** WFQ weight mode supported. Non-zero value indicates wfq
> weight mode
> +	 * is supported and a SP child (even a wfq group) can be configured
> to
> +	 * use packet-mode or byte-mode for weight calculations.
> +	 */
> +	int sched_wfq_weight_mode_supported;
> +

This is incorrect, as the WFQ support, including the weight aspect of WFQ, is already part of the existing set of capabilities: see sched_wfq_weight_max and the other sched_wfq_* capability fields.

> +	/** Private shaper and scheduler weight mode.
> +	 * When non-zero value indicates that all SP children should have
> +	 * same weight mode and the same mode applies to private
> +	 * shaper as well. This is only valid if
> +	 * *sched_wfq_weight_mode_supported* is set.
> +	 */
> +	int sched_shaper_private_weight_mode;
> +

If I understand your intention correctly, you are trying to introduce packet mode (in addition to the existing byte mode) for (1) scheduler WFQ weights and for (2) shaper rates. Basically, the ability to express WFQ weights in bytes as well as packets, and the ability to express shaper rates in bytes and well as packets. Is this correct?

Assuming yes, we probably need to do it in a slightly different way:
1/ Similar to the WRED packet mode that was introduced by Nikhil Rao's patches a while ago (in addition to WRED's byte mode), see WRED capability and configuration.
2/ Decouple between scheduler and shaper. So we should add sched_* fields and shaper_* fields, but never sched_shaper_*, as it creates a functional dependency that does not exist.

In line with methodology already used for WRED, I suggest:
a) Scheduler WFQ capabilities (TM/level/node): sched_wfq_packet_mode_supported, sched_wfq_byte_mode_supported
b) Shaper capabilities (TM/level/node): shaper_rate_packet_mode_supported, shaper_rate_byte_mode_supported.
c) Shaper profile (struct rte_tm_shaper_params): add an integer packet_mode flag with 0 = byte-mode (default) and 1 = packet mode for the values in struct rte_tm_token_bucket.

It is important to note that the API must allow a combination of packet mode and byte mode (for different nodes, not for the same), but an implementation can support either a single mode or both (should be enforced by the driver).

>  	/** WRED packet mode support. When non-zero, this parameter
> indicates
>  	 * that there is at least one leaf node that supports the WRED packet
>  	 * mode, which might not be true for all the leaf nodes. In packet
> @@ -554,6 +568,21 @@ struct rte_tm_level_capabilities {
>  			 */
>  			uint32_t sched_wfq_weight_max;
> 
> +			/** WFQ weight mode supported. Non-zero value
> indicates
> +			 * wfq weight mode is supported and a SP child
> +			 * (even a wfq group) can be configured to use
> +			 * packet-mode or byte-mode for weight
> calculations.
> +			 */
> +			int sched_wfq_weight_mode_supported;
> +
> +			/** Private shaper and scheduler weight mode.
> +			 * When non-zero value indicates that all SP children
> +			 * should have same weight mode and the same
> mode
> +			 * applies to private shaper as well. This is only
> +			 * valid if *sched_wfq_weight_mode_supported* is
> set.
> +			 */
> +			int sched_shaper_private_weight_mode;
> +
>  			/** Mask of statistics counter types supported by the
>  			 * non-leaf nodes on this level. Every supported
>  			 * statistics counter type is supported by at least one

See above the comments on TM capabilities.

> @@ -735,6 +764,21 @@ struct rte_tm_node_capabilities {
>  			 * WFQ weight, so WFQ is reduced to FQ.
>  			 */
>  			uint32_t sched_wfq_weight_max;
> +
> +			/** WFQ weight mode supported. Non-zero value
> indicates
> +			 * wfq weight mode is supported and a SP child
> +			 * (even a wfq group) can be configured to use
> +			 * packet-mode or byte-mode for weight
> calculations.
> +			 */
> +			int sched_wfq_weight_mode_supported;
> +
> +			/** Private shaper and scheduler weight mode.
> +			 * When non-zero value indicates that all SP children
> +			 * should have same weight mode and the same
> mode
> +			 * applies to private shaper as well. This is only
> +			 * valid if *sched_wfq_weight_mode_supported* is
> set.
> +			 */
> +			int sched_shaper_private_weight_mode;
>  		} nonleaf;
> 
>  		/** Items valid only for leaf nodes. */

See above the comments on the TM capabilities.

> @@ -836,10 +880,19 @@ struct rte_tm_wred_params {
>   * Token bucket
>   */
>  struct rte_tm_token_bucket {
> -	/** Token bucket rate (bytes per second) */
> +	/** Token bucket rate. This is in "bytes per second" by default.
> +	 * For private shaper attached to node that is set in packet mode
> +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> +	 * this is interpreted as "packets per second".
> +	 */
>  	uint64_t rate;
> 
> -	/** Token bucket size (bytes), a.k.a. max burst size */
> +	/** Token bucket size, a.k.a. max burst size.
> +	 * This is in "bytes" by default.
> +	 * For private shaper attached to node that is set in packet mode
> +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> +	 * this is interpreted as "packets".
> +	 */
>  	uint64_t size;
>  };
> 

Comments are not correct, as API should allow a combination of both the packet mode and the byte mode (for different nodes, not for the same node), so both capabilities shaper_rate_packet_mode and shaper_rate_byte_mode can be set. Hence, the comments should not specify a capability, but the fact that these values can specify either byte or packets, depending on a flag elsewhere.

> @@ -924,7 +977,10 @@ struct rte_tm_node_params {
>  			 * indicates that WFQ is to be used for all priorities.
>  			 * When non-NULL, it points to a pre-allocated array
> of
>  			 * *n_sp_priorities* values, with non-zero value for
> -			 * byte-mode and zero for packet-mode.
> +			 * byte-mode and zero for packet-mode. The same
> mode is
> +			 * used for private shaper connected to this node if
> +			 * tm capability
> *sched_shaper_private_weight_mode* is
> +			 * true.
>  			 */

This comment is incorrect, as sched should not be combined with shaper. The user should select between packet mode and byte mode for the WFQ weight independently of the mode for the shaper rate, although an implementation (driver) should enforce the correct values.

>  			int *wfq_weight_mode;
> 
> --
> 2.8.4


Regards,
Cristian
  
Nithin Dabilpuram April 7, 2020, 5:21 p.m. UTC | #3
Hi Cristian,

Thanks for your comments. I have some queries below.
On Tue, Apr 07, 2020 at 04:31:47PM +0000, Dumitrescu, Cristian wrote:
> External Email
> 
> ----------------------------------------------------------------------
> Hi Nithin,
> 
> > -----Original Message-----
> > From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > Sent: Monday, March 30, 2020 5:00 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> > Rybchenko <arybchenko@solarflare.com>
> > Cc: dev@dpdk.org; jerinj@marvell.com; kkanas@marvell.com; Nithin
> > Dabilpuram <ndabilpuram@marvell.com>
> > Subject: [PATCH 1/2] ethdev: add tm cap for private shaper packet mode
> > 
> > Some NIC hardware have private shaper attached to
> > every node and has a limitation where packet mode is applied
> > both to the scheduling of a node's children using WFQ and
> > shaping of traffic out of the private shaper.
> > This cannot be expressed using existing capabilities or configurations.
> > 
> > So this patch adds a tm capability that if set by a PMD implies that
> > packet mode when configured is even applied to private shaper
> > connected to that node. This also implies the limitation
> > that all the SP children of that node should have same mode
> > at any point of time i.e either packet mode or byte mode and
> > same applies to private shaper in that NIC PMD.
> > 
> > This patch also adds missing capability that tells whether PMD
> > supports wfq weight mode or not.
> > 
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > ---
> >  lib/librte_ethdev/rte_tm.h | 62
> > +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 59 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> > index f9c0cf3..50bcea6 100644
> > --- a/lib/librte_ethdev/rte_tm.h
> > +++ b/lib/librte_ethdev/rte_tm.h
> > @@ -339,6 +339,20 @@ struct rte_tm_capabilities {
> >  	 */
> >  	uint32_t sched_wfq_weight_max;
> > 
> > +	/** WFQ weight mode supported. Non-zero value indicates wfq
> > weight mode
> > +	 * is supported and a SP child (even a wfq group) can be configured
> > to
> > +	 * use packet-mode or byte-mode for weight calculations.
> > +	 */
> > +	int sched_wfq_weight_mode_supported;
> > +
> 
> This is incorrect, as the WFQ support, including the weight aspect of WFQ, is already part of the existing set of capabilities: see sched_wfq_weight_max and the other sched_wfq_* capability fields

This is a missing capability for an existing functionality.
"struct rte_tm_node_params:nonleaf.wfq_weight_mode" field could be used to toggle between
packet-mode or byte-mode for WFQ weights. 

The field if NULL also says that mode defaults to byte-mode.

> 
> > +	/** Private shaper and scheduler weight mode.
> > +	 * When non-zero value indicates that all SP children should have
> > +	 * same weight mode and the same mode applies to private
> > +	 * shaper as well. This is only valid if
> > +	 * *sched_wfq_weight_mode_supported* is set.
> > +	 */
> > +	int sched_shaper_private_weight_mode;
> > +
> 
> If I understand your intention correctly, you are trying to introduce packet mode (in addition to the existing byte mode) for (1) scheduler WFQ weights and for (2) shaper rates. Basically, the ability to express WFQ weights in bytes as well as packets, and the ability to express shaper rates in bytes and well as packets. Is this correct?

Isn't packet mode for (1) already supported via
"struct rte_tm_node_params:nonleaf.wfq_weight_mode" and
rte_tm_node_wfq_weight_mode_update() ?

I'm trying to add support for packet-mode for (2).

> 
> Assuming yes, we probably need to do it in a slightly different way:
> 1/ Similar to the WRED packet mode that was introduced by Nikhil Rao's patches a while ago (in addition to WRED's byte mode), see WRED capability and configuration.
> 2/ Decouple between scheduler and shaper. So we should add sched_* fields and shaper_* fields, but never sched_shaper_*, as it creates a functional dependency that does not exist.
> 
> In line with methodology already used for WRED, I suggest:
> a) Scheduler WFQ capabilities (TM/level/node): sched_wfq_packet_mode_supported, sched_wfq_byte_mode_supported

I'll add "sched_wfq_packet_mode_supported" field.

Since currently when "struct rte_tm_node_params:nonleaf.wfq_weight_mode" is NULL,
mode defaults to byte mode, is it needed to have "sched_wfq_byte_mode_supported" ?
Or I should also update the text in "struct rte_tm_node_params" ?


> b) Shaper capabilities (TM/level/node): shaper_rate_packet_mode_supported, shaper_rate_byte_mode_supported.
Ack.
> c) Shaper profile (struct rte_tm_shaper_params): add an integer packet_mode flag with 0 = byte-mode (default) and 1 = packet mode for the values in struct rte_tm_token_bucket.

Ok. I'll add a field "packet-mode" in rte_tm_shaper_params and enforce restrictions in PMD.
> 
> It is important to note that the API must allow a combination of packet mode and byte mode (for different nodes, not for the same), but an implementation can support either a single mode or both (should be enforced by the driver).
> 
> >  	/** WRED packet mode support. When non-zero, this parameter
> > indicates
> >  	 * that there is at least one leaf node that supports the WRED packet
> >  	 * mode, which might not be true for all the leaf nodes. In packet
> > @@ -554,6 +568,21 @@ struct rte_tm_level_capabilities {
> >  			 */
> >  			uint32_t sched_wfq_weight_max;
> > 
> > +			/** WFQ weight mode supported. Non-zero value
> > indicates
> > +			 * wfq weight mode is supported and a SP child
> > +			 * (even a wfq group) can be configured to use
> > +			 * packet-mode or byte-mode for weight
> > calculations.
> > +			 */
> > +			int sched_wfq_weight_mode_supported;
> > +
> > +			/** Private shaper and scheduler weight mode.
> > +			 * When non-zero value indicates that all SP children
> > +			 * should have same weight mode and the same
> > mode
> > +			 * applies to private shaper as well. This is only
> > +			 * valid if *sched_wfq_weight_mode_supported* is
> > set.
> > +			 */
> > +			int sched_shaper_private_weight_mode;
> > +
> >  			/** Mask of statistics counter types supported by the
> >  			 * non-leaf nodes on this level. Every supported
> >  			 * statistics counter type is supported by at least one
> 
> See above the comments on TM capabilities.
> 
> > @@ -735,6 +764,21 @@ struct rte_tm_node_capabilities {
> >  			 * WFQ weight, so WFQ is reduced to FQ.
> >  			 */
> >  			uint32_t sched_wfq_weight_max;
> > +
> > +			/** WFQ weight mode supported. Non-zero value
> > indicates
> > +			 * wfq weight mode is supported and a SP child
> > +			 * (even a wfq group) can be configured to use
> > +			 * packet-mode or byte-mode for weight
> > calculations.
> > +			 */
> > +			int sched_wfq_weight_mode_supported;
> > +
> > +			/** Private shaper and scheduler weight mode.
> > +			 * When non-zero value indicates that all SP children
> > +			 * should have same weight mode and the same
> > mode
> > +			 * applies to private shaper as well. This is only
> > +			 * valid if *sched_wfq_weight_mode_supported* is
> > set.
> > +			 */
> > +			int sched_shaper_private_weight_mode;
> >  		} nonleaf;
> > 
> >  		/** Items valid only for leaf nodes. */
> 
> See above the comments on the TM capabilities.
> 
> > @@ -836,10 +880,19 @@ struct rte_tm_wred_params {
> >   * Token bucket
> >   */
> >  struct rte_tm_token_bucket {
> > -	/** Token bucket rate (bytes per second) */
> > +	/** Token bucket rate. This is in "bytes per second" by default.
> > +	 * For private shaper attached to node that is set in packet mode
> > +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> > +	 * this is interpreted as "packets per second".
> > +	 */
> >  	uint64_t rate;
> > 
> > -	/** Token bucket size (bytes), a.k.a. max burst size */
> > +	/** Token bucket size, a.k.a. max burst size.
> > +	 * This is in "bytes" by default.
> > +	 * For private shaper attached to node that is set in packet mode
> > +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> > +	 * this is interpreted as "packets".
> > +	 */
> >  	uint64_t size;
> >  };
> > 
> 
> Comments are not correct, as API should allow a combination of both the packet mode and the byte mode (for different nodes, not for the same node), so both capabilities shaper_rate_packet_mode and shaper_rate_byte_mode can be set. Hence, the comments should not specify a capability, but the fact that these values can specify either byte or packets, depending on a flag elsewhere.

Ok. As per your above comment I'll add field "packet-mode" in "struct rte_tm_shaper_params" and update this comment accordingly.
> 
> > @@ -924,7 +977,10 @@ struct rte_tm_node_params {
> >  			 * indicates that WFQ is to be used for all priorities.
> >  			 * When non-NULL, it points to a pre-allocated array
> > of
> >  			 * *n_sp_priorities* values, with non-zero value for
> > -			 * byte-mode and zero for packet-mode.
> > +			 * byte-mode and zero for packet-mode. The same
> > mode is
> > +			 * used for private shaper connected to this node if
> > +			 * tm capability
> > *sched_shaper_private_weight_mode* is
> > +			 * true.
> >  			 */
> 
> This comment is incorrect, as sched should not be combined with shaper. The user should select between packet mode and byte mode for the WFQ weight independently of the mode for the shaper rate, although an implementation (driver) should enforce the correct values.
> 
> >  			int *wfq_weight_mode;
> > 
> > --
> > 2.8.4
> 
> 
> Regards,
> Cristian
  
Cristian Dumitrescu April 10, 2020, 11:45 a.m. UTC | #4
> -----Original Message-----
> From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> Sent: Tuesday, April 7, 2020 6:21 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org; jerinj@marvell.com;
> kkanas@marvell.com
> Subject: Re: [EXT] RE: [PATCH 1/2] ethdev: add tm cap for private shaper
> packet mode
> 
> Hi Cristian,
> 
> Thanks for your comments. I have some queries below.
> On Tue, Apr 07, 2020 at 04:31:47PM +0000, Dumitrescu, Cristian wrote:
> > External Email
> >
> > ----------------------------------------------------------------------
> > Hi Nithin,
> >
> > > -----Original Message-----
> > > From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > Sent: Monday, March 30, 2020 5:00 PM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas
> Monjalon
> > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Andrew
> > > Rybchenko <arybchenko@solarflare.com>
> > > Cc: dev@dpdk.org; jerinj@marvell.com; kkanas@marvell.com; Nithin
> > > Dabilpuram <ndabilpuram@marvell.com>
> > > Subject: [PATCH 1/2] ethdev: add tm cap for private shaper packet mode
> > >
> > > Some NIC hardware have private shaper attached to
> > > every node and has a limitation where packet mode is applied
> > > both to the scheduling of a node's children using WFQ and
> > > shaping of traffic out of the private shaper.
> > > This cannot be expressed using existing capabilities or configurations.
> > >
> > > So this patch adds a tm capability that if set by a PMD implies that
> > > packet mode when configured is even applied to private shaper
> > > connected to that node. This also implies the limitation
> > > that all the SP children of that node should have same mode
> > > at any point of time i.e either packet mode or byte mode and
> > > same applies to private shaper in that NIC PMD.
> > >
> > > This patch also adds missing capability that tells whether PMD
> > > supports wfq weight mode or not.
> > >
> > > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > ---
> > >  lib/librte_ethdev/rte_tm.h | 62
> > > +++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 59 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> > > index f9c0cf3..50bcea6 100644
> > > --- a/lib/librte_ethdev/rte_tm.h
> > > +++ b/lib/librte_ethdev/rte_tm.h
> > > @@ -339,6 +339,20 @@ struct rte_tm_capabilities {
> > >  	 */
> > >  	uint32_t sched_wfq_weight_max;
> > >
> > > +	/** WFQ weight mode supported. Non-zero value indicates wfq
> > > weight mode
> > > +	 * is supported and a SP child (even a wfq group) can be configured
> > > to
> > > +	 * use packet-mode or byte-mode for weight calculations.
> > > +	 */
> > > +	int sched_wfq_weight_mode_supported;
> > > +
> >
> > This is incorrect, as the WFQ support, including the weight aspect of WFQ, is
> already part of the existing set of capabilities: see sched_wfq_weight_max
> and the other sched_wfq_* capability fields
> 
> This is a missing capability for an existing functionality.
> "struct rte_tm_node_params:nonleaf.wfq_weight_mode" field could be
> used to toggle between
> packet-mode or byte-mode for WFQ weights.
> 
> The field if NULL also says that mode defaults to byte-mode.
> 

This capability field should be split into sched_wfq_weight_byte_mode_supported and sched_wfq_weight_packet_mode_supported.

I agree that both of these modes are already supported by the API, but not explicitly mentioned in capability structure, so I think it makes sense to add them to capabilities too. We should have the same look & feel for all the features that accept byte mode and packet mode, i.e. WFQ weight, shaper rate, WRED thresholds. Makes sense?

> >
> > > +	/** Private shaper and scheduler weight mode.
> > > +	 * When non-zero value indicates that all SP children should have
> > > +	 * same weight mode and the same mode applies to private
> > > +	 * shaper as well. This is only valid if
> > > +	 * *sched_wfq_weight_mode_supported* is set.
> > > +	 */
> > > +	int sched_shaper_private_weight_mode;
> > > +
> >
> > If I understand your intention correctly, you are trying to introduce packet
> mode (in addition to the existing byte mode) for (1) scheduler WFQ weights
> and for (2) shaper rates. Basically, the ability to express WFQ weights in bytes
> as well as packets, and the ability to express shaper rates in bytes and well as
> packets. Is this correct?
> 
> Isn't packet mode for (1) already supported via
> "struct rte_tm_node_params:nonleaf.wfq_weight_mode" and
> rte_tm_node_wfq_weight_mode_update() ?
> 
> I'm trying to add support for packet-mode for (2).
> 

See previous note.

> >
> > Assuming yes, we probably need to do it in a slightly different way:
> > 1/ Similar to the WRED packet mode that was introduced by Nikhil Rao's
> patches a while ago (in addition to WRED's byte mode), see WRED capability
> and configuration.
> > 2/ Decouple between scheduler and shaper. So we should add sched_*
> fields and shaper_* fields, but never sched_shaper_*, as it creates a
> functional dependency that does not exist.
> >
> > In line with methodology already used for WRED, I suggest:
> > a) Scheduler WFQ capabilities (TM/level/node):
> sched_wfq_packet_mode_supported, sched_wfq_byte_mode_supported
> 
> I'll add "sched_wfq_packet_mode_supported" field.
> 
> Since currently when "struct
> rte_tm_node_params:nonleaf.wfq_weight_mode" is NULL,
> mode defaults to byte mode, is it needed to have
> "sched_wfq_byte_mode_supported" ?
> Or I should also update the text in "struct rte_tm_node_params" ?
> 

See previous comment. Yes, it makes sense to add both sched_wfq_weight_byte_mode_supported and sched_wfq_weight_packet_mode_supported to capabilities structure, even though they refer to features already supported by the API. We should have the same look 7 feel for all features that support byte mode and packet mode.

> 
> > b) Shaper capabilities (TM/level/node):
> shaper_rate_packet_mode_supported,
> shaper_rate_byte_mode_supported.
> Ack.

OK, great.

> > c) Shaper profile (struct rte_tm_shaper_params): add an integer
> packet_mode flag with 0 = byte-mode (default) and 1 = packet mode for the
> values in struct rte_tm_token_bucket.
> 
> Ok. I'll add a field "packet-mode" in rte_tm_shaper_params and enforce
> restrictions in PMD.

OK, great.

> >
> > It is important to note that the API must allow a combination of packet
> mode and byte mode (for different nodes, not for the same), but an
> implementation can support either a single mode or both (should be
> enforced by the driver).
> >
> > >  	/** WRED packet mode support. When non-zero, this parameter
> > > indicates
> > >  	 * that there is at least one leaf node that supports the WRED packet
> > >  	 * mode, which might not be true for all the leaf nodes. In packet
> > > @@ -554,6 +568,21 @@ struct rte_tm_level_capabilities {
> > >  			 */
> > >  			uint32_t sched_wfq_weight_max;
> > >
> > > +			/** WFQ weight mode supported. Non-zero value
> > > indicates
> > > +			 * wfq weight mode is supported and a SP child
> > > +			 * (even a wfq group) can be configured to use
> > > +			 * packet-mode or byte-mode for weight
> > > calculations.
> > > +			 */
> > > +			int sched_wfq_weight_mode_supported;
> > > +
> > > +			/** Private shaper and scheduler weight mode.
> > > +			 * When non-zero value indicates that all SP children
> > > +			 * should have same weight mode and the same
> > > mode
> > > +			 * applies to private shaper as well. This is only
> > > +			 * valid if *sched_wfq_weight_mode_supported* is
> > > set.
> > > +			 */
> > > +			int sched_shaper_private_weight_mode;
> > > +
> > >  			/** Mask of statistics counter types supported by the
> > >  			 * non-leaf nodes on this level. Every supported
> > >  			 * statistics counter type is supported by at least one
> >
> > See above the comments on TM capabilities.
> >
> > > @@ -735,6 +764,21 @@ struct rte_tm_node_capabilities {
> > >  			 * WFQ weight, so WFQ is reduced to FQ.
> > >  			 */
> > >  			uint32_t sched_wfq_weight_max;
> > > +
> > > +			/** WFQ weight mode supported. Non-zero value
> > > indicates
> > > +			 * wfq weight mode is supported and a SP child
> > > +			 * (even a wfq group) can be configured to use
> > > +			 * packet-mode or byte-mode for weight
> > > calculations.
> > > +			 */
> > > +			int sched_wfq_weight_mode_supported;
> > > +
> > > +			/** Private shaper and scheduler weight mode.
> > > +			 * When non-zero value indicates that all SP children
> > > +			 * should have same weight mode and the same
> > > mode
> > > +			 * applies to private shaper as well. This is only
> > > +			 * valid if *sched_wfq_weight_mode_supported* is
> > > set.
> > > +			 */
> > > +			int sched_shaper_private_weight_mode;
> > >  		} nonleaf;
> > >
> > >  		/** Items valid only for leaf nodes. */
> >
> > See above the comments on the TM capabilities.
> >
> > > @@ -836,10 +880,19 @@ struct rte_tm_wred_params {
> > >   * Token bucket
> > >   */
> > >  struct rte_tm_token_bucket {
> > > -	/** Token bucket rate (bytes per second) */
> > > +	/** Token bucket rate. This is in "bytes per second" by default.
> > > +	 * For private shaper attached to node that is set in packet mode
> > > +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> > > +	 * this is interpreted as "packets per second".
> > > +	 */
> > >  	uint64_t rate;
> > >
> > > -	/** Token bucket size (bytes), a.k.a. max burst size */
> > > +	/** Token bucket size, a.k.a. max burst size.
> > > +	 * This is in "bytes" by default.
> > > +	 * For private shaper attached to node that is set in packet mode
> > > +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> > > +	 * this is interpreted as "packets".
> > > +	 */
> > >  	uint64_t size;
> > >  };
> > >
> >
> > Comments are not correct, as API should allow a combination of both the
> packet mode and the byte mode (for different nodes, not for the same
> node), so both capabilities shaper_rate_packet_mode and
> shaper_rate_byte_mode can be set. Hence, the comments should not
> specify a capability, but the fact that these values can specify either byte or
> packets, depending on a flag elsewhere.
> 
> Ok. As per your above comment I'll add field "packet-mode" in "struct
> rte_tm_shaper_params" and update this comment accordingly.

OK, great.

> >
> > > @@ -924,7 +977,10 @@ struct rte_tm_node_params {
> > >  			 * indicates that WFQ is to be used for all priorities.
> > >  			 * When non-NULL, it points to a pre-allocated array
> > > of
> > >  			 * *n_sp_priorities* values, with non-zero value for
> > > -			 * byte-mode and zero for packet-mode.
> > > +			 * byte-mode and zero for packet-mode. The same
> > > mode is
> > > +			 * used for private shaper connected to this node if
> > > +			 * tm capability
> > > *sched_shaper_private_weight_mode* is
> > > +			 * true.
> > >  			 */
> >
> > This comment is incorrect, as sched should not be combined with shaper.
> The user should select between packet mode and byte mode for the WFQ
> weight independently of the mode for the shaper rate, although an
> implementation (driver) should enforce the correct values.
> >
> > >  			int *wfq_weight_mode;
> > >
> > > --
> > > 2.8.4
> >
> >
> > Regards,
> > Cristian
  
Nithin Dabilpuram April 10, 2020, 11:56 a.m. UTC | #5
Thanks Cristian. Agree with your comments, Will send a v2 addressing them.
On Fri, Apr 10, 2020 at 11:45:06AM +0000, Dumitrescu, Cristian wrote:
> 
> 
> > -----Original Message-----
> > From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > Sent: Tuesday, April 7, 2020 6:21 PM
> > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; dev@dpdk.org; jerinj@marvell.com;
> > kkanas@marvell.com
> > Subject: Re: [EXT] RE: [PATCH 1/2] ethdev: add tm cap for private shaper
> > packet mode
> > 
> > Hi Cristian,
> > 
> > Thanks for your comments. I have some queries below.
> > On Tue, Apr 07, 2020 at 04:31:47PM +0000, Dumitrescu, Cristian wrote:
> > > External Email
> > >
> > > ----------------------------------------------------------------------
> > > Hi Nithin,
> > >
> > > > -----Original Message-----
> > > > From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > > Sent: Monday, March 30, 2020 5:00 PM
> > > > To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Thomas
> > Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Andrew
> > > > Rybchenko <arybchenko@solarflare.com>
> > > > Cc: dev@dpdk.org; jerinj@marvell.com; kkanas@marvell.com; Nithin
> > > > Dabilpuram <ndabilpuram@marvell.com>
> > > > Subject: [PATCH 1/2] ethdev: add tm cap for private shaper packet mode
> > > >
> > > > Some NIC hardware have private shaper attached to
> > > > every node and has a limitation where packet mode is applied
> > > > both to the scheduling of a node's children using WFQ and
> > > > shaping of traffic out of the private shaper.
> > > > This cannot be expressed using existing capabilities or configurations.
> > > >
> > > > So this patch adds a tm capability that if set by a PMD implies that
> > > > packet mode when configured is even applied to private shaper
> > > > connected to that node. This also implies the limitation
> > > > that all the SP children of that node should have same mode
> > > > at any point of time i.e either packet mode or byte mode and
> > > > same applies to private shaper in that NIC PMD.
> > > >
> > > > This patch also adds missing capability that tells whether PMD
> > > > supports wfq weight mode or not.
> > > >
> > > > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > > ---
> > > >  lib/librte_ethdev/rte_tm.h | 62
> > > > +++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 59 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> > > > index f9c0cf3..50bcea6 100644
> > > > --- a/lib/librte_ethdev/rte_tm.h
> > > > +++ b/lib/librte_ethdev/rte_tm.h
> > > > @@ -339,6 +339,20 @@ struct rte_tm_capabilities {
> > > >  	 */
> > > >  	uint32_t sched_wfq_weight_max;
> > > >
> > > > +	/** WFQ weight mode supported. Non-zero value indicates wfq
> > > > weight mode
> > > > +	 * is supported and a SP child (even a wfq group) can be configured
> > > > to
> > > > +	 * use packet-mode or byte-mode for weight calculations.
> > > > +	 */
> > > > +	int sched_wfq_weight_mode_supported;
> > > > +
> > >
> > > This is incorrect, as the WFQ support, including the weight aspect of WFQ, is
> > already part of the existing set of capabilities: see sched_wfq_weight_max
> > and the other sched_wfq_* capability fields
> > 
> > This is a missing capability for an existing functionality.
> > "struct rte_tm_node_params:nonleaf.wfq_weight_mode" field could be
> > used to toggle between
> > packet-mode or byte-mode for WFQ weights.
> > 
> > The field if NULL also says that mode defaults to byte-mode.
> > 
> 
> This capability field should be split into sched_wfq_weight_byte_mode_supported and sched_wfq_weight_packet_mode_supported.
> 
> I agree that both of these modes are already supported by the API, but not explicitly mentioned in capability structure, so I think it makes sense to add them to capabilities too. We should have the same look & feel for all the features that accept byte mode and packet mode, i.e. WFQ weight, shaper rate, WRED thresholds. Makes sense?
> 
> > >
> > > > +	/** Private shaper and scheduler weight mode.
> > > > +	 * When non-zero value indicates that all SP children should have
> > > > +	 * same weight mode and the same mode applies to private
> > > > +	 * shaper as well. This is only valid if
> > > > +	 * *sched_wfq_weight_mode_supported* is set.
> > > > +	 */
> > > > +	int sched_shaper_private_weight_mode;
> > > > +
> > >
> > > If I understand your intention correctly, you are trying to introduce packet
> > mode (in addition to the existing byte mode) for (1) scheduler WFQ weights
> > and for (2) shaper rates. Basically, the ability to express WFQ weights in bytes
> > as well as packets, and the ability to express shaper rates in bytes and well as
> > packets. Is this correct?
> > 
> > Isn't packet mode for (1) already supported via
> > "struct rte_tm_node_params:nonleaf.wfq_weight_mode" and
> > rte_tm_node_wfq_weight_mode_update() ?
> > 
> > I'm trying to add support for packet-mode for (2).
> > 
> 
> See previous note.
> 
> > >
> > > Assuming yes, we probably need to do it in a slightly different way:
> > > 1/ Similar to the WRED packet mode that was introduced by Nikhil Rao's
> > patches a while ago (in addition to WRED's byte mode), see WRED capability
> > and configuration.
> > > 2/ Decouple between scheduler and shaper. So we should add sched_*
> > fields and shaper_* fields, but never sched_shaper_*, as it creates a
> > functional dependency that does not exist.
> > >
> > > In line with methodology already used for WRED, I suggest:
> > > a) Scheduler WFQ capabilities (TM/level/node):
> > sched_wfq_packet_mode_supported, sched_wfq_byte_mode_supported
> > 
> > I'll add "sched_wfq_packet_mode_supported" field.
> > 
> > Since currently when "struct
> > rte_tm_node_params:nonleaf.wfq_weight_mode" is NULL,
> > mode defaults to byte mode, is it needed to have
> > "sched_wfq_byte_mode_supported" ?
> > Or I should also update the text in "struct rte_tm_node_params" ?
> > 
> 
> See previous comment. Yes, it makes sense to add both sched_wfq_weight_byte_mode_supported and sched_wfq_weight_packet_mode_supported to capabilities structure, even though they refer to features already supported by the API. We should have the same look 7 feel for all features that support byte mode and packet mode.
> 
> > 
> > > b) Shaper capabilities (TM/level/node):
> > shaper_rate_packet_mode_supported,
> > shaper_rate_byte_mode_supported.
> > Ack.
> 
> OK, great.
> 
> > > c) Shaper profile (struct rte_tm_shaper_params): add an integer
> > packet_mode flag with 0 = byte-mode (default) and 1 = packet mode for the
> > values in struct rte_tm_token_bucket.
> > 
> > Ok. I'll add a field "packet-mode" in rte_tm_shaper_params and enforce
> > restrictions in PMD.
> 
> OK, great.
> 
> > >
> > > It is important to note that the API must allow a combination of packet
> > mode and byte mode (for different nodes, not for the same), but an
> > implementation can support either a single mode or both (should be
> > enforced by the driver).
> > >
> > > >  	/** WRED packet mode support. When non-zero, this parameter
> > > > indicates
> > > >  	 * that there is at least one leaf node that supports the WRED packet
> > > >  	 * mode, which might not be true for all the leaf nodes. In packet
> > > > @@ -554,6 +568,21 @@ struct rte_tm_level_capabilities {
> > > >  			 */
> > > >  			uint32_t sched_wfq_weight_max;
> > > >
> > > > +			/** WFQ weight mode supported. Non-zero value
> > > > indicates
> > > > +			 * wfq weight mode is supported and a SP child
> > > > +			 * (even a wfq group) can be configured to use
> > > > +			 * packet-mode or byte-mode for weight
> > > > calculations.
> > > > +			 */
> > > > +			int sched_wfq_weight_mode_supported;
> > > > +
> > > > +			/** Private shaper and scheduler weight mode.
> > > > +			 * When non-zero value indicates that all SP children
> > > > +			 * should have same weight mode and the same
> > > > mode
> > > > +			 * applies to private shaper as well. This is only
> > > > +			 * valid if *sched_wfq_weight_mode_supported* is
> > > > set.
> > > > +			 */
> > > > +			int sched_shaper_private_weight_mode;
> > > > +
> > > >  			/** Mask of statistics counter types supported by the
> > > >  			 * non-leaf nodes on this level. Every supported
> > > >  			 * statistics counter type is supported by at least one
> > >
> > > See above the comments on TM capabilities.
> > >
> > > > @@ -735,6 +764,21 @@ struct rte_tm_node_capabilities {
> > > >  			 * WFQ weight, so WFQ is reduced to FQ.
> > > >  			 */
> > > >  			uint32_t sched_wfq_weight_max;
> > > > +
> > > > +			/** WFQ weight mode supported. Non-zero value
> > > > indicates
> > > > +			 * wfq weight mode is supported and a SP child
> > > > +			 * (even a wfq group) can be configured to use
> > > > +			 * packet-mode or byte-mode for weight
> > > > calculations.
> > > > +			 */
> > > > +			int sched_wfq_weight_mode_supported;
> > > > +
> > > > +			/** Private shaper and scheduler weight mode.
> > > > +			 * When non-zero value indicates that all SP children
> > > > +			 * should have same weight mode and the same
> > > > mode
> > > > +			 * applies to private shaper as well. This is only
> > > > +			 * valid if *sched_wfq_weight_mode_supported* is
> > > > set.
> > > > +			 */
> > > > +			int sched_shaper_private_weight_mode;
> > > >  		} nonleaf;
> > > >
> > > >  		/** Items valid only for leaf nodes. */
> > >
> > > See above the comments on the TM capabilities.
> > >
> > > > @@ -836,10 +880,19 @@ struct rte_tm_wred_params {
> > > >   * Token bucket
> > > >   */
> > > >  struct rte_tm_token_bucket {
> > > > -	/** Token bucket rate (bytes per second) */
> > > > +	/** Token bucket rate. This is in "bytes per second" by default.
> > > > +	 * For private shaper attached to node that is set in packet mode
> > > > +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> > > > +	 * this is interpreted as "packets per second".
> > > > +	 */
> > > >  	uint64_t rate;
> > > >
> > > > -	/** Token bucket size (bytes), a.k.a. max burst size */
> > > > +	/** Token bucket size, a.k.a. max burst size.
> > > > +	 * This is in "bytes" by default.
> > > > +	 * For private shaper attached to node that is set in packet mode
> > > > +	 * and tm capability *sched_shaper_private_weight_mode* is set,
> > > > +	 * this is interpreted as "packets".
> > > > +	 */
> > > >  	uint64_t size;
> > > >  };
> > > >
> > >
> > > Comments are not correct, as API should allow a combination of both the
> > packet mode and the byte mode (for different nodes, not for the same
> > node), so both capabilities shaper_rate_packet_mode and
> > shaper_rate_byte_mode can be set. Hence, the comments should not
> > specify a capability, but the fact that these values can specify either byte or
> > packets, depending on a flag elsewhere.
> > 
> > Ok. As per your above comment I'll add field "packet-mode" in "struct
> > rte_tm_shaper_params" and update this comment accordingly.
> 
> OK, great.
> 
> > >
> > > > @@ -924,7 +977,10 @@ struct rte_tm_node_params {
> > > >  			 * indicates that WFQ is to be used for all priorities.
> > > >  			 * When non-NULL, it points to a pre-allocated array
> > > > of
> > > >  			 * *n_sp_priorities* values, with non-zero value for
> > > > -			 * byte-mode and zero for packet-mode.
> > > > +			 * byte-mode and zero for packet-mode. The same
> > > > mode is
> > > > +			 * used for private shaper connected to this node if
> > > > +			 * tm capability
> > > > *sched_shaper_private_weight_mode* is
> > > > +			 * true.
> > > >  			 */
> > >
> > > This comment is incorrect, as sched should not be combined with shaper.
> > The user should select between packet mode and byte mode for the WFQ
> > weight independently of the mode for the shaper rate, although an
> > implementation (driver) should enforce the correct values.
> > >
> > > >  			int *wfq_weight_mode;
> > > >
> > > > --
> > > > 2.8.4
> > >
> > >
> > > Regards,
> > > Cristian
  

Patch

diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
index f9c0cf3..50bcea6 100644
--- a/lib/librte_ethdev/rte_tm.h
+++ b/lib/librte_ethdev/rte_tm.h
@@ -339,6 +339,20 @@  struct rte_tm_capabilities {
 	 */
 	uint32_t sched_wfq_weight_max;
 
+	/** WFQ weight mode supported. Non-zero value indicates wfq weight mode
+	 * is supported and a SP child (even a wfq group) can be configured to
+	 * use packet-mode or byte-mode for weight calculations.
+	 */
+	int sched_wfq_weight_mode_supported;
+
+	/** Private shaper and scheduler weight mode.
+	 * When non-zero value indicates that all SP children should have
+	 * same weight mode and the same mode applies to private
+	 * shaper as well. This is only valid if
+	 * *sched_wfq_weight_mode_supported* is set.
+	 */
+	int sched_shaper_private_weight_mode;
+
 	/** WRED packet mode support. When non-zero, this parameter indicates
 	 * that there is at least one leaf node that supports the WRED packet
 	 * mode, which might not be true for all the leaf nodes. In packet
@@ -554,6 +568,21 @@  struct rte_tm_level_capabilities {
 			 */
 			uint32_t sched_wfq_weight_max;
 
+			/** WFQ weight mode supported. Non-zero value indicates
+			 * wfq weight mode is supported and a SP child
+			 * (even a wfq group) can be configured to use
+			 * packet-mode or byte-mode for weight calculations.
+			 */
+			int sched_wfq_weight_mode_supported;
+
+			/** Private shaper and scheduler weight mode.
+			 * When non-zero value indicates that all SP children
+			 * should have same weight mode and the same mode
+			 * applies to private shaper as well. This is only
+			 * valid if *sched_wfq_weight_mode_supported* is set.
+			 */
+			int sched_shaper_private_weight_mode;
+
 			/** Mask of statistics counter types supported by the
 			 * non-leaf nodes on this level. Every supported
 			 * statistics counter type is supported by at least one
@@ -735,6 +764,21 @@  struct rte_tm_node_capabilities {
 			 * WFQ weight, so WFQ is reduced to FQ.
 			 */
 			uint32_t sched_wfq_weight_max;
+
+			/** WFQ weight mode supported. Non-zero value indicates
+			 * wfq weight mode is supported and a SP child
+			 * (even a wfq group) can be configured to use
+			 * packet-mode or byte-mode for weight calculations.
+			 */
+			int sched_wfq_weight_mode_supported;
+
+			/** Private shaper and scheduler weight mode.
+			 * When non-zero value indicates that all SP children
+			 * should have same weight mode and the same mode
+			 * applies to private shaper as well. This is only
+			 * valid if *sched_wfq_weight_mode_supported* is set.
+			 */
+			int sched_shaper_private_weight_mode;
 		} nonleaf;
 
 		/** Items valid only for leaf nodes. */
@@ -836,10 +880,19 @@  struct rte_tm_wred_params {
  * Token bucket
  */
 struct rte_tm_token_bucket {
-	/** Token bucket rate (bytes per second) */
+	/** Token bucket rate. This is in "bytes per second" by default.
+	 * For private shaper attached to node that is set in packet mode
+	 * and tm capability *sched_shaper_private_weight_mode* is set,
+	 * this is interpreted as "packets per second".
+	 */
 	uint64_t rate;
 
-	/** Token bucket size (bytes), a.k.a. max burst size */
+	/** Token bucket size, a.k.a. max burst size.
+	 * This is in "bytes" by default.
+	 * For private shaper attached to node that is set in packet mode
+	 * and tm capability *sched_shaper_private_weight_mode* is set,
+	 * this is interpreted as "packets".
+	 */
 	uint64_t size;
 };
 
@@ -924,7 +977,10 @@  struct rte_tm_node_params {
 			 * indicates that WFQ is to be used for all priorities.
 			 * When non-NULL, it points to a pre-allocated array of
 			 * *n_sp_priorities* values, with non-zero value for
-			 * byte-mode and zero for packet-mode.
+			 * byte-mode and zero for packet-mode. The same mode is
+			 * used for private shaper connected to this node if
+			 * tm capability *sched_shaper_private_weight_mode* is
+			 * true.
 			 */
 			int *wfq_weight_mode;