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 <ndabilpu...@marvell.com>
> > Sent: Monday, March 30, 2020 5:00 PM
> > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Thomas Monjalon
> > <tho...@monjalon.net>; Yigit, Ferruh <ferruh.yi...@intel.com>; Andrew
> > Rybchenko <arybche...@solarflare.com>
> > Cc: dev@dpdk.org; jer...@marvell.com; kka...@marvell.com; Nithin
> > Dabilpuram <ndabilpu...@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 <ndabilpu...@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
