[v2,1/4] ethdev: add tm support for shaper config in pkt mode

Message ID 20200411114430.18506-1-nithind1988@gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v2,1/4] ethdev: add tm support for shaper config in pkt mode |

Checks

Context Check Description
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
ci/checkpatch warning coding style issues

Commit Message

Nithin Dabilpuram April 11, 2020, 11:44 a.m. UTC
  From: Nithin Dabilpuram <ndabilpuram@marvell.com>

Some NIC hardware support shaper to work in packet mode i.e
shaping or ratelimiting traffic is in packets per second (PPS) as
opposed to default bytes per second (BPS). Hence this patch
adds support to configure shared or private shaper in packet mode,
provide rate in PPS and add related tm capabilities in port/level/node
capability structures.

This patch also updates tm port/level/node capability structures with
exiting features of scheduler wfq packet mode, scheduler wfq byte mode
and private/shared shaper byte mode.

Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---

v1..v2:
- Add seperate capability for shaper and scheduler pktmode and bytemode.
- Add packet_mode field in struct rte_tm_shaper_params to indicate
packet mode shaper profile.

 lib/librte_ethdev/rte_tm.h | 156 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 154 insertions(+), 2 deletions(-)
  

Comments

Ferruh Yigit April 16, 2020, 1:48 p.m. UTC | #1
On 4/11/2020 12:44 PM, Nithin Dabilpuram wrote:
> From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> 
> Some NIC hardware support shaper to work in packet mode i.e
> shaping or ratelimiting traffic is in packets per second (PPS) as
> opposed to default bytes per second (BPS). Hence this patch
> adds support to configure shared or private shaper in packet mode,
> provide rate in PPS and add related tm capabilities in port/level/node
> capability structures.
> 
> This patch also updates tm port/level/node capability structures with
> exiting features of scheduler wfq packet mode, scheduler wfq byte mode
> and private/shared shaper byte mode.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
> 
> v1..v2:
> - Add seperate capability for shaper and scheduler pktmode and bytemode.
> - Add packet_mode field in struct rte_tm_shaper_params to indicate
> packet mode shaper profile.
> 

Hi Cristian,

Can you please review this series?

Thanks,
ferruh
  
Nithin Dabilpuram April 21, 2020, 5:11 a.m. UTC | #2
Ping.

On Thu, Apr 16, 2020 at 02:48:49PM +0100, Ferruh Yigit wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On 4/11/2020 12:44 PM, Nithin Dabilpuram wrote:
> > From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > 
> > Some NIC hardware support shaper to work in packet mode i.e
> > shaping or ratelimiting traffic is in packets per second (PPS) as
> > opposed to default bytes per second (BPS). Hence this patch
> > adds support to configure shared or private shaper in packet mode,
> > provide rate in PPS and add related tm capabilities in port/level/node
> > capability structures.
> > 
> > This patch also updates tm port/level/node capability structures with
> > exiting features of scheduler wfq packet mode, scheduler wfq byte mode
> > and private/shared shaper byte mode.
> > 
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > ---
> > 
> > v1..v2:
> > - Add seperate capability for shaper and scheduler pktmode and bytemode.
> > - Add packet_mode field in struct rte_tm_shaper_params to indicate
> > packet mode shaper profile.
> > 
> 
> Hi Cristian,
> 
> Can you please review this series?
> 
> Thanks,
> ferruh
  
Cristian Dumitrescu April 21, 2020, 9:30 a.m. UTC | #3
Hi Nithin,

