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

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Nithin Dabilpuram <ndabilpuram at marvell.com>
> Sent: Tuesday, April 7, 2020 6:21 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>
> Cc: Thomas Monjalon <thomas at monjalon.net>; Yigit, Ferruh
> <ferruh.yigit at intel.com>; Andrew Rybchenko
> <arybchenko at solarflare.com>; dev at dpdk.org; jerinj at marvell.com;
> kkanas at 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 at marvell.com>
> > > Sent: Monday, March 30, 2020 5:00 PM
> > > To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>; Thomas
> Monjalon
> > > <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>;
> Andrew
> > > Rybchenko <arybchenko at solarflare.com>
> > > Cc: dev at dpdk.org; jerinj at marvell.com; kkanas at marvell.com; Nithin
> > > Dabilpuram <ndabilpuram at 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 at 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