Re: [dpdk-dev] [EXT] RE: [PATCH 1/2] ethdev: add tm cap for private shaper packet mode

[Nithin Dabilpuram <ndabilpuram@marvell.com>]

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