> -----Original Message-----
> From: Nithin Dabilpuram <nithind1988@gmail.com>
> Sent: Saturday, April 11, 2020 12:44 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 v2 1/4] ethdev: add tm support for shaper config in pkt
> mode
> 
> From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> 
> Some NIC hardware support shaper to work in packet mode i.e
> shaping or ratelimiting traffic is in packets per second (PPS) as
> opposed to default bytes per second (BPS). Hence this patch
> adds support to configure shared or private shaper in packet mode,
> provide rate in PPS and add related tm capabilities in port/level/node
> capability structures.
> 
> This patch also updates tm port/level/node capability structures with
> exiting features of scheduler wfq packet mode, scheduler wfq byte mode
> and private/shared shaper byte mode.
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
> 
> v1..v2:
> - Add seperate capability for shaper and scheduler pktmode and bytemode.
> - Add packet_mode field in struct rte_tm_shaper_params to indicate
> packet mode shaper profile.
> 
>  lib/librte_ethdev/rte_tm.h | 156
> ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> index f9c0cf3..38fff4c 100644
> --- a/lib/librte_ethdev/rte_tm.h
> +++ b/lib/librte_ethdev/rte_tm.h
> @@ -250,6 +250,23 @@ struct rte_tm_capabilities {
>  	 */
>  	uint64_t shaper_private_rate_max;
> 
> +	/** Shaper private packet mode supported. When non-zero, this
> parameter
> +	 * indicates that there is atleast one node that can be configured

Typo (please fix all occurrences): atleast -> at least

> +	 * with packet mode in it's private shaper. When shaper is configured

Typo (please fix all occurrences): it's -> its

> +	 * in packet mode, committed/peak rate provided is interpreted
> +	 * in packets per second.
> +	 */
> +	int shaper_private_packet_mode_supported;
> +
> +	/** Shaper private byte mode supported. When non-zero, this
> parameter
> +	 * indicates that there is atleast one node that can be configured
> +	 * with byte mode in it's private shaper. When shaper is configured
> +	 * in byte mode, committed/peak rate provided is interpreted in
> +	 * bytes per second.
> +	 */
> +	int shaper_private_byte_mode_supported;
> +
> +
>  	/** Maximum number of shared shapers. The value of zero indicates
> that
>  	 * shared shapers are not supported.
>  	 */
> @@ -284,6 +301,21 @@ struct rte_tm_capabilities {
>  	 */
>  	uint64_t shaper_shared_rate_max;
> 
> +	/** Shaper shared packet mode supported. When non-zero, this
> parameter
> +	 * indicates a shared shaper can be configured with packet mode.
> +	 * When shared shaper is configured in packet mode,
> committed/peak rate
> +	 * provided is interpreted in packets per second.
> +	 */
> +	int shaper_shared_packet_mode_supported;
> +
> +	/** Shaper shared byte mode supported. When non-zero, this
> parameter
> +	 * indicates that a shared shaper can be configured with byte mode.
> +	 * When shared shaper is configured in byte mode, committed/peak
> rate
> +	 * provided is interpreted in bytes per second.
> +	 */
> +	int shaper_shared_byte_mode_supported;
> +
> +
>  	/** Minimum value allowed for packet length adjustment for any
> private
>  	 * or shared shaper.
>  	 */
> @@ -339,6 +371,22 @@ struct rte_tm_capabilities {
>  	 */
>  	uint32_t sched_wfq_weight_max;
> 
> +	/** WFQ packet mode supported. When non-zero, this parameter
> indicates
> +	 * that there is at least one non-leaf node that supports packet mode
> +	 * for WFQ among its children. WFQ weights will be applied against
> +	 * packet count for scheduling children when a non-leaf node
> +	 * is configured appropriately.
> +	 */
> +	int sched_wfq_packet_mode_supported;
> +
> +	/** WFQ byte mode supported. When non-zero, this parameter
> indicates
> +	 * that there is at least one non-leaf node that supports byte mode
> +	 * for WFQ among its children. WFQ weights will be applied against
> +	 * bytes for scheduling children when a non-leaf node is configured
> +	 * appropriately.
> +	 */
> +	int sched_wfq_byte_mode_supported;
> +
>  	/** 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
> @@ -485,6 +533,24 @@ struct rte_tm_level_capabilities {
>  			 */
>  			uint64_t shaper_private_rate_max;
> 
> +			/** Shaper private packet mode supported. When
> non-zero,
> +			 * this parameter indicates there is atleast one
> +			 * non-leaf node at this level that can be configured
> +			 * with packet mode in its private shaper. When
> private
> +			 * shaper is configured in packet mode,
> committed/peak
> +			 * rate provided is interpreted in packets per second.
> +			 */
> +			int shaper_private_packet_mode_supported;
> +
> +			/** Shaper private byte mode supported. When
> non-zero,
> +			 * this parameter indicates there is atleast one
> +			 * non-leaf node at this level that can be configured
> +			 * with byte mode in its private shaper. When private
> +			 * shaper is configured in byte mode,
> committed/peak
> +			 * rate provided is interpreted in bytes per second.
> +			 */
> +			int shaper_private_byte_mode_supported;
> +
>  			/** Maximum number of shared shapers that any
> non-leaf
>  			 * node on this level can be part of. The value of zero
>  			 * indicates that shared shapers are not supported by
> @@ -554,6 +620,25 @@ struct rte_tm_level_capabilities {
>  			 */
>  			uint32_t sched_wfq_weight_max;
> 
> +			/** WFQ packet mode supported. When non-zero,
> this
> +			 * parameter indicates that there is at least one
> +			 * non-leaf node at this level that supports packet
> +			 * mode for WFQ among its children. WFQ weights
> will
> +			 * be applied against packet count for scheduling
> +			 * children when a non-leaf node is configured
> +			 * appropriately.
> +			 */
> +			int sched_wfq_packet_mode_supported;
> +
> +			/** WFQ byte mode supported. When non-zero, this
> +			 * parameter indicates that there is at least one
> +			 * non-leaf node at this level that supports byte
> +			 * mode for WFQ among its children. WFQ weights
> will
> +			 * be applied against bytes for scheduling children
> +			 * when a non-leaf node is configured appropriately.
> +			 */
> +			int sched_wfq_byte_mode_supported;
> +
>  			/** 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
> @@ -596,6 +681,24 @@ struct rte_tm_level_capabilities {
>  			 */
>  			uint64_t shaper_private_rate_max;
> 
> +			/** Shaper private packet mode supported. When
> non-zero,
> +			 * this parameter indicates there is atleast one leaf
> +			 * node at this level that can be configured with
> +			 * packet mode in its private shaper. When private
> +			 * shaper is configured in packet mode,
> committed/peak
> +			 * rate provided is interpreted in packets per second.
> +			 */
> +			int shaper_private_packet_mode_supported;
> +
> +			/** Shaper private byte mode supported. When
> non-zero,
> +			 * this parameter indicates there is atleast one leaf
> +			 * node at this level that can be configured with
> +			 * byte mode in its private shaper. When private
> shaper
> +			 * is configured in byte mode, committed/peak rate
> +			 * provided is interpreted in bytes per second.
> +			 */
> +			int shaper_private_byte_mode_supported;
> +
>  			/** Maximum number of shared shapers that any
> leaf node
>  			 * on this level can be part of. The value of zero
>  			 * indicates that shared shapers are not supported by

You are missing the shaper_shared_(packet, byte)_mode supported for non-leaf and leaf nodes in struct rte_tm_level_capabilities.

The description of this nodes should be aligned with the description of e.g. shaper_shared_n_max field: basically, we want to say that, when true, the flag signifies there is at least on non-leaf/leaf node on this level that can be part of a shared shaper that works in packet/byte mode. Makes sense?

> @@ -686,6 +789,20 @@ struct rte_tm_node_capabilities {
>  	 */
>  	uint64_t shaper_private_rate_max;
> 
> +	/** Shaper private packet mode supported. When non-zero, this
> parameter
> +	 * indicates private shaper of current node can be configured with
> +	 * packet mode. When configured in packet mode, committed/peak
> rate
> +	 * provided is interpreted in packets per second.
> +	 */
> +	int shaper_private_packet_mode_supported;
> +
> +	/** Shaper private byte mode supported. When non-zero, this
> parameter
> +	 * indicates private shaper of current node can be configured with
> +	 * byte mode. When configured in byte mode, committed/peak rate
> +	 * provided is interpreted in bytes per second.
> +	 */
> +	int shaper_private_byte_mode_supported;
> +
>  	/** Maximum number of shared shapers the current node can be
> part of.
>  	 * The value of zero indicates that shared shapers are not supported
> by
>  	 * the current node.

You are missing the shaper_shared_(packet, byte)_mode supported (applicable for both non-leaf and leaf nodes, so it should occur only once in the common part of the struct) in struct rte_tm_node_capabilities. See the above comment on the applicable description style.

> @@ -735,6 +852,23 @@ struct rte_tm_node_capabilities {
>  			 * WFQ weight, so WFQ is reduced to FQ.
>  			 */
>  			uint32_t sched_wfq_weight_max;
> +
> +			/** WFQ packet mode supported. When non-zero,
> this
> +			 * parameter indicates that current node supports
> packet
> +			 * mode for WFQ among its children. WFQ weights
> will be
> +			 * applied against packet count for scheduling
> children
> +			 * when configured appropriately.
> +			 */
> +			int sched_wfq_packet_mode_supported;
> +
> +			/** WFQ byte mode supported. When non-zero, this
> +			 * parameter indicates that current node supports
> byte
> +			 * mode for WFQ among its children. WFQ weights
> will be
> +			 * applied against  bytes for scheduling children when
> +			 * configured appropriately.
> +			 */
> +			int sched_wfq_byte_mode_supported;
> +
>  		} nonleaf;
> 
>  		/** Items valid only for leaf nodes. */
> @@ -836,10 +970,10 @@ struct rte_tm_wred_params {
>   * Token bucket
>   */
>  struct rte_tm_token_bucket {
> -	/** Token bucket rate (bytes per second) */
> +	/** Token bucket rate (bytes per second or packets per second) */
>  	uint64_t rate;
> 
> -	/** Token bucket size (bytes), a.k.a. max burst size */
> +	/** Token bucket size (bytes or packets), a.k.a. max burst size */
>  	uint64_t size;
>  };
> 
> @@ -860,6 +994,11 @@ struct rte_tm_token_bucket {
>   * Dual rate shapers use both the committed and the peak token buckets.
> The
>   * rate of the peak bucket has to be bigger than zero, as well as greater than
>   * or equal to the rate of the committed bucket.
> + *
> + * @see struct
> rte_tm_capabilities::shaper_private_packet_mode_supported
> + * @see struct rte_tm_capabilities::shaper_private_byte_mode_supported
> + * @see struct
> rte_tm_capabilities::shaper_shared_packet_mode_supported
> + * @see struct rte_tm_capabilities::shaper_shared_byte_mode_supported
>   */
>  struct rte_tm_shaper_params {
>  	/** Committed token bucket */
> @@ -874,6 +1013,17 @@ struct rte_tm_shaper_params {
>  	 * RTE_TM_ETH_FRAMING_OVERHEAD_FCS).
>  	 */
>  	int32_t pkt_length_adjust;
> +
> +	/** When zero, the private or shared shaper that is associated to this
> +	 * profile works in byte mode and hence *rate* and *size* fields in
> +	 * both token bucket configurations are specified in bytes per second
> +	 * and bytes respectively.
> +	 * When non-zero, that private or shared shaper works in packet
> mode and
> +	 * hence *rate* and *size* fields in both token bucket configurations
> +	 * are specified in packets per second and packets respectively. In
> +	 * packet mode, *pkt_length_adjust* is ignored.
> +	 */

Please move the last statement ("In packet mode, *pkt_length_adjust* is ignored.") to the description of the pkt_length_adjust field.

> +	int packet_mode;
>  };
> 
>  /**
> @@ -925,6 +1075,8 @@ struct rte_tm_node_params {
>  			 * 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.
> +			 * @see struct
> rte_tm_node_capabilities::sched_wfq_packet_mode_supported
> +			 * @see struct
> rte_tm_node_capabilities::sched_wfq_byte_mode_supported
>  			 */
>  			int *wfq_weight_mode;
> 
> --
> 2.8.4

Regards,
Cristian
  
Nithin Dabilpuram April 21, 2020, 9:58 a.m. UTC | #4
Hi Cristian,

On Tue, Apr 21, 2020 at 09:30:55AM +0000, Dumitrescu, Cristian wrote:
> Hi Nithin,
> 
> > -----Original Message-----
> > From: Nithin Dabilpuram <nithind1988@gmail.com>
> > Sent: Saturday, April 11, 2020 12:44 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 v2 1/4] ethdev: add tm support for shaper config in pkt
> > mode
> > 
> > From: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > 
> > Some NIC hardware support shaper to work in packet mode i.e
> > shaping or ratelimiting traffic is in packets per second (PPS) as
> > opposed to default bytes per second (BPS). Hence this patch
> > adds support to configure shared or private shaper in packet mode,
> > provide rate in PPS and add related tm capabilities in port/level/node
> > capability structures.
> > 
> > This patch also updates tm port/level/node capability structures with
> > exiting features of scheduler wfq packet mode, scheduler wfq byte mode
> > and private/shared shaper byte mode.
> > 
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > ---
> > 
> > v1..v2:
> > - Add seperate capability for shaper and scheduler pktmode and bytemode.
> > - Add packet_mode field in struct rte_tm_shaper_params to indicate
> > packet mode shaper profile.
> > 
> >  lib/librte_ethdev/rte_tm.h | 156
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 154 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
> > index f9c0cf3..38fff4c 100644
> > --- a/lib/librte_ethdev/rte_tm.h
> > +++ b/lib/librte_ethdev/rte_tm.h
> > @@ -250,6 +250,23 @@ struct rte_tm_capabilities {
> >  	 */
> >  	uint64_t shaper_private_rate_max;
> > 
> > +	/** Shaper private packet mode supported. When non-zero, this
> > parameter
> > +	 * indicates that there is atleast one node that can be configured
> 
> Typo (please fix all occurrences): atleast -> at least
Ack.
> 
> > +	 * with packet mode in it's private shaper. When shaper is configured
> 
> Typo (please fix all occurrences): it's -> its
Ack.
> 
> > +	 * in packet mode, committed/peak rate provided is interpreted
> > +	 * in packets per second.
> > +	 */
> > +	int shaper_private_packet_mode_supported;
> > +
> > +	/** Shaper private byte mode supported. When non-zero, this
> > parameter
> > +	 * indicates that there is atleast one node that can be configured
> > +	 * with byte mode in it's private shaper. When shaper is configured
> > +	 * in byte mode, committed/peak rate provided is interpreted in
> > +	 * bytes per second.
> > +	 */
> > +	int shaper_private_byte_mode_supported;
> > +
> > +
> >  	/** Maximum number of shared shapers. The value of zero indicates
> > that
> >  	 * shared shapers are not supported.
> >  	 */
> > @@ -284,6 +301,21 @@ struct rte_tm_capabilities {
> >  	 */
> >  	uint64_t shaper_shared_rate_max;
> > 
> > +	/** Shaper shared packet mode supported. When non-zero, this
> > parameter
> > +	 * indicates a shared shaper can be configured with packet mode.
> > +	 * When shared shaper is configured in packet mode,
> > committed/peak rate
> > +	 * provided is interpreted in packets per second.
> > +	 */
> > +	int shaper_shared_packet_mode_supported;
> > +
> > +	/** Shaper shared byte mode supported. When non-zero, this
> > parameter
> > +	 * indicates that a shared shaper can be configured with byte mode.
> > +	 * When shared shaper is configured in byte mode, committed/peak
> > rate
> > +	 * provided is interpreted in bytes per second.
> > +	 */
> > +	int shaper_shared_byte_mode_supported;
> > +
> > +
> >  	/** Minimum value allowed for packet length adjustment for any
> > private
> >  	 * or shared shaper.
> >  	 */
> > @@ -339,6 +371,22 @@ struct rte_tm_capabilities {
> >  	 */
> >  	uint32_t sched_wfq_weight_max;
> > 
> > +	/** WFQ packet mode supported. When non-zero, this parameter
> > indicates
> > +	 * that there is at least one non-leaf node that supports packet mode
> > +	 * for WFQ among its children. WFQ weights will be applied against
> > +	 * packet count for scheduling children when a non-leaf node
> > +	 * is configured appropriately.
> > +	 */
> > +	int sched_wfq_packet_mode_supported;
> > +
> > +	/** WFQ byte mode supported. When non-zero, this parameter
> > indicates
> > +	 * that there is at least one non-leaf node that supports byte mode
> > +	 * for WFQ among its children. WFQ weights will be applied against
> > +	 * bytes for scheduling children when a non-leaf node is configured
> > +	 * appropriately.
> > +	 */
> > +	int sched_wfq_byte_mode_supported;
> > +
> >  	/** 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
> > @@ -485,6 +533,24 @@ struct rte_tm_level_capabilities {
> >  			 */
> >  			uint64_t shaper_private_rate_max;
> > 
> > +			/** Shaper private packet mode supported. When
> > non-zero,
> > +			 * this parameter indicates there is atleast one
> > +			 * non-leaf node at this level that can be configured
> > +			 * with packet mode in its private shaper. When
> > private
> > +			 * shaper is configured in packet mode,
> > committed/peak
> > +			 * rate provided is interpreted in packets per second.
> > +			 */
> > +			int shaper_private_packet_mode_supported;
> > +
> > +			/** Shaper private byte mode supported. When
> > non-zero,
> > +			 * this parameter indicates there is atleast one
> > +			 * non-leaf node at this level that can be configured
> > +			 * with byte mode in its private shaper. When private
> > +			 * shaper is configured in byte mode,
> > committed/peak
> > +			 * rate provided is interpreted in bytes per second.
> > +			 */
> > +			int shaper_private_byte_mode_supported;
> > +
> >  			/** Maximum number of shared shapers that any
> > non-leaf
> >  			 * node on this level can be part of. The value of zero
> >  			 * indicates that shared shapers are not supported by
> > @@ -554,6 +620,25 @@ struct rte_tm_level_capabilities {
> >  			 */
> >  			uint32_t sched_wfq_weight_max;
> > 
> > +			/** WFQ packet mode supported. When non-zero,
> > this
> > +			 * parameter indicates that there is at least one
> > +			 * non-leaf node at this level that supports packet
> > +			 * mode for WFQ among its children. WFQ weights
> > will
> > +			 * be applied against packet count for scheduling
> > +			 * children when a non-leaf node is configured
> > +			 * appropriately.
> > +			 */
> > +			int sched_wfq_packet_mode_supported;
> > +
> > +			/** WFQ byte mode supported. When non-zero, this
> > +			 * parameter indicates that there is at least one
> > +			 * non-leaf node at this level that supports byte
> > +			 * mode for WFQ among its children. WFQ weights
> > will
> > +			 * be applied against bytes for scheduling children
> > +			 * when a non-leaf node is configured appropriately.
> > +			 */
> > +			int sched_wfq_byte_mode_supported;
> > +
> >  			/** 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
> > @@ -596,6 +681,24 @@ struct rte_tm_level_capabilities {
> >  			 */
> >  			uint64_t shaper_private_rate_max;
> > 
> > +			/** Shaper private packet mode supported. When
> > non-zero,
> > +			 * this parameter indicates there is atleast one leaf
> > +			 * node at this level that can be configured with
> > +			 * packet mode in its private shaper. When private
> > +			 * shaper is configured in packet mode,
> > committed/peak
> > +			 * rate provided is interpreted in packets per second.
> > +			 */
> > +			int shaper_private_packet_mode_supported;
> > +
> > +			/** Shaper private byte mode supported. When
> > non-zero,
> > +			 * this parameter indicates there is atleast one leaf
> > +			 * node at this level that can be configured with
> > +			 * byte mode in its private shaper. When private
> > shaper
> > +			 * is configured in byte mode, committed/peak rate
> > +			 * provided is interpreted in bytes per second.
> > +			 */
> > +			int shaper_private_byte_mode_supported;
> > +
> >  			/** Maximum number of shared shapers that any
> > leaf node
> >  			 * on this level can be part of. The value of zero
> >  			 * indicates that shared shapers are not supported by
> 
> You are missing the shaper_shared_(packet, byte)_mode supported for non-leaf and leaf nodes in struct rte_tm_level_capabilities.
> 
> The description of this nodes should be aligned with the description of e.g. shaper_shared_n_max field: basically, we want to say that, when true, the flag signifies there is at least on non-leaf/leaf node on this level that can be part of a shared shaper that works in packet/byte mode. Makes sense?

I intentionally didn't add shaper_shared_(packet, byte)_mode in node and level
capabilities and added it in only global cap assuming existing semantics are 
enforcing that. 

Currently, except for 'shaper_shared_n_max', all the other existing shared shaper capabilities like 
shaper_shared_dual_rate_n_max, shaper_shared_rate_min, etc are only provided in global cap.

I felt the semantics are as such because, shared shaper doesn't really belong to any node
or level and any node from any level can attach to a particular shared shaper. Isn't it so
?



> 
> > @@ -686,6 +789,20 @@ struct rte_tm_node_capabilities {
> >  	 */
> >  	uint64_t shaper_private_rate_max;
> > 
> > +	/** Shaper private packet mode supported. When non-zero, this
> > parameter
> > +	 * indicates private shaper of current node can be configured with
> > +	 * packet mode. When configured in packet mode, committed/peak
> > rate
> > +	 * provided is interpreted in packets per second.
> > +	 */
> > +	int shaper_private_packet_mode_supported;
> > +
> > +	/** Shaper private byte mode supported. When non-zero, this
> > parameter
> > +	 * indicates private shaper of current node can be configured with
> > +	 * byte mode. When configured in byte mode, committed/peak rate
> > +	 * provided is interpreted in bytes per second.
> > +	 */
> > +	int shaper_private_byte_mode_supported;
> > +
> >  	/** Maximum number of shared shapers the current node can be
> > part of.
> >  	 * The value of zero indicates that shared shapers are not supported
> > by
> >  	 * the current node.
> 
> You are missing the shaper_shared_(packet, byte)_mode supported (applicable for both non-leaf and leaf nodes, so it should occur only once in the common part of the struct) in struct rte_tm_node_capabilities. See the above comment on the applicable description style.

Please see above.
>
> > @@ -735,6 +852,23 @@ struct rte_tm_node_capabilities {
> >  			 * WFQ weight, so WFQ is reduced to FQ.
> >  			 */
> >  			uint32_t sched_wfq_weight_max;
> > +
> > +			/** WFQ packet mode supported. When non-zero,
> > this
> > +			 * parameter indicates that current node supports
> > packet
> > +			 * mode for WFQ among its children. WFQ weights
> > will be
> > +			 * applied against packet count for scheduling
> > children
> > +			 * when configured appropriately.
> > +			 */
> > +			int sched_wfq_packet_mode_supported;
> > +
> > +			/** WFQ byte mode supported. When non-zero, this
> > +			 * parameter indicates that current node supports
> > byte
> > +			 * mode for WFQ among its children. WFQ weights
> > will be
> > +			 * applied against  bytes for scheduling children when
> > +			 * configured appropriately.
> > +			 */
> > +			int sched_wfq_byte_mode_supported;
> > +
> >  		} nonleaf;
> > 
> >  		/** Items valid only for leaf nodes. */
> > @@ -836,10 +970,10 @@ struct rte_tm_wred_params {
> >   * Token bucket
> >   */
> >  struct rte_tm_token_bucket {
> > -	/** Token bucket rate (bytes per second) */
> > +	/** Token bucket rate (bytes per second or packets per second) */
> >  	uint64_t rate;
> > 
> > -	/** Token bucket size (bytes), a.k.a. max burst size */
> > +	/** Token bucket size (bytes or packets), a.k.a. max burst size */
> >  	uint64_t size;
> >  };
> > 
> > @@ -860,6 +994,11 @@ struct rte_tm_token_bucket {
> >   * Dual rate shapers use both the committed and the peak token buckets.
> > The
> >   * rate of the peak bucket has to be bigger than zero, as well as greater than
> >   * or equal to the rate of the committed bucket.
> > + *
> > + * @see struct
> > rte_tm_capabilities::shaper_private_packet_mode_supported
> > + * @see struct rte_tm_capabilities::shaper_private_byte_mode_supported
> > + * @see struct
> > rte_tm_capabilities::shaper_shared_packet_mode_supported
> > + * @see struct rte_tm_capabilities::shaper_shared_byte_mode_supported
> >   */
> >  struct rte_tm_shaper_params {
> >  	/** Committed token bucket */
> > @@ -874,6 +1013,17 @@ struct rte_tm_shaper_params {
> >  	 * RTE_TM_ETH_FRAMING_OVERHEAD_FCS).
> >  	 */
> >  	int32_t pkt_length_adjust;
> > +
> > +	/** When zero, the private or shared shaper that is associated to this
> > +	 * profile works in byte mode and hence *rate* and *size* fields in
> > +	 * both token bucket configurations are specified in bytes per second
> > +	 * and bytes respectively.
> > +	 * When non-zero, that private or shared shaper works in packet
> > mode and
> > +	 * hence *rate* and *size* fields in both token bucket configurations
> > +	 * are specified in packets per second and packets respectively. In
> > +	 * packet mode, *pkt_length_adjust* is ignored.
> > +	 */
> 
> Please move the last statement ("In packet mode, *pkt_length_adjust* is ignored.") to the description of the pkt_length_adjust field.
Ack.
> 
> > +	int packet_mode;
> >  };
> > 
> >  /**
> > @@ -925,6 +1075,8 @@ struct rte_tm_node_params {
> >  			 * 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.
> > +			 * @see struct
> > rte_tm_node_capabilities::sched_wfq_packet_mode_supported
> > +			 * @see struct
> > rte_tm_node_capabilities::sched_wfq_byte_mode_supported
> >  			 */
> >  			int *wfq_weight_mode;
> > 
> > --
> > 2.8.4
> 
> Regards,
> Cristian
  
Cristian Dumitrescu April 21, 2020, 10:23 a.m. UTC | #5
Hi Nithin,

<snip>...

> > You are missing the shaper_shared_(packet, byte)_mode supported for
> non-leaf and leaf nodes in struct rte_tm_level_capabilities.
> >
> > The description of this nodes should be aligned with the description of e.g.
> shaper_shared_n_max field: basically, we want to say that, when true, the
> flag signifies there is at least on non-leaf/leaf node on this level that can be
> part of a shared shaper that works in packet/byte mode. Makes sense?
> 
> I intentionally didn't add shaper_shared_(packet, byte)_mode in node and
> level
> capabilities and added it in only global cap assuming existing semantics are
> enforcing that.
> 
> Currently, except for 'shaper_shared_n_max', all the other existing shared
> shaper capabilities like
> shaper_shared_dual_rate_n_max, shaper_shared_rate_min, etc are only
> provided in global cap.
> 
> I felt the semantics are as such because, shared shaper doesn't really belong
> to any node
> or level and any node from any level can attach to a particular shared shaper.
> Isn't it so
> ?

That's exactly why we need to formulate node/level capability from node's perspective, and not from the shared shaper's perspective, as a shared shaper is by definition related to a set of nodes, not just one node.

The fact that a given node can be part of a shared shaper that works in packet or byte mode, etc is a node capability in itself, right? So the node's capability called "shaper_shared_(packet, byte)_mode" being supported by the node means that this specific node can be part of a shared shaper that has those properties. To me, this is a valuable thing to capture in node/level capabilities.

We already have other node level capabilities for shared shaper, and we apply the same rationale there.

What do you think?

Regards,
Cristian
  
Nithin Dabilpuram April 21, 2020, 11:55 a.m. UTC | #6
On Tue, Apr 21, 2020 at 10:23:11AM +0000, Dumitrescu, Cristian wrote:
> External Email
> 
> ----------------------------------------------------------------------
> Hi Nithin,
> 
> <snip>...
> 
> > > You are missing the shaper_shared_(packet, byte)_mode supported for
> > non-leaf and leaf nodes in struct rte_tm_level_capabilities.
> > >
> > > The description of this nodes should be aligned with the description of e.g.
> > shaper_shared_n_max field: basically, we want to say that, when true, the
> > flag signifies there is at least on non-leaf/leaf node on this level that can be
> > part of a shared shaper that works in packet/byte mode. Makes sense?
> > 
> > I intentionally didn't add shaper_shared_(packet, byte)_mode in node and
> > level
> > capabilities and added it in only global cap assuming existing semantics are
> > enforcing that.
> > 
> > Currently, except for 'shaper_shared_n_max', all the other existing shared
> > shaper capabilities like
> > shaper_shared_dual_rate_n_max, shaper_shared_rate_min, etc are only
> > provided in global cap.
> > 
> > I felt the semantics are as such because, shared shaper doesn't really belong
> > to any node
> > or level and any node from any level can attach to a particular shared shaper.
> > Isn't it so
> > ?
> 
> That's exactly why we need to formulate node/level capability from node's perspective, and not from the shared shaper's perspective, as a shared shaper is by definition related to a set of nodes, not just one node.
> 
> The fact that a given node can be part of a shared shaper that works in packet or byte mode, etc is a node capability in itself, right? So the node's capability called "shaper_shared_(packet, byte)_mode" being supported by the node means that this specific node can be part of a shared shaper that has those properties. To me, this is a valuable thing to capture in node/level capabilities.
> 
> We already have other node level capabilities for shared shaper, and we apply the same rationale there.
> 
> What do you think?
Ok. I'll add them from the node's perspective. 
Thanks.

> 
> Regards,
> Cristian
  

Patch

diff --git a/lib/librte_ethdev/rte_tm.h b/lib/librte_ethdev/rte_tm.h
index f9c0cf3..38fff4c 100644
--- a/lib/librte_ethdev/rte_tm.h
+++ b/lib/librte_ethdev/rte_tm.h
@@ -250,6 +250,23 @@  struct rte_tm_capabilities {
 	 */
 	uint64_t shaper_private_rate_max;
 
+	/** Shaper private packet mode supported. When non-zero, this parameter
+	 * indicates that there is atleast one node that can be configured
+	 * with packet mode in it's private shaper. When shaper is configured
+	 * in packet mode, committed/peak rate provided is interpreted
+	 * in packets per second.
+	 */
+	int shaper_private_packet_mode_supported;
+
+	/** Shaper private byte mode supported. When non-zero, this parameter
+	 * indicates that there is atleast one node that can be configured
+	 * with byte mode in it's private shaper. When shaper is configured
+	 * in byte mode, committed/peak rate provided is interpreted in
+	 * bytes per second.
+	 */
+	int shaper_private_byte_mode_supported;
+
+
 	/** Maximum number of shared shapers. The value of zero indicates that
 	 * shared shapers are not supported.
 	 */
@@ -284,6 +301,21 @@  struct rte_tm_capabilities {
 	 */
 	uint64_t shaper_shared_rate_max;
 
+	/** Shaper shared packet mode supported. When non-zero, this parameter
+	 * indicates a shared shaper can be configured with packet mode.
+	 * When shared shaper is configured in packet mode, committed/peak rate
+	 * provided is interpreted in packets per second.
+	 */
+	int shaper_shared_packet_mode_supported;
+
+	/** Shaper shared byte mode supported. When non-zero, this parameter
+	 * indicates that a shared shaper can be configured with byte mode.
+	 * When shared shaper is configured in byte mode, committed/peak rate
+	 * provided is interpreted in bytes per second.
+	 */
+	int shaper_shared_byte_mode_supported;
+
+
 	/** Minimum value allowed for packet length adjustment for any private
 	 * or shared shaper.
 	 */
@@ -339,6 +371,22 @@  struct rte_tm_capabilities {
 	 */
 	uint32_t sched_wfq_weight_max;
 
+	/** WFQ packet mode supported. When non-zero, this parameter indicates
+	 * that there is at least one non-leaf node that supports packet mode
+	 * for WFQ among its children. WFQ weights will be applied against
+	 * packet count for scheduling children when a non-leaf node
+	 * is configured appropriately.
+	 */
+	int sched_wfq_packet_mode_supported;
+
+	/** WFQ byte mode supported. When non-zero, this parameter indicates
+	 * that there is at least one non-leaf node that supports byte mode
+	 * for WFQ among its children. WFQ weights will be applied against
+	 * bytes for scheduling children when a non-leaf node is configured
+	 * appropriately.
+	 */
+	int sched_wfq_byte_mode_supported;
+
 	/** 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
@@ -485,6 +533,24 @@  struct rte_tm_level_capabilities {
 			 */
 			uint64_t shaper_private_rate_max;
 
+			/** Shaper private packet mode supported. When non-zero,
+			 * this parameter indicates there is atleast one
+			 * non-leaf node at this level that can be configured
+			 * with packet mode in its private shaper. When private
+			 * shaper is configured in packet mode, committed/peak
+			 * rate provided is interpreted in packets per second.
+			 */
+			int shaper_private_packet_mode_supported;
+
+			/** Shaper private byte mode supported. When non-zero,
+			 * this parameter indicates there is atleast one
+			 * non-leaf node at this level that can be configured
+			 * with byte mode in its private shaper. When private
+			 * shaper is configured in byte mode, committed/peak
+			 * rate provided is interpreted in bytes per second.
+			 */
+			int shaper_private_byte_mode_supported;
+
 			/** Maximum number of shared shapers that any non-leaf
 			 * node on this level can be part of. The value of zero
 			 * indicates that shared shapers are not supported by
@@ -554,6 +620,25 @@  struct rte_tm_level_capabilities {
 			 */
 			uint32_t sched_wfq_weight_max;
 
+			/** WFQ packet mode supported. When non-zero, this
+			 * parameter indicates that there is at least one
+			 * non-leaf node at this level that supports packet
+			 * mode for WFQ among its children. WFQ weights will
+			 * be applied against packet count for scheduling
+			 * children when a non-leaf node is configured
+			 * appropriately.
+			 */
+			int sched_wfq_packet_mode_supported;
+
+			/** WFQ byte mode supported. When non-zero, this
+			 * parameter indicates that there is at least one
+			 * non-leaf node at this level that supports byte
+			 * mode for WFQ among its children. WFQ weights will
+			 * be applied against bytes for scheduling children
+			 * when a non-leaf node is configured appropriately.
+			 */
+			int sched_wfq_byte_mode_supported;
+
 			/** 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
@@ -596,6 +681,24 @@  struct rte_tm_level_capabilities {
 			 */
 			uint64_t shaper_private_rate_max;
 
+			/** Shaper private packet mode supported. When non-zero,
+			 * this parameter indicates there is atleast one leaf
+			 * node at this level that can be configured with
+			 * packet mode in its private shaper. When private
+			 * shaper is configured in packet mode, committed/peak
+			 * rate provided is interpreted in packets per second.
+			 */
+			int shaper_private_packet_mode_supported;
+
+			/** Shaper private byte mode supported. When non-zero,
+			 * this parameter indicates there is atleast one leaf
+			 * node at this level that can be configured with
+			 * byte mode in its private shaper. When private shaper
+			 * is configured in byte mode, committed/peak rate
+			 * provided is interpreted in bytes per second.
+			 */
+			int shaper_private_byte_mode_supported;
+
 			/** Maximum number of shared shapers that any leaf node
 			 * on this level can be part of. The value of zero
 			 * indicates that shared shapers are not supported by
@@ -686,6 +789,20 @@  struct rte_tm_node_capabilities {
 	 */
 	uint64_t shaper_private_rate_max;
 
+	/** Shaper private packet mode supported. When non-zero, this parameter
+	 * indicates private shaper of current node can be configured with
+	 * packet mode. When configured in packet mode, committed/peak rate
+	 * provided is interpreted in packets per second.
+	 */
+	int shaper_private_packet_mode_supported;
+
+	/** Shaper private byte mode supported. When non-zero, this parameter
+	 * indicates private shaper of current node can be configured with
+	 * byte mode. When configured in byte mode, committed/peak rate
+	 * provided is interpreted in bytes per second.
+	 */
+	int shaper_private_byte_mode_supported;
+
 	/** Maximum number of shared shapers the current node can be part of.
 	 * The value of zero indicates that shared shapers are not supported by
 	 * the current node.
@@ -735,6 +852,23 @@  struct rte_tm_node_capabilities {
 			 * WFQ weight, so WFQ is reduced to FQ.
 			 */
 			uint32_t sched_wfq_weight_max;
+
+			/** WFQ packet mode supported. When non-zero, this
+			 * parameter indicates that current node supports packet
+			 * mode for WFQ among its children. WFQ weights will be
+			 * applied against packet count for scheduling children
+			 * when configured appropriately.
+			 */
+			int sched_wfq_packet_mode_supported;
+
+			/** WFQ byte mode supported. When non-zero, this
+			 * parameter indicates that current node supports byte
+			 * mode for WFQ among its children. WFQ weights will be
+			 * applied against  bytes for scheduling children when
+			 * configured appropriately.
+			 */
+			int sched_wfq_byte_mode_supported;
+
 		} nonleaf;
 
 		/** Items valid only for leaf nodes. */
@@ -836,10 +970,10 @@  struct rte_tm_wred_params {
  * Token bucket
  */
 struct rte_tm_token_bucket {
-	/** Token bucket rate (bytes per second) */
+	/** Token bucket rate (bytes per second or packets per second) */
 	uint64_t rate;
 
-	/** Token bucket size (bytes), a.k.a. max burst size */
+	/** Token bucket size (bytes or packets), a.k.a. max burst size */
 	uint64_t size;
 };
 
@@ -860,6 +994,11 @@  struct rte_tm_token_bucket {
  * Dual rate shapers use both the committed and the peak token buckets. The
  * rate of the peak bucket has to be bigger than zero, as well as greater than
  * or equal to the rate of the committed bucket.
+ *
+ * @see struct rte_tm_capabilities::shaper_private_packet_mode_supported
+ * @see struct rte_tm_capabilities::shaper_private_byte_mode_supported
+ * @see struct rte_tm_capabilities::shaper_shared_packet_mode_supported
+ * @see struct rte_tm_capabilities::shaper_shared_byte_mode_supported
  */
 struct rte_tm_shaper_params {
 	/** Committed token bucket */
@@ -874,6 +1013,17 @@  struct rte_tm_shaper_params {
 	 * RTE_TM_ETH_FRAMING_OVERHEAD_FCS).
 	 */
 	int32_t pkt_length_adjust;
+
+	/** When zero, the private or shared shaper that is associated to this
+	 * profile works in byte mode and hence *rate* and *size* fields in
+	 * both token bucket configurations are specified in bytes per second
+	 * and bytes respectively.
+	 * When non-zero, that private or shared shaper works in packet mode and
+	 * hence *rate* and *size* fields in both token bucket configurations
+	 * are specified in packets per second and packets respectively. In
+	 * packet mode, *pkt_length_adjust* is ignored.
+	 */
+	int packet_mode;
 };
 
 /**
@@ -925,6 +1075,8 @@  struct rte_tm_node_params {
 			 * 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.
+			 * @see struct rte_tm_node_capabilities::sched_wfq_packet_mode_supported
+			 * @see struct rte_tm_node_capabilities::sched_wfq_byte_mode_supported
 			 */
 			int *wfq_weight_mode;