Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add tm support for shaper config in pkt mode

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

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